Java/Cfg: Introduce new shared CFG library and replace the Java CFG. by aschackmull · Pull Request #21290 · github/codeql

aschackmull · 2026-02-06T16:36:30Z

This PR introduces a new shared CFG library, which encapsulated the control flow semantics of the most common AST constructs. Additionally, the Java CFG is replaced with an instantiation of this new library.
Control flow semantics of individual AST nodes are given in a modular non-recursive way, which allows seamless integration of the few Java-specific control flow definitions with the shared parts by way of simple union.

Any existing Java libraries with implicit reliance on the exact structure of the CFG have been updated.

Commit-by-commit review is strongly encouraged. There are a few initial setup commits, then the big commit with the main change followed by a long tail of commits to fix various tests and dependent Java libraries.

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

…YieldCompletions.

Copilot

Pull request overview

This PR introduces a new shared CFG library that encapsulates control flow semantics of common AST constructs and replaces the Java CFG implementation with an instantiation of this library. The change enables modular, non-recursive control flow definitions that integrate Java-specific semantics seamlessly with shared components.

Changes:

Introduces a new ConditionKind abstraction to classify different conditional successor types (Boolean, Nullness, Matching, Emptiness)
Adds performance optimizations to Guards.qll using pragma[inline_late] and bindingset annotations
Replaces the Java CFG implementation with a shared library instantiation
Updates numerous test expectations to reflect the new CFG structure with additional synthetic nodes
Removes deprecated Completion.qll and updates SSA, dataflow, and guards implementations
Adds test utility modules for working with AST-based CFG views

Reviewed changes

Copilot reviewed 140 out of 143 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
shared/controlflow/codeql/controlflow/SuccessorType.qll	Adds ConditionKind abstraction and getDual() method for conditional successors
shared/controlflow/codeql/controlflow/Guards.qll	Adds strictlyDominatesCheck helper with pragma annotations for performance
shared/controlflow/codeql/controlflow/BasicBlock.qll	Adds getEnclosingCallable() method as alias for getScope()
java/ql/test/*/.expected	Updates test expectations to reflect new CFG nodes and edges
java/ql/test/*/.ql	Updates test queries to use AST-based CFG successors
java/ql/test/*/FalseSuccessors.	Removes FalseSuccessors test files (no longer needed)
java/ql/lib/semmle/code/java/metrics/MetricCallable.qll	Updates switch case analysis to use AST structure instead of CFG
java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll	Removes hasDominanceInformation checks and updates CFG node access
java/ql/lib/semmle/code/java/dataflow/TypeFlow.qll	Fixes instanceof disjunction analysis for single-predecessor cases
java/ql/lib/semmle/code/java/controlflow/Guards.qll	Simplifies switch case edge detection using MatchingSuccessor
java/ql/lib/utils/test/*.qll	Adds test utilities for AST-based CFG views
java/ql/lib/change-notes/2026-02-18-cfg.md	Documents breaking changes

owen-mc

I haven't looked at it all in detail.

owen-mc · 2026-02-24T11:30:54Z

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

+   * An example of this are resource declarations in Java's try-with-resources
+   * statement.
+   */
+  default AstNode getTryInit(TryStmt try, int index) { none() }


Out of interest, why not make this a member predicate on TryStmt, which is clearly marked as being optional?

Because most languages don't need it and making it a member means that it can't have a default implementation, so it ends up mandatory and ruins any chances that a language-specific instantiation can provide TryStmt as a simple alias.

Ah. I didn't know it couldn't have a default implementation as a member predicate.

owen-mc · 2026-02-24T11:44:11Z

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

+   * Gets an integer indicating the control flow order of a case within a switch.
+   * This is most often the same as the AST order, but can be different in some
+   * languages if the language allows a default case to appear before other
+   * cases.


I see that this doesn't have to be contiguous. It would be nice to state that here somehow.

Suggested change

* Gets an integer indicating the control flow order of a case within a switch.

* This is most often the same as the AST order, but can be different in some

* languages if the language allows a default case to appear before other

* cases.

* Gets an integer indicating the control flow order of a case within a switch.

* This is most often the same as the AST order, but can be different in some

* languages if the language allows a default case to appear before other

* cases.

*

* The values do not need to be contiguous; only the relative ordering matters.

owen-mc · 2026-02-24T13:43:51Z

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

+     * Holds if `n` is in a conditional context of kind `kind`. For example,
+     * the left-hand side of a short-circuiting `&&` expression is in a
+     * boolean conditional context.


I suggest making it clearer that this predicate is for adding extra cases on top of the ones already included by default. I also noticed this for beginAbruptCompletion, and there are probably other places.

owen-mc · 2026-02-24T13:53:19Z

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

