Skip to content

Shared: Local name resolution library#21873

Open
hvitved wants to merge 8 commits into
github:mainfrom
hvitved:local-name-resolution
Open

Shared: Local name resolution library#21873
hvitved wants to merge 8 commits into
github:mainfrom
hvitved:local-name-resolution

Conversation

@hvitved
Copy link
Copy Markdown
Contributor

@hvitved hvitved commented May 20, 2026

This PR adds a new shared library for doing local name binding, and applies it to Ruby and Rust.

The interface to the library is inspired by a similar unreleased library created by @asgerf, but unlike that library, this library supports sibling-based shadowing as known from Rust:

let x = 1;
let x = x + 1;     // shadows `x` above, but `x` in the RHS still refers to the above
println!("{}", x); // refers to the shadowing `x`

The implementation handles sibling shadowing much simpler than the current Rust implementation (which also has a bug, see one of the added tests), by treating shadowing sibling declarations as defining a new scope, and then letting all subsequent statements be children of that scope. That is, when looking up names by going up the AST, we rewrite the AST above to (eliding leaf nodes):

      let x = 1;
       /      \
     x + 1   let x = x + 1
                   |
            println!("{}", x);

Note how the RHS is moved away from let x = x + 1 to ensure that it resolves to the preceding declaration.

DCA for Rust and Ruby are good; no result changes but a minor speedup (for Rust).

@github-actions github-actions Bot added the Rust Pull requests that update Rust code label May 20, 2026
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Fixed
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Fixed
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Fixed
Comment thread rust/ql/lib/codeql/rust/elements/internal/LocalNameBinding.qll Fixed
Comment thread shared/namebinding/codeql/namebinding/LocalNameBinding.qll Fixed
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Fixed
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Fixed
Comment thread rust/ql/lib/codeql/rust/elements/internal/LocalNameBinding.qll Fixed
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Fixed
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Fixed
@hvitved hvitved force-pushed the local-name-resolution branch from 46e6643 to 78a5319 Compare May 21, 2026 12:58
Comment thread shared/namebinding/codeql/namebinding/LocalNameBinding.qll Fixed
Comment thread shared/namebinding/codeql/namebinding/LocalNameBinding.qll Fixed

