Skip to content

Unified: implement local scoping - #22120

Merged
tausbn merged 13 commits into
github:mainfrom
asgerf:unified/local-scoping
Jul 8, 2026
Merged

Unified: implement local scoping#22120
tausbn merged 13 commits into
github:mainfrom
asgerf:unified/local-scoping

Conversation

@asgerf

@asgerfasgerf commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

This adds local scoping and does a few other things in order to get that working:

  • Moves libraries into codeql.unified.internal and imports their Public modules into unified.qll. Makes it easy to share internal code between the libraries.
  • The last commit sets out to use Yeast context-passing to correctly translate identifers in patterns to either name_pattern or expr_equality_pattern wrapping a name_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.

guard statements could not be natively handled by the shared library, but it turns out can handle it by flattening their children into the enclosing Block.

@asgerfasgerf added the no-change-note-required This PR does not need a change note label Jul 3, 2026
Comment on lines +17 to +19
/**
* Declaration of a local or top-level variable.
*/
Comment on lines +29 to +31
/**
* 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:%")
@asgerf
asgerf requested a review from CopilotJuly 3, 2026 12:18
@asgerf
asgerf marked this pull request as ready for review July 3, 2026 12:19
@asgerf
asgerf requested review from a team as code ownersJuly 3, 2026 12:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 by codeql/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
FileDescription
unified/ql/test/library-tests/variables/variables.qlAdds an InlineExpectationsTest query to validate VariableAccessVariable resolution via inline comments.
unified/ql/test/library-tests/variables/variables.expectedInline-expectations .expected placeholder for the new test (expected to be empty).
unified/ql/test/library-tests/variables/test.swiftAdds a comprehensive Swift fixture exercising shadowing and scoping scenarios.
unified/ql/lib/unified.qllSwitches Unified’s public imports to re-export internal Public modules (Variables/Ast extras).
unified/ql/lib/qlpack.ymlAdds dependency on codeql/namebinding to support local name binding.
unified/ql/lib/codeql/unified/internal/Variables.qllImplements local variable declarations/accesses and scope lookup wiring for Unified.
unified/ql/lib/codeql/unified/internal/dev/debugScopeGraph.qlAdds a dev-only graph query to render/debug the local scope graph.
unified/ql/lib/codeql/unified/internal/AstExtra.qllAdds non-generated AST helper classes (including Comment) under an internal module.
unified/ql/lib/codeql/unified/Comments.qllRemoves the old comments helper (moved under internal AstExtra).
unified/extractor/tests/corpus/swift/variables/tuple-destructuring-binding.outputUpdates expected AST output for tuple destructuring bindings.
unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.swiftAdds corpus fixture ensuring binding modifiers don’t leak into initializer translation.
unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.outputAdds expected output for the initializer leakage fixture.
unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.swiftAdds 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.outputAdds expected output for the accessor-body leakage fixture.
unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.swiftAdds 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.outputAdds expected output for the sibling leakage fixture.
unified/extractor/src/languages/swift/swift.rsUpdates Swift Yeast rules to publish binding context, translate/reset subtrees, and pattern-translate identifiers appropriately.
shared/yeast/src/build.rsAdds translate_reset helper to translate captures under a fresh user context.
shared/namebinding/codeql/namebinding/LocalNameBinding.qllAdjusts the namebinding interface and adds a debug scope graph helper module.
ruby/ql/lib/codeql/ruby/ast/internal/Variable.qllUpdates Ruby implementation to match the adjusted namebinding signature.

Review details

  • Files reviewed: 19/20 changed files
  • Comments generated: 7
  • Review effort level: Low

Comment threadshared/yeast/src/build.rs Outdated
Comment threadunified/ql/lib/codeql/unified/internal/Variables.qll Outdated
Comment threadunified/ql/lib/codeql/unified/internal/AstExtra.qll Outdated
Comment threadunified/ql/test/library-tests/variables/test.swift Outdated
Comment threadunified/ql/test/library-tests/variables/test.swift Outdated
Comment threadunified/ql/test/library-tests/variables/test.swift Outdated
Comment threadunified/ql/test/library-tests/variables/test.swift Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@hvitvedhvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment threadunified/ql/lib/codeql/unified/internal/Variables.qll Outdated
)
}

Expr getNthLeaf(int n) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do something similar in Rust for let chains; perhaps we should consider making this transformation inside the shared library.

Comment threadunified/ql/lib/codeql/unified/internal/Variables.qll
Comment threadunified/ql/lib/codeql/unified/internal/Variables.qll
Comment threadunified/ql/lib/codeql/unified/internal/Variables.qll
Comment threadunified/ql/test/library-tests/variables/test.swift
Comment threadunified/ql/test/library-tests/variables/test.swift
Comment threadunified/ql/test/library-tests/variables/test.swift
Comment threadshared/namebinding/codeql/namebinding/LocalNameBinding.qll
Comment on lines +244 to +247
definingNode = getEnclosingOrPattern(pattern)
or
not exists(getEnclosingOrPattern(pattern)) and
definingNode = pattern

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, something similar is done for Rust, so there may be potential for sharing.

@tausbntausbn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tausbn
tausbn merged commit 3c3f740 into github:mainJul 8, 2026
137 of 142 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-requiredThis PR does not need a change noteRuby

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@asgerf@tausbn@hvitved@github-advanced-security