-      or
-      result.getIndex() <= getFirstPatternCase(switch).getIndex()
+  /** Holds if this catch clause catches all exceptions. */
+  predicate catchAll(Ast::CatchClause catch) {


It feels like catchAll and matchAll are out of order. I expected them below additionalNode.

hvitved

Very nice work; I'm really happy with the design.

shared/controlflow/codeql/controlflow/BasicBlock.qll

hvitved · 2026-02-25T12:19:25Z

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

+          // the `isAfter(exprstmt.getExpr())` to `isAfter(exprstmt)` case is handled by `propagatesValue` above.
+        )
+        or
+        exists(BlockStmt blockstmt |


I would have expected ExprStmts and BlockStmts to be handled by default left-to-right evaluation; perhaps by extending getChild inside the shared library where the 0 child of an ExprStmt is the expression and the ith child of a BlockStmt is the ith statement within.

I guess we could do that if we make the default left-to-right case take propagatesValue into account. I can try it out to see if it looks cleaner that way.

Agreed to do this follow-up.

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

hvitved · 2026-02-26T13:28:36Z

java/ql/lib/utils/test/BasicBlock.qll

Is this perhaps something that should be put into the shared library?

Possibly. It was mildly useful for making sense of the Java tests across the CFG upheaval. I guess it might be equally useful for C# when I get there, so that seems like a reasonable time to pull it out into shared code.

java/ql/lib/semmle/code/java/controlflow/Guards.qll

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

hvitved · 2026-02-27T08:31:31Z

java/ql/lib/utils/test/AstCfg.qll

Could be in the shared library.

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

hvitved · 2026-03-02T09:49:10Z

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

+          // `(x || y) ?? z`, the `||` may short-circuit with a known boolean
+          // value `t`, but it occurs in a nullness conditional context, which
+          // means that the `t0` has nullness kind. In these cases we check
+          // whether there is an implication that allows translatiion from `t`


translatiion -> translation

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

hvitved · 2026-03-02T09:56:58Z

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

+            n2 = beforeBody
+            or
+            n1.isAfterFalse(case.getGuard()) and
+            n2.isAfterValue(case, any(MatchingSuccessor t | t.getValue() = false))


Agreed to do this follow-up.

hvitved · 2026-03-02T10:01:55Z

java/ql/lib/semmle/code/java/controlflow/BasicBlocks.qll

 /** A basic block that ends in an exit node. */
 class ExitBlock extends BasicBlock {
  ExitBlock() { this.getLastNode() instanceof ControlFlow::ExitNode }
 }


hvitved · 2026-03-02T10:02:45Z

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

-    /** Gets the statement this `Node` corresponds to, if any. */
-    Stmt asStmt() { this = TStmtNode(result) }
+/** Provides an implementation of the AST signature for Java. */
+module Ast implements AstSig<Location> {


In that case it should probably be moved into some internal module, I doubt we want to expose it?

hvitved · 2026-03-02T10:03:15Z

java/ql/lib/utils/test/BasicBlock.qll

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

github-actions bot added Java Kotlin labels Feb 6, 2026

github-advanced-security bot found potential problems Feb 6, 2026

View reviewed changes

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Fixed Show fixed Hide fixed

aschackmull force-pushed the cfg/new-shared branch 6 times, most recently from f54bdf4 to 0e7c3ba Compare February 16, 2026 14:04

aschackmull force-pushed the cfg/new-shared branch 4 times, most recently from 3437f86 to b90ee55 Compare February 18, 2026 09:25

github-advanced-security bot found potential problems Feb 18, 2026

View reviewed changes

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Fixed Show fixed Hide fixed

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Fixed Show fixed Hide fixed

aschackmull force-pushed the cfg/new-shared branch from b90ee55 to 47a45ce Compare February 18, 2026 09:35

github-actions bot added the documentation label Feb 18, 2026

aschackmull force-pushed the cfg/new-shared branch 6 times, most recently from df51520 to 75b9346 Compare February 23, 2026 14:05

aschackmull added 8 commits February 23, 2026 15:09

Cfg: Add ConditionKind and getDual to ConditionalSuccessor.

723a896

Java: Preparatory tweaks.

2e98734

Java: Replace ControlFlowNode.asCall with Call.getControlFlowNode.

4a97a44

Cfg: Add getEnclosingCallable to shared BasicBlock

48d7d9c

Java: Replace idominance tests.

0c9931f

Java/Cfg: Introduce new shared CFG library and replace the Java CFG.

48e3724

Java: Fix Cyclomatic complexity calculation.

6fbdb2c

Java: Fix RangeAnalysis/ModulusAnalysis.

1e9dcea

aschackmull added 13 commits February 23, 2026 15:09

Java: Accept dispatch precision improvement.

a72cf56

Java: Fix enhancedForEarlyExit implementation.

b798bc2

Java: Accept new TP in NullMaybe.

8b0dd7b

Java: Accept removal of spurious reason (the alert stays).

d84e0e2

Java: Accept reduced precision from no longer nesting completions in …

106a9d4

…YieldCompletions.

Java: Accept revised CFG.

eb37c41

Java: Remove obsolete tests - false successors are no longer special.

352b371

Java: Enable Cfg consistency checks.

f7317b6

Java: Adjust switch case guards test.

d4873dd

Java: Add change note.

0d0711f

Java: Handle missing throws clauses.

bdbbd45

Java: Add nullness test covering known FP.

2b8e719

Guards: Improve join-order.

94121f1

aschackmull force-pushed the cfg/new-shared branch from 75b9346 to 94121f1 Compare February 23, 2026 14:10

aschackmull marked this pull request as ready for review February 23, 2026 14:13

aschackmull requested a review from a team as a code owner February 23, 2026 14:13

Copilot AI review requested due to automatic review settings February 23, 2026 14:13

aschackmull requested review from a team as code owners February 23, 2026 14:13

Copilot started reviewing on behalf of aschackmull February 23, 2026 14:14 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

owen-mc reviewed Feb 24, 2026

View reviewed changes

hvitved requested changes Feb 27, 2026

View reviewed changes

Cfg: Address review comments.

ab94524

hvitved reviewed Mar 2, 2026

View reviewed changes

Cfg: A few more review tweaks.

627654c

hvitved approved these changes Mar 2, 2026

View reviewed changes

aschackmull merged commit e695477 into github:main Mar 2, 2026
143 of 145 checks passed

aschackmull deleted the cfg/new-shared branch March 2, 2026 12:57

Conversation

aschackmull commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

owen-mc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aschackmull commented Feb 6, 2026 •

edited

Loading