Shared CFG: allow destructuring in ForeachStmt#22047
Open
owen-mc wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the shared CFG library’s ForeachStmt handling to support languages whose foreach/range loops conceptually produce an implicit “current element” value that must be destructured into multiple loop variables (for example, Go’s for k, v := range m).
Changes:
- Documents that
ForeachStmt.getVariable()may be absent when a language needs multi-variable destructuring. - Adds an opt-in hook
foreachHasElementNode(ForeachStmt)to request a synthesized per-iteration element node. - Updates shared CFG routing to enter the per-iteration element node (when opted in) and avoids routing
getVariable() -> bodyin that mode.
Show a summary per file
| File | Description |
|---|---|
| shared/controlflow/codeql/controlflow/ControlFlowGraph.qll | Adds an opt-in per-iteration foreach element node and updates CFG wiring/docs to support destructuring-based foreach semantics. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
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.
This commit is taken from #21614. The following commit in that PR shows how Go uses the changes in this commit to implement go's range statement using the shared library's
ForeachStmt. Here are some examples showing the valid syntax for range statements:Design notes / alternatives considered
The shared library now lets a language opt in to a synthesized per-iteration
ForeachElementnode (foreachHasElementNode). The intended contract is:This is a deliberate extension point, not an omission: when a language opts in, the shared library intentionally does not emit the getVariable() → body edge and instead hands control to the language at [ForeachElement].
Two more conventional designs were considered and rejected:
Mirror C#: a single target node, with no change to the shared library.
C# already supports tuple foreach (
foreach (var (a, b) in xs)) without special CFG machinery —getVariable()returns a singleVariableDeclTuplenode and the generic expression CFG flows through its children to destructure. The equivalent for Go would be to present a single "target"/"pattern" node asgetVariable(). This doesn't transfer: Go's range bindings are not a single grouping AST node.for k, v := range mis two independent LHS expressions plus an implicit extraction from the iterator, so there is no existing node to hand to the shared library. TheForeachElementnode fills exactly this gap.Generalize
getVariable()to an indexedgetVariable(int i)and chain inside the shared library (variable 0 → variable 1 → … → body).This keeps all wiring in the shared library and produces a complete CFG with no language patching. It was prototyped and then rejected: Go's loop variables are not ordinary target expressions that can be evaluated in sequence — each is bound by extracting a component from the implicit current element. That per-variable extraction is language-specific and is more naturally owned by Go than modelled as a generic chain in the shared library.
Hence the chosen design: the shared library provides the loop skeleton plus a single opt-in element node, and Go wires
ForeachElement→ extract/assign → body itself. Java and C# do not opt in, keep their singlegetVariable(), and their generated CFG is byte-identical.