Rust: Take trait visibility into account when resolving paths and methods #20321

paldepind · 2025-08-30T11:30:01Z

In Rust a trait method can only be invoked if the trait is visible/in scope, but currently all traits are treated as globally visible.

With this PR, for a path Type::method we only consider method methods on traits that are visible. Similarly, for method calls foo.bar(...) we only consider bar methods on traits that are visible.

This PR is motived by a type explosion in Deno that is magnified by #20133. Accounting for visible traits in #20133 in the same way as in this PR should also reduce false positives for blanket implementations.

DCA shows:

Insignificant change to analysis time. I struggled a bit with getting acceptable performance, but it seem ok now.
Path resolution inconsistencies are down by around 25%. I think this is explained by a reduction in multipleCallTargets violations.
Nodes with type at length limit is reduced by a small amount.
We loose call targets for 0.381% of all calls. I've inspected a bunch of lost targets, and they looked like false targets. Looking at the data, e.g., for databend we loose targets for 1020 calls and path resolution inconsistencies is down 5191. One plausible interpretation is that there where 5191 calls where we found the correct target plus a wrong one and 1020 calls where we only found a wrong one.

…olution

rust/ql/lib/codeql/rust/internal/PathResolution.qll

Copilot

Pull Request Overview

This PR improves Rust trait scoping by ensuring that trait methods can only be invoked when the trait is visible or in scope. Previously, all traits were treated as globally visible, which led to incorrect path resolution and false positives.

Key changes include:

Added trait visibility checking for method calls and qualified paths
Implemented a TraitIsVisible module to determine trait scope
Added support for underscore imports (use trait as _) which make traits visible but unnameable

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
rust/ql/lib/codeql/rust/internal/TypeInference.qll	Updated method resolution to check trait visibility for method calls
rust/ql/lib/codeql/rust/internal/PathResolution.qll	Added trait visibility infrastructure and path resolution improvements
rust/ql/test/library-tests/type-inference/main.rs	Added test cases for trait visibility scenarios
rust/ql/test/library-tests/path-resolution/main.rs	Added comprehensive trait visibility test module
*.expected files	Updated expected test results reflecting improved path resolution

Copilot · 2025-09-02T07:03:56Z

rust/ql/lib/codeql/rust/internal/PathResolution.qll

    exists(UseItemNode use |
      use = this.getASuccessor(_, _) and
-      result = use.(ItemNode).getASuccessor(name, kind)
+      result = use.getASuccessor(name, kind)


The removed explicit cast use.(ItemNode) was unnecessary since use is already declared as UseItemNode which extends ItemNode. This change improves code readability by removing redundant casting.

Copilot · 2025-09-02T07:03:57Z

rust/ql/lib/codeql/rust/internal/PathResolution.qll

-      kind.isExternalOrBoth() and
-      result instanceof AssocItemNode
-    )
+    typeImplEdge(this, _, name, kind, result)


Refactoring the complex inline logic into the typeImplEdge predicate improves code modularity and readability. This makes the logic reusable and easier to maintain.

Copilot · 2025-09-02T07:03:57Z

rust/ql/lib/codeql/rust/internal/PathResolution.qll

+          // When the rename doesn't have a name it's an underscore import. This
+          // makes the imported item visible but unnameable. We represent this
+          // by using the name `_` which can never occur in a path.  See also:
+          // https://doc.rust-lang.org/reference/items/use-declarations.html#r-items.use.as-underscore
+          not rename.hasName() and
+          name = "_"


Excellent documentation explaining the underscore import feature. The comment clearly explains why _ is used as a special name and includes a reference to the official Rust documentation.

Copilot · 2025-09-02T07:03:57Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

 pragma[nomagic]
 private predicate methodCandidateTrait(Type type, Trait trait, string name, int arity, Impl impl) {
-  trait = resolvePath(impl.(ImplItemNode).getTraitPath()) and
+  trait = impl.(ImplItemNode).resolveTraitTy() and


The change from resolvePath(impl.(ImplItemNode).getTraitPath()) to impl.(ImplItemNode).resolveTraitTy() suggests using a more direct method for trait resolution, which likely improves performance and code clarity.

paldepind · 2025-09-02T07:06:12Z

rust/ql/lib/codeql/rust/internal/PathResolution.qll

+ */
+module TraitIsVisible<relevantTraitVisibleSig/2 relevantTraitVisible> {
+  /** Holds if the trait might be looked up in `encl`. */
+  private predicate traitLookup(ItemNode encl, Element element, Trait trait) {


The implementation of this predicate is inspired by unqualifiedPathLookup. In my original attempt I used getADescendant*(), but that seemed to be part of the performance issues in that PR.

paldepind · 2025-09-02T07:08:47Z

rust/ql/lib/codeql/rust/internal/PathResolution.qll

+          // https://doc.rust-lang.org/reference/items/use-declarations.html#r-items.use.as-underscore
+          not rename.hasName() and
+          name = "_"
+        )


Note, the old disjunct here did in fact not do anything. When there is a rename it's name is never _. That is,

predicate isUnderscope(UseTree tree) { tree.getRename().getName().getText() = "_" }

is empty. Instead not rename.hasName() holds when _ is used in the source code.

paldepind · 2025-09-02T07:09:51Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

+        // order for the `impl` to be valid.
+        exists(Trait trait |
+          pragma[only_bind_into](trait) = impl.(ImplItemNode).resolveTraitTy() and
+          TraitIsVisible<relevantTraitVisible/2>::traitIsVisible(mc, pragma[only_bind_into](trait))


The pragma here is to prevent resolveTraitTy from being joined with traitIsVisible on the trait column.

hvitved

Looks great; good to have so many inconsistencies resolved.

Rust: Add examples where trait visibility affects path and method res…

c1c7127

…olution

github-actions bot added the Rust Pull requests that update Rust code label Aug 30, 2025

github-advanced-security bot found potential problems Aug 31, 2025

View reviewed changes

rust/ql/lib/codeql/rust/internal/PathResolution.qll Fixed Show fixed Hide fixed

paldepind force-pushed the rust/trait-method-scope-2 branch 2 times, most recently from fd51708 to 135ebce Compare September 1, 2025 13:15

paldepind mentioned this pull request Sep 2, 2025

Rust: Take trait visibility into account when resolving methods #20309

Closed

Rust: Account for trait visibility when resolving paths and methods

322ef4d

paldepind force-pushed the rust/trait-method-scope-2 branch from 135ebce to 322ef4d Compare September 2, 2025 07:01

paldepind marked this pull request as ready for review September 2, 2025 07:02

paldepind requested a review from a team as a code owner September 2, 2025 07:02

Copilot AI review requested due to automatic review settings September 2, 2025 07:02

Copilot AI reviewed Sep 2, 2025

View reviewed changes

paldepind commented Sep 2, 2025

View reviewed changes

paldepind added the no-change-note-required This PR does not need a change note label Sep 2, 2025

paldepind changed the title ~~Rust: Account for trait scope~~ Rust: Take trait visibility into account when resolving paths and methods Sep 2, 2025

hvitved approved these changes Sep 2, 2025

View reviewed changes

paldepind merged commit 0ed6428 into github:main Sep 2, 2025
21 of 22 checks passed

Rust: Take trait visibility into account when resolving paths and methods #20321

Rust: Take trait visibility into account when resolving paths and methods #20321

Uh oh!

Conversation

paldepind commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

paldepind commented Aug 30, 2025 •

edited

Loading

paldepind Sep 2, 2025 •

edited

Loading