Unified: implement local scoping - #22120
Conversation
Also move into 'internal' and expose through a Public module
As documented in the test, 'guard if' statements are still not properly supported.
The original test was not valid Swift syntax
- Context is fully reset when stepping into an expr/stmt/body. - Binding modifier is rolled into outer_modifiers.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /** | ||
| * Declaration of a local or top-level variable. | ||
| */ |
| /** | ||
| * Declaration of a local or top-level function. | ||
| */ |
| // TODO: self | ||
| } | ||
|
|
||
| predicate accessCand(AstNode n, string name) { |
| */ | ||
| predicate relevantNode(AstNode node) { | ||
| // Match an ancestor node by location so its whole subtree is shown. | ||
| node.getParent*().getLocation().toString().matches("%test.swift@227:%") |
There was a problem hiding this comment.
Pull request overview
This PR introduces lexical (local) scoping support to the Unified libraries by adding a name-binding–based Variables library, restructuring Unified’s public surface to re-export internal Public modules via unified.qll, and updating the Swift Yeast translation to correctly distinguish binding patterns from expression-equality patterns while preventing modifier/context leakage into unrelated subtrees.
Changes:
- Add a Unified local-variable model (
Variables) backed bycodeql/namebinding, plus a developer debug query for the scope graph. - Update Swift extractor translation to use context-passing + context resets so patterns are translated correctly and binding modifiers don’t leak into initializers/bodies.
- Add/expand Swift corpus and Unified library-tests covering variable binding, shadowing, and guard/conditional scoping behaviors.
Show a summary per file
| File | Description |
|---|---|
| unified/ql/test/library-tests/variables/variables.ql | Adds an InlineExpectationsTest query to validate VariableAccess→Variable resolution via inline comments. |
| unified/ql/test/library-tests/variables/variables.expected | Inline-expectations .expected placeholder for the new test (expected to be empty). |
| unified/ql/test/library-tests/variables/test.swift | Adds a comprehensive Swift fixture exercising shadowing and scoping scenarios. |
| unified/ql/lib/unified.qll | Switches Unified’s public imports to re-export internal Public modules (Variables/Ast extras). |
| unified/ql/lib/qlpack.yml | Adds dependency on codeql/namebinding to support local name binding. |
| unified/ql/lib/codeql/unified/internal/Variables.qll | Implements local variable declarations/accesses and scope lookup wiring for Unified. |
| unified/ql/lib/codeql/unified/internal/dev/debugScopeGraph.ql | Adds a dev-only graph query to render/debug the local scope graph. |
| unified/ql/lib/codeql/unified/internal/AstExtra.qll | Adds non-generated AST helper classes (including Comment) under an internal module. |
| unified/ql/lib/codeql/unified/Comments.qll | Removes the old comments helper (moved under internal AstExtra). |
| unified/extractor/tests/corpus/swift/variables/tuple-destructuring-binding.output | Updates expected AST output for tuple destructuring bindings. |
| unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.swift | Adds corpus fixture ensuring binding modifiers don’t leak into initializer translation. |
| unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.output | Adds expected output for the initializer leakage fixture. |
| unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.swift | Adds corpus fixture ensuring binding modifiers don’t leak into accessor bodies. |
| unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.output | Adds expected output for the accessor-body leakage fixture. |
| unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.swift | Adds corpus fixture ensuring binding modifiers don’t leak to sibling statements. |
| unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.output | Adds expected output for the sibling leakage fixture. |
| unified/extractor/src/languages/swift/swift.rs | Updates Swift Yeast rules to publish binding context, translate/reset subtrees, and pattern-translate identifiers appropriately. |
| shared/yeast/src/build.rs | Adds translate_reset helper to translate captures under a fresh user context. |
| shared/namebinding/codeql/namebinding/LocalNameBinding.qll | Adjusts the namebinding interface and adds a debug scope graph helper module. |
| ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll | Updates Ruby implementation to match the adjusted namebinding signature. |
Review details
- Files reviewed: 19/20 changed files
- Comments generated: 7
- Review effort level: Low
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
QL changes LGTM, I mostly have some minor comments. I have approved in case we want to merge in order to unblock other work; my comments can be addressed follow-up if needed.
| private class LogicalAndRoot extends LogicalAndExpr { | ||
| LogicalAndRoot() { not this = any(LogicalAndExpr e).getAnOperand() } | ||
|
|
||
| private Expr getDescendent(string path) { |
| ) | ||
| } | ||
|
|
||
| Expr getNthLeaf(int n) { |
There was a problem hiding this comment.
We do something similar in Rust for let chains; perhaps we should consider making this transformation inside the shared library.
| predicate accessCand(AstNode n, string name) { | ||
| n.(NameExpr).getIdentifier().getValue() = name | ||
| or | ||
| n.(NamePattern).getIdentifier().getValue() = name |
There was a problem hiding this comment.
I don't understand why this case is needed; I realize that removing it removes the results on lines 347 and 348 in test.swift, but those look like declarations and not accesses to me.
There was a problem hiding this comment.
@hvitved There is currently no distinction between declarations and accesses; VariableAccess contains both.
The test suite hides this however by suppressing the need for annotating the result of getDefiningNode (here), but this suppression does nothing when the defining node is an OrPattern. So in that specific case the declaring identifiers must be annotated as accesses.
We could consider renaming VariableAccess to VariableReference to be more in line with naming conventions from other languages.
| n.(Identifier).getValue() = name | ||
| } | ||
|
|
||
| predicate lookupStartsAt(AstNode n, AstNode scope) { none() } |
| /** | ||
| * A local variable. | ||
| */ | ||
| class Variable extends LocalNameBindingOutput::Local { |
There was a problem hiding this comment.
We may want to exclude local functions from this class at some point (same for VariableAccess).
There was a problem hiding this comment.
I was expecting to just treat local functions as variables holding a function value.
| // Switch with multiple cases | ||
| func t11(value: Int) { // name=value1 | ||
| switch value { // $ access=value1 | ||
| case let x where x > 0: // name=x1 |
There was a problem hiding this comment.
I would also expect an access=x1 here.
There was a problem hiding this comment.
Good catch! This seems to be bug, will try to fix.
| let x = 2 // name=x2 | ||
| print(x) // $ access=x2 | ||
| } | ||
| inner() // $ access=inner1 |
There was a problem hiding this comment.
Don't know if it is valid in Swift, but it would be nice to have:
let inner = 2 // name=inner2
print(inner) // $access=inner2after this line.
| } | ||
| } | ||
|
|
||
| // Switch with same variable name in different cases |
There was a problem hiding this comment.
comment does not match test case
| /** Holds if `node` should be included in the debug tree. */ | ||
| private signature predicate relevantNodeSig(AstNode node); | ||
|
|
||
| module DebugScopeGraph<relevantNodeSig/1 relevantNode> { |
| definingNode = getEnclosingOrPattern(pattern) | ||
| or | ||
| not exists(getEnclosingOrPattern(pattern)) and | ||
| definingNode = pattern |
There was a problem hiding this comment.
Again, something similar is done for Rust, so there may be potential for sharing.
tausbn
left a comment
tausbn
left a comment
There was a problem hiding this comment.
I think the changes here look good. My only concern is the new translate_reset mechanism, which is a bit too "all or nothing" for my taste. I've come up with a slightly different API that I think is equally good, but a bit more flexible. In the interest of moving things along, I'll approve and merge this PR, and then make the necessary improvements later on.
This adds local scoping and does a few other things in order to get that working:
codeql.unified.internaland imports theirPublicmodules intounified.qll. Makes it easy to share internal code between the libraries.name_patternorexpr_equality_patternwrapping aname_expr. In the future we might want to move this logic into QL but I wanted to make sure that Yeast can handle this kind of problem if we need it to. The commit also fixes a bunch of potential context-leaking issues where modifiers would leak into expr/stmt subtrees.guardstatements could not be natively handled by the shared library, but it turns out can handle it by flattening their children into the enclosingBlock.