Shared: Add a shared SuccessorType implementation #20300

aschackmull · 2025-08-27T12:22:24Z

This adds a shared implementation of the SuccessorType class that's used as edge labels in our CFGs. I've also updated all languages and all shared libraries to use this new shared code.

Commit-by-commit review is encouraged.

A few notes:

SuccessorType is now a shared non-parameterized implementation. This means that it's not using language-specific details like exception type or goto-labels.
Splitting in C# on finally blocks use SuccessorType, so the change has a slight semantic impact here. Initially I tried a version that bunched all of the jump successors together, but that gave a single new FP in C# nullness, so I choose to have a more detailed distinction between different jump successors.
I've kept the successorTypeIsCondition predicate in a few places in order to not disrupt basic block boundaries too much. Streamlining this across languages is left as a potential follow-up consideration.

csharp/ql/lib/semmle/code/csharp/controlflow/internal/Completion.qll

  ExceptionClass getExceptionClass() { result = ec }

-  override ExceptionSuccessor getAMatchingSuccessorType() { result.getExceptionClass() = ec }
+  override ExceptionSuccessor getAMatchingSuccessorType() { any() }


ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll

  }

-  override RaiseSuccessor getAMatchingSuccessorType() { any() }
+  override ExceptionSuccessor getAMatchingSuccessorType() { any() }


ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

swift/ql/lib/codeql/swift/controlflow/internal/Completion.qll

  FallthroughCompletion() { this = TFallthroughCompletion(dest) }

-  override FallthroughSuccessor getAMatchingSuccessorType() { any() }
+  override DirectSuccessor getAMatchingSuccessorType() { any() }


csharp/ql/lib/semmle/code/csharp/controlflow/internal/Completion.qll

  string getLabel() { result = label }

-  override GotoSuccessor getAMatchingSuccessorType() { result.getLabel() = label }
+  override GotoSuccessor getAMatchingSuccessorType() { any() }


ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll

  }

-  override NextSuccessor getAMatchingSuccessorType() { any() }
+  override ContinueSuccessor getAMatchingSuccessorType() { any() }


Copilot

Pull Request Overview

This PR adds a shared implementation of the SuccessorType class used as edge labels in control flow graphs (CFGs). The implementation centralizes successor type definitions in the shared/controlflow module and updates all languages to use this shared code instead of maintaining language-specific implementations.

Key changes include:

Adding a comprehensive shared SuccessorType implementation with a well-defined hierarchy
Removing language-specific successor type implementations
Updating all languages to import and use the shared successor types

Reviewed Changes

