Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
External Go gotos targeting an inner stacked label still fail to connect to their destination.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Generalizes shared CFG abrupt completions to carry resolved jump targets, enabling precise Go and C# goto handling.
Changes:
- Adds resolved
JumpTargetsupport to the shared CFG. - Implements target resolution for Go and C#.
- Adds Go regression coverage and updates other language integrations.
| File | Description |
|---|---|
shared/controlflow/codeql/controlflow/ControlFlowGraph.qll |
Adds targeted abrupt-completion propagation. |
go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll |
Resolves Go goto destinations. |
go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.ql |
Queries goto target reachability. |
go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.expected |
Records expected test results. |
go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go |
Supplies stacked-label test cases. |
csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowGraph.qll |
Resolves label, case, and default gotos. |
java/ql/lib/semmle/code/java/ControlFlowGraph.qll |
Implements the extended shared interface. |
python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll |
Implements the extended shared interface. |
ruby/ql/lib/codeql/ruby/controlflow/ControlFlowGraph.qll |
Implements the extended shared interface. |
unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll |
Implements the extended shared interface. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
owen-mc
force-pushed
the
shared/cfg/goto-targets
branch
2 times, most recently
from
September 28, 2026 23:43
ba548eb to
86ead25
Compare
owen-mc
force-pushed
the
shared/cfg/goto-targets
branch
from
September 29, 2026 08:42
86ead25 to
832228f
Compare
owen-mc
force-pushed
the
shared/cfg/goto-targets
branch
from
September 30, 2026 15:27
832228f to
22dea67
Compare
owen-mc
changed the title
owen-mc
changed the title
owen-mc
force-pushed
the
shared/cfg/goto-targets
branch
from
September 30, 2026 15:56
8e98770 to
e7d3ae3
Compare
owen-mc
force-pushed
the
shared/cfg/goto-targets
branch
from
September 30, 2026 22:04
c08ea98 to
fc044f4
Compare
Contributor
Author
|
@aschackmull I've updated this PR to implement the approach you outlined in your review of Go: fix CFG for nested labels. Please can you have a look. |
aschackmull
reviewed
Oct 1, 2026
aschackmull
reviewed
Oct 1, 2026
aschackmull
reviewed
Oct 1, 2026
Comment on lines
+1288
to
+1293
| /** Holds if `n` has `l`, possibly through enclosing labeled statements. */ | ||
| private predicate hasLabel(AstNode n, Input1::Label l) { | ||
| Input1::hasLabel(n, l) | ||
| or | ||
| exists(LabeledStmt labeled | labeled.getStmt() = n and hasLabel(labeled, l)) | ||
| } |
Contributor
There was a problem hiding this comment.
This predicate is semantically different from Input1::hasLabel, so it should have a different name. Also, it's never needed for nodes, n, for which Input1::hasLabel(n, _) holds so we may use strict enclosure. That means we can simplify using a TC:
Suggested change
| /** Holds if `n` has `l`, possibly through enclosing labeled statements. */ | |
| private predicate hasLabel(AstNode n, Input1::Label l) { | |
| Input1::hasLabel(n, l) | |
| or | |
| exists(LabeledStmt labeled | labeled.getStmt() = n and hasLabel(labeled, l)) | |
| } | |
| /** Holds if `n` is marked with a `LabeledStmt` with label `l`. */ | |
| private predicate hasEnclosingLabel(AstNode n, Input1::Label l) { | |
| exists(LabeledStmt labeled | labeled.getStmt+() = n and Input1::hasLabel(labeled, l)) | |
| } |
aschackmull
reviewed
Oct 1, 2026
aschackmull
reviewed
Oct 1, 2026
aschackmull
requested changes
Oct 1, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix nested and self-looping goto targets in the shared CFG by adding labeled statements to the shared AST interface and handling label propagation centrally.
Labels now propagate inward for labeled break/continue and outward through nested labeled statements for goto, avoiding incorrect extra targets. The change updates Go, Java, and C# integrations and adds Go tests for stacked labels, sibling labels, and direct self-loops.