Skip to content

C#: Move handling of callables into shared control flow library#21743

Merged
hvitved merged 4 commits intogithub:mainfrom
hvitved:cfg/body-parts
Apr 23, 2026
Merged

C#: Move handling of callables into shared control flow library#21743
hvitved merged 4 commits intogithub:mainfrom
hvitved:cfg/body-parts

Conversation

@hvitved
Copy link
Copy Markdown
Contributor

@hvitved hvitved commented Apr 22, 2026

In preparation for adding parameters to the CFG, which we also want the shared CFG library to handle.

@hvitved hvitved added the no-change-note-required This PR does not need a change note label Apr 22, 2026
@hvitved hvitved marked this pull request as ready for review April 22, 2026 11:43
@hvitved hvitved requested a review from a team as a code owner April 22, 2026 11:43
Copilot AI review requested due to automatic review settings April 22, 2026 11:43
@hvitved hvitved requested review from a team as code owners April 22, 2026 11:43
@hvitved hvitved requested a review from aschackmull April 22, 2026 11:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors callable-handling in the shared control-flow library to support upcoming CFG parameter support, moving language-specific “multiple body parts” handling behind a shared interface.

Changes:

  • Extends the shared CFG AST signature with a CallableBodyPartContext and callableGetBodyPart hook.
  • Updates shared callable entry/exit and stepping logic to work with either a single callable body or multiple ordered body parts.
  • Adapts Java and C# CFG inputs to provide an appropriate CallableBodyPartContext (Void for Java, CompilationExt for C#) and moves C# constructor/initializer body-part handling into callableGetBodyPart.
Show a summary per file
File Description
shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Adds shared callable body-part abstraction and integrates it into shared CFG construction/consistency checks.
java/ql/lib/semmle/code/java/ControlFlowGraph.qll Wires Java into the new shared callable body-part interface using Void as context.
csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowGraph.qll Moves C# constructor/initializer callable handling to callableGetBodyPart and introduces CompilationExt as context.

Copilot's findings

Comments suppressed due to low confidence (3)

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll:470

  • Wording in doc comment: "indexth" is awkward; consider using "index-th" (or "indexth" without code formatting) to avoid the grammatical glitch.
     * Gets the `index`th part of the body of `c` in context `ctx`.
     *

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll:472

  • Doc comment is a bit ambiguous because callableGetBody is a function: instead of "must never both hold", consider wording like "must never both produce a result" / "must not both be defined" for the same callable c.
     * `callableGetBodyPart(c, _, _)` and `callableGetBody(c)` must never both hold
     * for a given callable `c`.

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll:2183

  • The comment "included in both callableGetBody and callableGetBodyPart" is unclear (since these are relations/functions rather than collections). Consider rephrasing to explicitly state that it holds when both callableGetBody(c) has a result and callableGetBodyPart(c, _, _) has a result for the same c.
          /**
           * Holds if `c` is included in both `callableGetBody` and `callableGetBodyPart`.
           */
  • Files reviewed: 3/3 changed files
  • Comments generated: 1

Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll
Comment thread csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Copy link
Copy Markdown
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Generally LGTM. I have a few slight preferences for some small tweaks, though.

Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
aschackmull
aschackmull previously approved these changes Apr 23, 2026
@hvitved
Copy link
Copy Markdown
Contributor Author

hvitved commented Apr 23, 2026

DCA reported alert difference for C#, which happens because in some cases the same Compilation can have multiple body parts at the same index. The CFG before would branch out to each part, while not ideal, it is better than skipping body parts, which would happen because the rank indices generated in the shared code were not consecutive. I have added a commit that uses dense ranking throughout in the shared library to prevent such cases.

@hvitved hvitved requested a review from aschackmull April 23, 2026 08:31
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
Comment thread shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Outdated
@hvitved hvitved merged commit eee5b06 into github:main Apr 23, 2026
137 of 140 checks passed
@hvitved hvitved deleted the cfg/body-parts branch April 23, 2026 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# Java no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants