Unified: Add data flow support for getters and setters by asgerf · Pull Request #22896 · github/codeql

asgerf · 2026-10-26T12:29:14Z

Adds support calls to getters and setters, called "computed properties" in Swift.

Some complications for Swift:

Getters and setters can be declared locally. A local getter/setter can capture variables.
Setters have an implicit parameter called newValue. We insert this parameter in the AST mapping.
Writebacks: If p refers to a value-typed computed property p.x = y results in a call to p's setter.
Writebacks also occur in foo(&p) where p is passed as an inout argument. Full support for inout parameters is out of scope for this PR, however, so this is only half-supported right now.
Writebacks also occur for calls like p.mutate() where mutate refers to a mutating func. This is effectively the same as seeing the receiver as an inout argument.

Also does some clean up in CaptureSsa

Commit-by-commit review recommended.

Copilot

🟡 Changes recommended

Unresolved accessor resolution, state propagation, and capture-ordering gaps can cause valid data flows to be missed.

5 open issues

What changed in this PR

Adds getter and setter modeling to Unified’s data-flow analysis, with Swift extraction support and regression fixtures.

Changes:

Extracts accessor return types, parameters, and property modifiers.
Models accessor calls, receiver flow, and captured-variable synchronization.
Adds local, instance, and static accessor tests and updates expected results.

File	Description
`unified/ql/test/library-tests/dataflow/test.expected`	Updates accessor and closure-flow expectations.
`unified/ql/test/library-tests/dataflow/accessors_static.swift`	Adds static accessor cases.
`unified/ql/test/library-tests/dataflow/accessors_local.swift`	Adds local accessor and capture cases.
`unified/ql/test/library-tests/dataflow/accessors_instance.swift`	Adds instance accessor and receiver-flow cases.
`unified/ql/lib/unified.dbscheme`	Renames the accessor return-type relation.
`unified/ql/lib/codeql/unified/internal/typeinference/Type.qll`	Exposes the class represented by a type.
`unified/ql/lib/codeql/unified/internal/StaticNameBinding.qll`	Unifies qualified and unqualified member access.
`unified/ql/lib/codeql/unified/internal/NameBindingPlugin.qll`	Recognizes instance accessors.
`unified/ql/lib/codeql/unified/internal/LocalNameBinding.qll`	Identifies computed local variables.
`unified/ql/lib/codeql/unified/internal/ExprPositions.qll`	Uses the renamed accessor return-type API.
`unified/ql/lib/codeql/unified/internal/dataflow/Step.qll`	Adds call-graph-only flow steps.
`unified/ql/lib/codeql/unified/internal/dataflow/LocalSsa.qll`	Excludes computed locals from stored-variable tracking.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowNode.qll`	Extends receiver APIs and accessor control-flow positions.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowInstantiation.qll`	Connects getter results and setter arguments.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowGraph.qll`	Adds accessor receiver and callable edges.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowCall.qll`	Represents implicit getter and setter calls.
`unified/ql/lib/codeql/unified/internal/dataflow/CaptureSsa.qll`	Revises closure alias and escape tracking.
`unified/ql/lib/codeql/unified/internal/dataflow/CallGraph.qll`	Resolves accessor call targets.
`unified/ql/lib/codeql/unified/internal/Ast.qll`	Updates generated accessor return-type APIs.
`unified/ql/consistency-queries/DataFlowConsistency.ql`	Exempts setter inputs from post-update checks.
`unified/extractor/tests/corpus/swift/variables/property-with-willset-and-didset-observers.output`	Records synthesized observer parameters.
`unified/extractor/tests/corpus/swift/types/protocol-with-read-only-and-read-write-property-requirements.output`	Updates protocol accessor expectations.
`unified/extractor/tests/corpus/swift/types/property-with-getter-and-setter.output`	Records getter return types and setter parameters.
`unified/extractor/tests/corpus/swift/types/computed-property.output`	Updates computed-property return-type output.
`unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.output`	Updates the accessor return-type field.
`unified/extractor/src/languages/swift/swift.rs`	Extracts accessor parameters, return types, and modifiers.
`unified/extractor/ast_types.yml`	Defines the accessor return-type field.

