Unified: captured variables by asgerf · Pull Request #22726 · github/codeql

asgerf · 2026-10-01T14:12:28Z

Adds flow through captured variables and improves handling of capture-declarations on closure expressions.

Data flow node splitting

The TDataFlowNode type is now split into multiple stages, and the language plugin currently only contributes to the first stage (before SSA and capture instantiation).

This is a slightly large change to bring out just for capture, but it is also needed for other useful features like transform steps and withContent/withoutContent steps. For capture we use it to perform simple alias analysis for closures without duplicating logic already expressed in the form of data flow steps.

Capture declarations

Closures can specify that certain variables are captured "by value"

let closure1 = { print(x) }        // capture 'x' by reference
let closure2 = { [x] in print(x) } // capture 'x' by value

We interpret such capture-declarations essentially as if they were a let-bound variable sitting immediately around the closure expression. For example, the second closure is essentially seen as:

let closure2 = let x' = x in { print (x') }

This is implemented via two changes:

[x] is desugared to [x = x] that is, the variable x is initialised to x. Shadowing rules ensure that a new x binding is introduced and its RHS refers to the outer x binding.
Scoping and CFG rules are updated so FunctionExpr.getCaptureDeclaration(_) is placed "around" the closure, rather than inside it.

+      /**
+       * The given `read` reads multiple variables at once. `var` is bound to one of them.
+       */


+/**
+ * Provides data-flow modelling of constructor patterns / enum-case constructors.
+ */


asgerf · 2026-10-09T10:58:43Z

Rerun has been triggered: 5 restarted 🚀

Copilot

🔵 Needs a closer look

It substantially changes shared SSA, control-flow, and data-flow construction and warrants final human validation.

1 open finding

Fix typo: use Optional.some for weak capture · New

What changed in this PR

Adds captured-variable support to Unified Swift data flow, including closure ownership semantics and local-function captures.

Changes:

Integrates capture SSA and callable/callee data-flow nodes.
Models strong, weak, and unowned Swift captures.
Adds capture tests and updates generated expectations.