Copilot reviewed 64 out of 68 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
shared/controlflow/codeql/controlflow/SuccessorType.qll	New shared successor type implementation with comprehensive hierarchy
shared/controlflow/codeql/controlflow/Guards.qll	Updated to import shared successor types
shared/controlflow/codeql/controlflow/Cfg.qll	Updated to use shared successor types and removed language-specific definitions
shared/controlflow/codeql/controlflow/BasicBlock.qll	Updated to import shared successor types
swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll	Updated to use shared `BooleanSuccessor` type
swift/ql/lib/codeql/swift/controlflow/*	Removed Swift-specific successor types and updated to use shared implementation
rust/ql/lib/codeql/rust/controlflow/*	Removed Rust-specific successor types and updated to use shared implementation
ruby/ql/lib/codeql/ruby/controlflow/*	Removed Ruby-specific successor types and updated to use shared implementation
java/ql/lib/semmle/code/java/controlflow/SuccessorType.qll	Removed Java-specific successor type implementation
csharp/ql/test/library-tests/goto/Goto1.expected	Updated test expectations for goto successor label changes
ruby/ql/test/library-tests/controlflow/graph/*.expected	Updated test expectations for successor type label changes

Comments suppressed due to low confidence (1)

swift/ql/lib/codeql/swift/controlflow/internal/Completion.qll:1

The change from MatchSuccessor to MatchingSuccessor suggests a naming inconsistency between the old Swift implementation and the new shared implementation. Ensure all references are updated consistently.

/**

Mostly toString changes, and a slight change to splitting in C#.

hvitved · 2025-09-01T12:52:31Z

shared/controlflow/codeql/controlflow/SuccessorType.qll

+private import codeql.util.Boolean
+
+/*
+ * SuccessorType


I would add this as ```text QL doc to the class.

hvitved · 2025-09-01T12:55:03Z

shared/controlflow/codeql/controlflow/SuccessorType.qll

+ */
+class SuccessorType extends TSuccessorType {
+  /** Gets a textual representation of this successor type. */
+  abstract string toString();


We should not be exposing abstract classes or predicates. Instead, apply the usual private Impl + final external alias trick.

hvitved · 2025-09-01T12:56:28Z

shared/controlflow/codeql/controlflow/SuccessorType.qll

+ *
+ * has a control flow graph containing Boolean successors:
+ *
+ * ```


I would add a text suffix to prevent VS code from attempting to syntax highlighting this. Same for other examples.

hvitved · 2025-09-01T12:59:17Z

shared/controlflow/codeql/controlflow/SuccessorType.qll

+}
+
+/** A Java `yield` control flow successor. */
+class JavaYieldSuccessor extends JumpSuccessor, TJavaYieldSuccessor {


Would it be weird for Java to reuse Break instead for this (it looks similar to how one can break with a value in Ruby)?

No that's probably sensible. I'll delete JavaYieldSuccessor.

hvitved · 2025-09-01T13:03:12Z

ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll

   * the `ensure` block executes normally).
   */
-  class EnsureSplitType extends SuccessorType {
+  class EnsureSplitType instanceof SuccessorType {


Revert this when SuccessorType is no longer abstract.

hvitved · 2025-09-01T13:03:17Z

ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll

    EnsureSplitType() { not this instanceof ConditionalSuccessor }

+    /** Gets a textual representation of this successor type. */
+    string toString() { result = super.toString() }


hvitved · 2025-09-01T13:05:07Z

csharp/ql/test/library-tests/controlflow/graph/EnclosingCallable.expected

-| CompileTimeOperators.cs:37:13:37:40 | [finally: goto(End)] call to method WriteLine | CompileTimeOperators.cs:28:10:28:10 | M |
-| CompileTimeOperators.cs:37:13:37:41 | [finally: goto(End)] ...; | CompileTimeOperators.cs:28:10:28:10 | M |
-| CompileTimeOperators.cs:37:31:37:39 | [finally: goto(End)] "Finally" | CompileTimeOperators.cs:28:10:28:10 | M |
+| CompileTimeOperators.cs:36:9:38:9 | [finally: goto] {...} | CompileTimeOperators.cs:28:10:28:10 | M |


Hmm, when we no longer store the target of the goto, how can we make the correct jump after finally?

Discussed offline. Summary: This is an intentional trade-off. We can still get the same level of precision in a non-split CFG as we typically use a more precise internal edge label called Completion during CFG construction. So this trade-off only relates to any splitting we may do based on SuccessorType. Dca didn't show any impact and having multiple different goto statements inside a try block is fairly corner-case.

hvitved · 2025-09-01T13:07:30Z

csharp/ql/test/library-tests/controlflow/graph/NodeGraph.expected

-| Finally.cs:114:19:114:35 | [finally: exception(Exception)] ... == ... | Finally.cs:114:17:114:36 | [false, finally: exception(Exception)] !... | true |
-| Finally.cs:114:19:114:35 | [finally: exception(Exception)] ... == ... | Finally.cs:114:17:114:36 | [true, finally: exception(Exception)] !... | false |
-| Finally.cs:114:19:114:35 | [finally: exception(NullReferenceException)] ... == ... | Finally.cs:114:17:114:36 | [false, finally: exception(NullReferenceException)] !... | true |
-| Finally.cs:114:19:114:35 | [finally: exception(NullReferenceException)] ... == ... | Finally.cs:114:17:114:36 | [true, finally: exception(NullReferenceException)] !... | false |
-| Finally.cs:114:19:114:35 | [finally: exception(OutOfMemoryException)] ... == ... | Finally.cs:114:17:114:36 | [false, finally: exception(OutOfMemoryException)] !... | true |
-| Finally.cs:114:19:114:35 | [finally: exception(OutOfMemoryException)] ... == ... | Finally.cs:114:17:114:36 | [true, finally: exception(OutOfMemoryException)] !... | false |
+| Finally.cs:114:19:114:35 | [finally: exception] ... == ... | Finally.cs:114:17:114:36 | [false, finally: exception] !... | true |


Same: When we don't record the exception type, we may not be able to get control flow right when a finally is inside a try.

hvitved · 2025-09-01T13:12:04Z

shared/controlflow/codeql/controlflow/SuccessorType.qll

 }
+
+/** Holds if `t` is an abnormal exit type out of a CFG scope. */
+predicate isAbnormalExitType(SuccessorType t) {


This file is reexported elsewhere, so not nice to have a top-level predicate reexported.

How about I'll make it a member on AbruptSuccessor instead, then?

Or maybe I'll just move it to Cfg.qll...

github-actions bot added C# Ruby labels Aug 27, 2025

github-advanced-security bot found potential problems Aug 27, 2025

View reviewed changes

aschackmull force-pushed the cfg/successortype branch from ce85279 to 75ea5f2 Compare August 28, 2025 08:03

github-actions bot added Rust Pull requests that update Rust code Java Swift Actions Analysis of GitHub Actions labels Aug 28, 2025

github-advanced-security bot found potential problems Aug 28, 2025

View reviewed changes

aschackmull force-pushed the cfg/successortype branch from d9a59a2 to 01fcc41 Compare August 29, 2025 08:26

github-advanced-security bot found potential problems Aug 29, 2025

View reviewed changes

aschackmull force-pushed the cfg/successortype branch from 01fcc41 to 0df7d6f Compare August 29, 2025 08:59

aschackmull marked this pull request as ready for review August 29, 2025 10:59

aschackmull requested review from a team as code owners August 29, 2025 10:59

Copilot AI review requested due to automatic review settings August 29, 2025 10:59

Copilot AI reviewed Aug 29, 2025

View reviewed changes

aschackmull added the no-change-note-required This PR does not need a change note label Sep 1, 2025

aschackmull added 8 commits September 1, 2025 12:47

Controlflow: Add a shared SuccessorType implementation.

8434dc3

C#: Use shared SuccessorType.

8b50ac2

Ruby: Use shared SuccessorType.

d8c193d

Rust: Use shared SuccessorType.

cf9196f

C#/Ruby: Accept qltest changes.

c1662cf

Mostly toString changes, and a slight change to splitting in C#.

Swift: Use shared SuccessorType.

1e25b4d

Actions: Use shared SuccessorType.

92fcda3

Java: Use shared SuccessorType.

4685b4f

aschackmull added 4 commits September 1, 2025 13:43

Shared: Use shared SuccessorType in shared Cfg and BasicBlock libs.

144e34c

C#: Fix caching dependencies.

bbf7995

Cfg: Allow for multiple exception successors.

0d9b8d0

Guards: Use shared SuccessorType.

4e70627

aschackmull force-pushed the cfg/successortype branch from bcc61a5 to 4e70627 Compare September 1, 2025 11:55

aschackmull requested review from a team as code owners September 1, 2025 11:55

github-actions bot added JS C++ Python labels Sep 1, 2025

hvitved requested changes Sep 1, 2025

View reviewed changes

SuccessorType: Address review comments.

3d4d347

hvitved approved these changes Sep 2, 2025

View reviewed changes

aschackmull merged commit f833fe0 into github:main Sep 2, 2025
66 of 67 checks passed

aschackmull deleted the cfg/successortype branch September 2, 2025 12:09

MathiasVP added a commit to microsoft/codeql that referenced this pull request Sep 23, 2025

PS: Mirror the changes from github#20300.

3332f70

Shared: Add a shared SuccessorType implementation #20300

Shared: Add a shared SuccessorType implementation #20300

Uh oh!

Conversation

aschackmull commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Uh oh!

Check warning

Uh oh!

Check warning

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aschackmull commented Aug 27, 2025 •

edited

Loading