_{Review effort: Balanced}

Tests are split into three categories: local, static, and instance. They follow mostly the same structure.

Accessors previously had a 'type' field. Now, it has a return_type instead, which is only set for getters. For set, didSet, and willSet, we insert a synthetic newValue/oldValue parameter, which also carries the type previously stored in accessor_declaration.type.

Modifiers like 'static' did not get preserved when extracting accessors, making most of the tests fail for static getters/setters. All those tests now pass.

…dMemberAccess

This does the minimal change needed to get things working, but it's not pretty. A later commit will refactor this to have less ad-hoc logic here.

Now that all data flow nodes are in Expr, we get more noise if we don't restrict this

This was already handled in the AST mapping, but no QL test was added initially

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

If `p` is a getter/setter pair for a value type, then `p.f = x` ends with a call to p's setter with the updated value (called "writebacks" in Swift).

Writebacks are a language-specific thing so let's add a hook for it

Copilot

🟡 Changes recommended

Writeback eligibility can introduce spurious setter flows, and closure alias traversal can miss capture synchronization through casts and force-unwraps.

3 open issues

4 resolved since last review

_{Review effort: Balanced}

isMutatedByCallOrAssign was a synonym for hasPostUpdateAfterCallOrAssign

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

UnqualifiedMemberAccess is only populated when static name binding positively identifies the resolution target, so either the first case catches it, and if unresolved, the second case will miss it anyway because it won't be a MemberAccess instance.

hvitved

Looks plausible to me (lots of moving parts).

aschackmull · 2026-10-29T09:33:33Z

+    isInWriteBackPosition(expr)
+    // TODO: Check that 'expr' has a value type. Reference types don't cause writebacks.
+  }
+}
+
+/**
+ * Holds if `expr` is target of an assignment or 'inout' argument,
+ * or the base of such an expression, e.g. `x` in `x.f1.f2 = foo()`
+ */
+private predicate isInWriteBackPosition(Expr expr) {
+  isInAssignmentContext(expr, _)
+  or
+  // Arguments to an 'inout' parameter must be prefixed with '&'
+  exists(UnaryExpr unary |
+    unary.getOperator().(PrefixOperator).getValue() = "&" and
+    unary.getOperand() = expr
+    // TODO: support custom operators with an 'inout' parameter, which lack the '&' indicator
+  )
+  or
+  // The receiver of a call to a 'mutating' function counts as an 'inout' argument
+  exists(CallExpr call, Callable target |
+    target = T::resolveCallTarget(call) and
+    target.hasModifier("mutating") and
+    expr = call.getCallee().(MemberAccessExpr).getBase()
+  )
+  or
+  exists(MemberAccessExpr member |
+    isInWriteBackPosition(member) and
+    expr = member.getBase()
+  )


Was this addition meant for the Swift plugin?

Indeed it was. Will move it 👍

aschackmull

One clarifying question, otherwise LGTM.

This was accidentally put outside the plugin

asgerf requested a balanced review from Copilot October 26, 2026 12:36

Copilot started reviewing on behalf of asgerf October 26, 2026 12:36

View session

github-actions Bot added the Unified label Oct 26, 2026

Copilot AI reviewed Oct 26, 2026

View reviewed changes

asgerf and others added 26 commits October 27, 2026 14:15

unified: Add tests with accessors

6a46da4

Tests are split into three categories: local, static, and instance. They follow mostly the same structure.

unified: Allow accessors to be seen as instance members

6270fad

unified: Add DataFlowCalls for getter and setter calls

3fcfb68

unified: Resolve calls statically

61a102c

unified: Add out-node from getter calls

f1f4d84

unified: Pass the incoming value into setter calls

5dec1dc

unified: Ensure 'static' modifier is preserved for accessors

c7c80a7

Modifiers like 'static' did not get preserved when extracting accessors, making most of the tests fail for static getters/setters. All those tests now pass.

unified: Add MemberAccess as union of MemberAccessExpr and Unqualifie…

32979b2

…dMemberAccess

unified: Also resolve accessors on instances

364e982

unified: Make accessors introduce LocalVariables too

9da580f

unified: Handle capturing local getters/setters

34de910

This does the minimal change needed to get things working, but it's not pretty. A later commit will refactor this to have less ad-hoc logic here.

unified: Propagate receiver argument into accessor calls

5c9942f

unified: Allow self-return via receiver parameter

b348803

unified: Add more capture tests

edc465e

unified: Add test with chain of accessors

6444d0b

unified: Split up hasControlFlowPosition()

4bd9473

unified: Associate CFG nodes with implicit getter/setter arguments

a808fde

unified: Use hasControlFlowPosition in CaptureSsa

3f7735f

unified: Move ad-hoc aliasing logic into a step

d0d1363

unified: Add basic escape

e3883da

Now that all data flow nodes are in Expr, we get more noise if we don't restrict this

unified: Update nodes/edges in test output

09fb8f1

unified: Tolerate missing post-update for RHS

b084467

unified: Add test for explicitly-named setter parameter

1cc99d5

This was already handled in the AST mapping, but no QL test was added initially

Update isEscape predicate to include incoming values

612c241

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

unified: Add test for receiver updates from getter reads

ac8adee

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

asgerf added 5 commits October 27, 2026 14:18

unified: Simple fix for getter calls missing post update node

6741896

unified: More precise CFG node for implicit setter arguments

4a510f7

unified: Support setter calls from writebacks

ed70ef4

If `p` is a getter/setter pair for a value type, then `p.f = x` ends with a call to p's setter with the updated value (called "writebacks" in Swift).

unified: Add a hook for writebacks

fcf39ec

Writebacks are a language-specific thing so let's add a hook for it

unified: Test that writebacks after a mutating call works

3769c46

asgerf force-pushed the unified/accessors branch from 235291a to 3769c46 Compare October 27, 2026 13:23

asgerf requested a balanced review from Copilot October 27, 2026 13:23

Copilot started reviewing on behalf of asgerf October 27, 2026 13:24

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

Comment thread unified/ql/lib/codeql/unified/internal/dataflow/CaptureSsa.qll Resolved

Comment thread unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPluginSwift.qll Resolved

asgerf added 2 commits October 28, 2026 11:20

unified: Be more precise about writebacks

0d1b4b9

unified: Remove superfluous forwarding-predicate

405e322

isMutatedByCallOrAssign was a synonym for hasPostUpdateAfterCallOrAssign

asgerf force-pushed the unified/accessors branch from 29a2bc5 to 405e322 Compare October 28, 2026 10:21

Enhance step function to include valueMustFlow check

bd2d5a4

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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

asgerf added 2 commits October 28, 2026 11:45

unified: Fix bad join

88c4786

asgerf marked this pull request as ready for review October 28, 2026 13:04

asgerf requested review from a team as code owners October 28, 2026 13:04

hvitved reviewed Oct 28, 2026

View reviewed changes

Comment thread unified/ql/test/library-tests/dataflow/test.expected Resolved

Comment thread unified/ql/lib/codeql/unified/internal/LocalNameBinding.qll Resolved

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

unified: Hide generated nodes

8ebaab9

hvitved previously approved these changes Oct 29, 2026

View reviewed changes

aschackmull reviewed Oct 29, 2026

View reviewed changes

unified: Move mayWriteBackViaSetterCall into swift plugin

1bb7125

This was accidentally put outside the plugin

asgerf dismissed hvitved’s stale review via 1bb7125 October 29, 2026 09:46

aschackmull approved these changes Oct 29, 2026

View reviewed changes

asgerf merged commit 824bba2 into github:main Oct 29, 2026
16 checks passed

Navigation Menu

Repository navigation

Unified: Add data flow support for getters and setters - #22896

Unified: Add data flow support for getters and setters#22896
asgerf merged 38 commits into
github:mainfrom
asgerf:unified/accessors

asgerf commented Oct 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

hvitved left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aschackmull Oct 29, 2026

Uh oh!

asgerf Oct 29, 2026

Uh oh!

aschackmull left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

asgerf commented Oct 26, 2026 • edited 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

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aschackmull Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

asgerf Oct 29, 2026

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asgerf commented Oct 26, 2026 •

edited

Loading