File	Description
`unified/ql/test/library-tests/local-name-binding/test.swift`	Updates local-function binding expectation.
`unified/ql/test/library-tests/local-name-binding/self_access.swift`	Tests explicit `self` capture modes.
`unified/ql/test/library-tests/dataflow/test.expected`	Updates generated capture-flow results.
`unified/ql/test/library-tests/dataflow/capture.swift`	Adds capture data-flow tests.
`unified/ql/test/library-tests/controlflow/cfg.swift`	Updates CFG expectations for function values.
`unified/ql/test/library-tests/controlflow/cfg.expected`	Updates generated CFG results.
`unified/ql/test/library-tests/controlflow/basicblock-slices.expected`	Updates generated basic-block results.
`unified/ql/lib/codeql/unified/internal/LocalNameBinding.qll`	Generalizes child ordering and capture binding.
`unified/ql/lib/codeql/unified/internal/FacadeAst.qll`	Associates function names with enclosing callables.
`unified/ql/lib/codeql/unified/internal/dataflow/ParameterPositions.qll`	Adds implicit callee positions.
`unified/ql/lib/codeql/unified/internal/dataflow/LocalVariableRefNode.qll`	Introduces reusable variable-reference nodes.
`unified/ql/lib/codeql/unified/internal/dataflow/LocalSsa.qll`	Adapts local SSA to staged nodes.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPluginSwift.qll`	Models weak and unowned captures.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPlugin.qll`	Moves plugin steps to builder nodes.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowNode.qll`	Adds staged, callable, and capture nodes.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowInstantiation.qll`	Integrates capture and lambda flow.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowGraph.qll`	Adds callable and capture graph edges.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowCallable.qll`	Adds callable conversion support.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowCall.qll`	Adds call conversion support.
`unified/ql/lib/codeql/unified/internal/dataflow/Content.qll`	Adds captured-variable content.
`unified/ql/lib/codeql/unified/internal/dataflow/CaptureSsa.qll`	Implements capture SSA integration.
`unified/ql/lib/codeql/unified/internal/dataflow/AllDataFlow.qll`	Re-exports new capture components.
`unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll`	Adds function-name CFG children.
`unified/ql/consistency-queries/CaptureSsaConsistency.ql`	Enables capture SSA consistency checks.
`unified/extractor/tests/corpus/swift/closures/closure-with-capture-list.output`	Updates generated capture AST output.
`unified/extractor/src/languages/swift/swift.rs`	Expands capture-list initializers.
`shared/dataflow/codeql/dataflow/VariableCapture.qll`	Refines shared capture analysis integration.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

This appeared after an LLM-assisted rebase

Rebasing on the Optional<T> bypass makes this part of the test pass.

paldepind · 2026-10-14T08:37:17Z

+                    match val {
+                        Some(val) => val,
+                        None => tree!((identifier #{name})),
+                    }


Could use unwrap_or_else.

Suggested change

match val {

Some(val) => val,

None => tree!((identifier #{name})),

}

val.unwrap_or_else(|| tree!((identifier #{name})))

aschackmull · 2026-10-14T12:10:25Z

+
+  LocalVariableRefNode() { this = TLocalVariableRefNode(repr, var, kind) }
+
+  predicate hasCfgNode(BasicBlock bb, int i) {


When this was factored out, the (kind.isRead() or kind.isPostUpdate()) and condition was dropped. Was that intentional?

Good question. It was technically unnecessary to begin with - I think it added some clarity originally because we never needed an Expr to represent a write, but there's no harm in having Expr representatives for writes.

aschackmull · 2026-10-14T13:02:38Z

+    predicate hasBody(Callable body) { callable = body }
+
+    predicate hasAliasedAccess(Expr f) {
+      callableHasLocalAlias(callable, f) and not f.(LocalVariableRefNode).getRefKind().isWrite()


This relation was not meant to be reflexive. Also, it was not meant to include all the aliases - just the ones that might escape or represent an invocation.

I'll leave a TODO for now. It's easy to accidentally make something too under-approximate and not notice because FNs are hard to detect. I want to over-approximate this first and defer escape analysis until we are in a better position to measure its correctness at scale.

aschackmull · 2026-10-14T13:04:22Z

+  }
+
+  class CapturedParameter extends CapturedVariable {
+    // This class is not needed. Variables aren't the first point of contact with parameter values.


Could you elaborate?

Fair enough, expanded on the comment.

aschackmull · 2026-10-14T13:11:05Z

+    )
+  }
+
+  class ClosureExpr extends Expr instanceof TCallableNode {


This equates closure expressions and callables. I'm not sure if this happens in Swift, but that won't work in general. Some languages allow the construction of closure objects with multiple methods, e.g. anonymous class instances in Java (of which a Java lambda is but one example).

A local class definition is another example where capture might happen in an object with multiple methods (again, I'm not sure which languages this applies to, but I know it's valid Java).

As discussed offline, I'll add a TODO. Swift local classes can't capture variables so we can't exercise this feature with Swift.

aschackmull · 2026-10-15T07:49:40Z


 private newtype TParameterPosition =
  TReceiverParameter() or
+  TCalleeParameter() or


Why do we need to introduce this? I would expect the receiver parameter to handle this role. I know that we already have the distinction between receiver parameter and callee parameter in some of our languages, but I'd like to question whether that's really necessary.

Note also that in some languages (e.g. Java), you can't tell the difference between these two cases.

Summary from our offline discussion:

The two parameter positions are strictly necessary for JS so attempts to merge them into one isn't going to be future proof.

The argument positions for a call x.foo() can be provided without knowing if this is a method call or a field read followed by a lambda call: x is the receiver argument and x.foo is the callee argument. This is important for dynamic languages where there is no type system to tell the difference.

Given the knowledge that call sites always pass in the receiver and callee as specified above, CaptureSsa.qll should be able to pick the right parameter position to use on a case by case basis (depending on the language and/or node type). We expect it will eventually use a mix of receiver/callee parameters when we start supporting a language where classes can capture variables (which Swift does not).

aschackmull · 2026-10-15T08:16:43Z

  }
 }
+
+DataFlowCall getDataFlowCall(CallExpr call) { result.asExplicitCall() = call }


The get prefix doesn't feel exactly right to me. How about toDataFlowCall instead?

aschackmull · 2026-10-15T08:17:24Z

  Location getLocation() { result = this.asSourceCallable().getLocation() }
 }
+
+DataFlowCallable getDataFlowCallable(Callable callable) { result.asSourceCallable() = callable }


aschackmull · 2026-10-15T10:51:14Z

  }

-  predicate clearsContent(Node n, ContentSet c) { none() } // TODO
+  predicate clearsContent(Node n, ContentSet c) { captureSsaClearsContent(n, c) }


Maybe leave a TODO comment in place for the regular store-into-field clearsContent.

aschackmull

There's plenty more to figure out for variable capture as already discussed, but that's not for this PR. So LGTM for merge!

github-advanced-security Bot found potential problems Oct 1, 2026

View reviewed changes

Comment thread unified/ql/lib/codeql/unified/internal/dataflow/DataFlowNode.qll Fixed

github-actions Bot added DataFlow Library Unified labels Oct 1, 2026

asgerf force-pushed the unified/captured-variables branch from 753f0d4 to d880379 Compare October 5, 2026 08:46

github-advanced-security Bot found potential problems Oct 5, 2026

View reviewed changes

Comment thread shared/ssa/codeql/ssa/Ssa.qll

Comment on lines +2159 to +2161

/**

* The given `read` reads multiple variables at once. `var` is bound to one of them.

*/

github-actions Bot added C# JS Java Python Ruby Rust Pull requests that update Rust code labels Oct 5, 2026

asgerf force-pushed the unified/captured-variables branch from d880379 to 9ce4785 Compare October 9, 2026 08:13

github-advanced-security Bot found potential problems Oct 9, 2026

View reviewed changes

Comment thread unified/ql/lib/codeql/unified/internal/dataflow/ConstructorPatterns.qll

Comment on lines +1 to +3

/**

* Provides data-flow modelling of constructor patterns / enum-case constructors.

*/

github-actions Bot removed C# JS Java Python Ruby Rust Pull requests that update Rust code labels Oct 9, 2026

asgerf force-pushed the unified/captured-variables branch 2 times, most recently from b9702e7 to b9910e3 Compare October 9, 2026 12:34

asgerf requested a balanced review from Copilot October 9, 2026 12:34

Copilot started reviewing on behalf of asgerf October 9, 2026 12:35

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

Comment thread unified/extractor/src/languages/swift/swift.rs Outdated

asgerf marked this pull request as ready for review October 9, 2026 13:42

asgerf requested review from a team as code owners October 9, 2026 13:43

asgerf added the no-change-note-required This PR does not need a change note label Oct 9, 2026

asgerf added 11 commits October 14, 2026 09:42

unified: Desugar capture declaration list

3321c32

fixup! dont add unary_expr

7b462e4

unified: Update test case

f30f6c9

unified: Fix scoping for capture declarations

6533df1

unified: Simplify with getParentIndex

4afec0c

Shared: Force join order in VariableCapture

d0a38cb

unified: Remove blank lines from expected file

7f9ebee

This appeared after an LLM-assisted rebase

unified: Update test after rebasing

f3afa24

Rebasing on the Optional<T> bypass makes this part of the test pass.

unified: Add tests with 'unowned' captures

d82fc83

unified: Auto-format self_access.swift

f50c74c

unified: Fix variable not being used in all disjuncts

1b94078

asgerf force-pushed the unified/captured-variables branch from 5bf8d21 to 1b94078 Compare October 14, 2026 08:12

paldepind reviewed Oct 14, 2026

View reviewed changes

aschackmull reviewed Oct 14, 2026

View reviewed changes

aschackmull reviewed Oct 15, 2026

View reviewed changes

asgerf added 4 commits October 15, 2026 11:08

unified: Avoid spurious recursion in toString and getLocation

1ab8fcd

unified: Add TODO about escape analysis

cd6386a

unified: Elaborate on CapturedParameter

31488c0

unified: Add TODO in ClosureExpr

4be4ff5

aschackmull reviewed Oct 15, 2026

View reviewed changes

asgerf added 3 commits October 15, 2026 12:57

unified: Rename get -> to

48e3e11

unified: Preserve TODO at clearsContent

10853e6

unified: Autoformat

f666d7e

aschackmull approved these changes Oct 15, 2026

View reviewed changes

asgerf merged commit 0c40f4d into github:main Oct 15, 2026
110 of 113 checks passed


		LocalVariableRefNode() { this = TLocalVariableRefNode(repr, var, kind) }

		predicate hasCfgNode(BasicBlock bb, int i) {

Conversation

asgerf commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Data flow node splitting

Capture declarations

Uh oh!

Uh oh!

asgerf commented Oct 9, 2026 • edited by codeql-ci-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

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!

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!

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

asgerf commented Oct 1, 2026 •

edited

Loading

asgerf commented Oct 9, 2026 •

edited by codeql-ci-app Bot

Loading