Skip to content

Shared: update code comments explaining models-as-data format to include barriers and barrier guards - #21584

Merged
owen-mc merged 10 commits into
github:mainfrom
owen-mc:shared/update-mad-comments
Apr 14, 2026
Merged

Shared: update code comments explaining models-as-data format to include barriers and barrier guards#21584
owen-mc merged 10 commits into
github:mainfrom
owen-mc:shared/update-mad-comments

Conversation

@owen-mc

Copy link
Copy Markdown
Contributor

I have also copied some sections that were missing for some languages and corrected the explanation of neutral models. These changes have been done in separate commits.

@owen-mcowen-mc added the no-change-note-required This PR does not need a change note label Mar 26, 2026
@owen-mc
owen-mc marked this pull request as ready for review March 26, 2026 11:47
@owen-mc
owen-mc requested review from a team as code ownersMarch 26, 2026 11:47
CopilotAI review requested due to automatic review settings March 26, 2026 11:47
@owen-mc
owen-mc requested review from a team as code ownersMarch 26, 2026 11:47

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates Models-as-Data / API-graph model documentation comments across several languages to describe barrier and barrier-guard model formats, and to refine the description of neutral models.

Changes:

  • Document Barriers and BarrierGuards row formats in multiple MaD / ApiGraph model comment blocks.
  • Add/expand documentation for barrier-guard condition columns (branch / acceptingvalue).
  • Adjust neutral-model description text in ExternalFlow docs.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
FileDescription
rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qllUpdates MaD format comment to include barriers/guards and neutrals.
ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qllDocuments Barriers/BarrierGuards rows and branch meaning for API-graph models.
python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qllDocuments Barriers/BarrierGuards rows and branch meaning for API-graph models.
javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qllDocuments Barriers/BarrierGuards rows and branch meaning for API-graph models.
java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qllDocuments Barriers/BarrierGuards and adjusts neutral-model description.
go/ql/lib/semmle/go/dataflow/ExternalFlow.qllDocuments Barriers/BarrierGuards and provenance; adjusts neutral-model description.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qllDocuments Barriers/BarrierGuards and adjusts neutral-model description.
cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qllDocuments Barriers/BarrierGuards, acceptingvalue, and provenance.
Comments suppressed due to low confidence (3)

cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll:96

  • Minor grammar: "In future" should be "In the future," (or similar) in this sentence.
 * "false". In future "no-exception", "not-zero", "null", "not-null" may be
* supported.

rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll:18

  • The Neutrals row/description here doesn't match the rest of this module: the file defines neutralModel(string path, string kind, string provenance, ...) and later states kind can be source, sink, or summary. Please align the Neutrals row columns and the neutral explanation with the neutralModel predicate and its documented effect below.
 * - Neutrals:
* `package; type; name; signature; kind; provenance`
* A neutral is used to indicate that a callable is neutral with respect to flow (no summary).

rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll:46

  • This description says the barrier-guard branch column may support values like "no-exception"/"not-zero" in the future, but the barrierGuardModel documentation below states branch is either true or false (a branch selector). To avoid conflicting documentation, either keep branch restricted to true/false here or update the later barrierGuardModel docs to match the intended semantics.
 * 3. The `branch` column of barrier guard models specifies which branch of the
* guard is blocking flow. It can be "true" or "false". In future
* "no-exception", "not-zero", "null", "not-null" may be supported.

Comment threadrust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll Outdated
Comment threadjava/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll Outdated
Comment threadcsharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll Outdated
Comment threadgo/ql/lib/semmle/go/dataflow/ExternalFlow.qll Outdated
Comment threadcpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll Outdated
@owen-mc
owen-mcforce-pushed the shared/update-mad-comments branch from a782c0f to 1d0e402CompareMarch 26, 2026 12:17
* - BarrierGuards:
* `path; input; branch; kind; provenance`
* - Neutrals:
* `package; type; name; signature; kind; provenance`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be updated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* - "Property[p]": Selects the contents of property `p`.
*
* 8. The `kind` column is a tag that can be referenced from QL to determine to
* 8. The `acceptingvalue` column of barrier guard models specifies the condition

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Rust it is called branch; we should probably align on this.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you want me to do it in this PR. I think it's equally good as a follow-up or in the future when rust adopts the shared guards library.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps simply rename it to acceptingvalue (I would actually prefer acceptingValue) so that all languages are aligned.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, now I look I see that all the dynamic languages call it branch. Shall I change it for all of them?

@michaelnebelmichaelnebel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this!

@owen-mc
owen-mcforce-pushed the shared/update-mad-comments branch from 1d0e402 to 5451424CompareMarch 27, 2026 09:48

@aschackmullaschackmull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaelnebelmichaelnebel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

yoff
yoff approved these changes Apr 7, 2026

@yoffyoff left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python 👍

@owen-mc
owen-mc merged commit 7458674 into github:mainApr 14, 2026
149 checks passed
@owen-mc
owen-mc deleted the shared/update-mad-comments branch April 14, 2026 08:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C#C++DataFlow LibraryGoJavaJSno-change-note-requiredThis PR does not need a change notePythonRubyRustPull requests that update Rust codeSwift

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@owen-mc@yoff@hvitved@michaelnebel@aschackmull