/** Gets the name of this variable as a string. */
string getText() { result = text }
predicate accessCand(AstNode n, string name) {
Comment thread shared/namebinding/codeql/namebinding/LocalNameBinding.qll Fixed
Comment thread shared/namebinding/codeql/namebinding/LocalNameBinding.qll Fixed
@hvitved hvitved force-pushed the local-name-resolution branch from 0de851d to 35ff093 Compare May 21, 2026 14:46
Comment thread shared/namebinding/codeql/namebinding/LocalNameBinding.qll Fixed
@hvitved hvitved force-pushed the local-name-resolution branch from 35ff093 to cad848a Compare May 21, 2026 14:49
* We also move any `else` branch _before_ the condition to ensure that shadowing declarations
* inside the condition are not in scope.
*/
private AstNode getChildAdj(AstNode parent, int index) {
@hvitved hvitved force-pushed the local-name-resolution branch 3 times, most recently from 9ce2e74 to baef7eb Compare May 22, 2026 08:01

class Ranked = AstNode;

int getRank(C parent, Ranked child) {
@hvitved hvitved force-pushed the local-name-resolution branch 2 times, most recently from 59b8380 to 0f3ee22 Compare May 22, 2026 13:24
@hvitved
Copy link
Copy Markdown
Contributor Author

hvitved commented May 22, 2026

@hvitved
Copy link
Copy Markdown
Contributor Author

hvitved commented May 22, 2026

Rerun has been triggered: 1 restarted 🚀

@hvitved hvitved force-pushed the local-name-resolution branch from 0f3ee22 to b5586e5 Compare May 26, 2026 13:06
@github-actions github-actions Bot added the Ruby label May 26, 2026
)
}

predicate accessCand(AstNode n, string name) {
@hvitved hvitved force-pushed the local-name-resolution branch 4 times, most recently from da949b1 to 392fab1 Compare May 28, 2026 08:25
@hvitved hvitved force-pushed the local-name-resolution branch from 392fab1 to 128dd5d Compare May 28, 2026 08:50
/**
* Holds if `n` is a node that may access a local named `name`.
*/
predicate accessCand(AstNode n, string name);
@hvitved hvitved changed the title Rust: Move local name resolution logic into shared library Shared: Local name resolution library May 28, 2026
@hvitved
Copy link
Copy Markdown
Contributor Author

hvitved commented May 28, 2026

Rerun has been triggered: 2 restarted 🚀

@hvitved hvitved force-pushed the local-name-resolution branch 3 times, most recently from cba5d4e to aa128c1 Compare May 28, 2026 18:27
@hvitved hvitved force-pushed the local-name-resolution branch from aa128c1 to 0937133 Compare May 29, 2026 07:06
@hvitved
Copy link
Copy Markdown
Contributor Author

hvitved commented May 29, 2026

Rerun has been triggered: 2 restarted 🚀

@hvitved hvitved marked this pull request as ready for review May 29, 2026 11:19
@hvitved hvitved requested review from a team as code owners May 29, 2026 11:19
Copilot AI review requested due to automatic review settings May 29, 2026 11:19
@hvitved hvitved requested a review from a team as a code owner May 29, 2026 11:19
@hvitved hvitved added the no-change-note-required This PR does not need a change note label May 29, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 a shared local name-binding library and wires it into Ruby and Rust variable resolution, replacing language-specific scoping logic with a common sibling-shadowing-aware implementation.

Changes:

  • Adds codeql/namebinding shared qlpack and LocalNameBinding.qll.
  • Refactors Rust and Ruby local variable/access resolution to use the shared library.
  • Expands Rust and Ruby variable-scope tests for shadowing, let chains, exception variables, and block parameters.
Show a summary per file
File Description
shared/namebinding/qlpack.yml Defines the new shared namebinding library pack.
shared/namebinding/codeql/namebinding/LocalNameBinding.qll Adds the shared local name-binding implementation.
rust/ql/lib/qlpack.yml Adds the shared namebinding dependency.
rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Replaces Rust-specific variable binding with shared namebinding integration.
rust/ql/lib/codeql/rust/elements/internal/ParamBaseImpl.qll Adds callable lookup support for parameters.
rust/ql/.gitattributes Marks ParamBaseImpl.qll as hand-modified rather than generated.
rust/ql/.generated.list Removes ParamBaseImpl.qll from generated tracking.
rust/ql/test/library-tests/variables/main.rs Adds Rust shadowing and conditional let-chain test cases.
rust/ql/test/library-tests/variables/variables.expected Updates Rust variable test expectations.
ruby/ql/lib/qlpack.yml Adds the shared namebinding dependency.
ruby/ql/lib/codeql/ruby/ast/Parameter.qll Updates parameter-to-variable lookup for the new local variable representation.
ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll Refactors Ruby local variable/access resolution onto shared namebinding.
ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll Updates synthetic self/hash-value access handling.
ruby/ql/lib/codeql/ruby/ast/internal/Scope.qll Exposes parent/scope helpers needed by namebinding.
ruby/ql/lib/codeql/ruby/ast/internal/Parameter.qll Updates internal parameter variable lookup.
ruby/ql/lib/codeql/ruby/ast/internal/AST.qll Updates local variable access construction.
ruby/ql/test/library-tests/variables/scopes.rb Adds Ruby exception-variable and parameter-shadowing tests.
ruby/ql/test/library-tests/variables/varscopes.expected Updates Ruby scope expectations.
ruby/ql/test/library-tests/variables/variable.expected Updates Ruby variable expectations.
ruby/ql/test/library-tests/variables/varaccess.expected Updates Ruby variable access expectations.
ruby/ql/test/library-tests/variables/ssa.expected Updates Ruby SSA expectations.
ruby/ql/test/library-tests/variables/parameter.expected Updates Ruby parameter expectations.

Copilot's findings

  • Files reviewed: 22/24 changed files
  • Comments generated: 4

Comment thread shared/namebinding/codeql/namebinding/LocalNameBinding.qll Outdated
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Outdated
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Outdated
Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Outdated
Copy link
Copy Markdown
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Looks great! I've reviewed everything, though for Rust maybe get a second reviewer.

The only potential blocker I'd say actually comes from a QL4QL alert about the f field in NestedFunctionAccess being unused, which seems legit.

* }
* ```
*/
class Conditional extends AstNode {
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.

Perhaps mention how to represent if let Some(x) = opt else { ... } where the effective then-branch is to fall through?

This type of statement is sort-of hinted at in the SiblingShadowingDecl class below, but the feature is orthogonal to sibling shadowing. Swift has the equivalent of if let ... else ... but does not have sibling shadowing.

or
isTopScope(this)
or
lookupStartsAt(_, this)
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'm not sure the last case is needed here? The corresponding parameter of lookupStartsAt is named scope but it's just the AST node where upward traversal begins.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Your are right.

Comment thread rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Fixed
hvitved and others added 2 commits May 29, 2026 14:24
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Ruby Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants