Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 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.
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
asgerf
force-pushed
the
unified/accessors
branch
from
October 27, 2026 13:23
235291a to
3769c46
Compare
isMutatedByCallOrAssign was a synonym for hasPostUpdateAfterCallOrAssign
asgerf
force-pushed
the
unified/accessors
branch
from
October 28, 2026 10:21
29a2bc5 to
405e322
Compare
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.
asgerf
marked this pull request as ready for review
October 28, 2026 13:04
hvitved
reviewed
Oct 28, 2026
hvitved
left a comment
hvitved
left a comment
Contributor
There was a problem hiding this comment.
Looks plausible to me (lots of moving parts).
hvitved
approved these changes
Oct 29, 2026
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.
Adds support calls to getters and setters, called "computed properties" in Swift.
Some complications for Swift:
newValue. We insert this parameter in the AST mapping.prefers to a value-typed computed propertyp.x = yresults in a call top's setter.foo(&p)wherepis passed as aninoutargument. Full support forinoutparameters is out of scope for this PR, however, so this is only half-supported right now.p.mutate()wheremutaterefers to amutating func. This is effectively the same as seeing the receiver as aninoutargument.Also does some clean up in
CaptureSsaCommit-by-commit review recommended.