Rust: Fallback crate resolution #20225

hvitved · 2025-08-14T11:03:56Z

Before this PR, we relied strictly on crate dependency information from the extractor when resolving paths to crates. However, for whatever reason, this information may sometimes be incomplete, so in this PR we relax this condition and allow for a path to resolve to any crate in the DB, as long as the name of that crate does not conflict with the name of a known dependency.

In order to reduce path resolution inconsistencies, this PR also adds a step to make resolution of qualified paths like q::p consistent; of all the candidate resolution targets of q, only chose the one(s) that are consistent with what q::p may resolve to.

DCA looks great: Percentage of calls with call target increases from 61.52 to 62.44, path resolution inconsistencies are reduced by 41 %, at only a marginal slowdown.

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

Copilot

Pull Request Overview

This PR introduces fallback crate resolution to handle cases where crate dependency information from the extractor may be incomplete. The key change allows paths to resolve to any crate in the database when the crate name doesn't conflict with known dependencies, and adds consistency improvements for qualified path resolution.

Key changes:

Adds fallback crate resolution when dependency information is missing
Implements consistency checks to ensure qualified paths like q::p resolve consistently with their qualifiers
Refactors path resolution to separate candidate resolution from final resolution with consistency checks

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

File	Description
PathResolutionConsistency.qll	Updates path resolution consistency check to exclude crate conflicts from multiple resolution warnings
PathResolution.qll	Major refactoring adding fallback crate resolution, consistency checks, and new implementation classes
TypeParamImpl.qll	Simplifies type bound resolution by delegating to TypeParamItemNode
*.expected files	Updated test expectations reflecting reduced path resolution inconsistencies

Copilot · 2025-08-25T19:52:56Z

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

+  // As a fallback, give all files access to crates that do not conflict with known dependencies
+  // and declarations.
+  name = dep.getName() and
+  not declaresDirectly(file, TTypeNamespace(), name) and


The TTypeNamespace() call should likely be name.getNamespace() or a similar method to get the appropriate namespace for the crate name, rather than always using the type namespace.

Suggested change

not declaresDirectly(file, TTypeNamespace(), name) and

not declaresDirectly(file, dep.getNamespace(), name) and

Copilot · 2025-08-25T19:52:56Z

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

@@ -1218,7 +1303,7 @@ class RelevantPath extends Path {
  pragma[nomagic]
  predicate isUnqualified(string name) {
    not exists(this.getQualifier()) and
-    not this = any(UseTreeList list).getAUseTree().getPath() and
+    not this = any(UseTreeList list).getAUseTree().getPath().getQualifier*() and
    name = this.getText()
  }



The transitive closure operator * after getQualifier creates a potential infinite recursion if there are circular qualifier relationships. Consider using a bounded recursion or a different approach to handle qualifier chains.

Suggested change

not isQualifiedViaUseTree(this) and

name = this.getText()

}

/**

* Helper predicate to check if `path` is reachable via qualifiers from any UseTreeList,

* using bounded recursion to avoid infinite loops in case of cycles.

*/

private predicate isQualifiedViaUseTree(Path path) {

exists(UseTreeList list, UseTree ut, Path p |

ut = list.getAUseTree() and

p = ut.getPath() and

isReachableQualifier(p, path, [])

)

}

/**

* Bounded recursion: holds if `target` is reachable from `current` via qualifiers,

* without revisiting any path in `visited`.

*/

private predicate isReachableQualifier(Path current, Path target, Path[] visited) {

current = target

or

not visited.contains(current) and

exists(Path next |

next = current.getQualifier() and

isReachableQualifier(next, target, visited + [current])

)

}

paldepind

Look good. Great improvement to the resolved calls percentage 🤩

I have left a few comments and questions.

paldepind · 2025-08-27T08:31:53Z

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

+  // if `path` is the qualifier of a resolvable `parent`, then we should
+  // resolve `path` to something consistent with what `parent` resolves to
+  exists(RelevantPath parent |
+    resolvePathCandQualified(path, result, parent, _) = resolvePathParent(path, parent)


resolvePathCandQualified implies that path is the qualifier of parent, so could this be simplified to

Suggested change

resolvePathCandQualified(path, result, parent, _) = resolvePathParent(path, parent)

resolvePathCandQualified(path, result, parent, _) = resolvePath(parent)

and resolvePathParent removed?

Indeed, good spot.

paldepind · 2025-08-27T08:37:51Z

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

@@ -748,6 +744,11 @@ private class ImplTraitTypeReprItemNode extends TypeItemNode instanceof ImplTrai
  override string getCanonicalPath(Crate c) { none() }
 }

+private class ImplTraitTypeReprItemNodeImpl extends ImplTraitTypeReprItemNode {
+  pragma[nomagic]
+  ItemNode resolveABound() { result = resolvePathCand(this.getABoundPath()) }


I think it's a bit subtle to have Foo.method do one thing and FooImpl.method do another thing. What about including cand in the name similar to resolvePath and resolvePathCand? Then this would be resolveABoundCand. And similar for the other "shadowed" method names?

paldepind · 2025-08-27T08:41:40Z

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

@@ -241,14 +241,14 @@ abstract class ItemNode extends Locatable {
    or
    // items made available by an implementation where `this` is the implementing type
    exists(ItemNode node |
-      this = node.(ImplItemNode).resolveSelfTy() and
+      this = node.(ImplItemNodeImpl).resolveSelfTy() and


Is it correct that we're using ImplItemNodeImpl only because resolveSelfTy from ImplItemNode would give non-monotonic recursion? If so, maybe we can write that down somewhere? Perhaps a QLdoc on resolvePathCand saying that it's used internally to avoid non-monotonic recursion?

paldepind · 2025-08-27T08:42:28Z

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

+  crateDependency(file, name, dep)
+  or
+  // As a fallback, give all files access to crates that do not conflict with known dependencies
+  // and declarations.


Maybe mention here that we need the fallback since the extractor based information used in crateDependency might be incomplete?

paldepind

Thanks for the PR and for addressing my comments. Looks great 💪

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

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

View reviewed changes

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

hvitved force-pushed the rust/path-resolution-crate-fallback branch 6 times, most recently from f1e4bb9 to 8b44182 Compare August 19, 2025 08:29

hvitved force-pushed the rust/path-resolution-crate-fallback branch 4 times, most recently from 4ee31bd to 8a3c3aa Compare August 25, 2025 07:24

hvitved added the no-change-note-required This PR does not need a change note label Aug 25, 2025

hvitved force-pushed the rust/path-resolution-crate-fallback branch 5 times, most recently from 245b001 to 4a32fc0 Compare August 25, 2025 18:43

hvitved marked this pull request as ready for review August 25, 2025 19:52

hvitved requested a review from a team as a code owner August 25, 2025 19:52

Copilot AI review requested due to automatic review settings August 25, 2025 19:52

Copilot AI reviewed Aug 25, 2025

View reviewed changes

paldepind requested changes Aug 27, 2025

View reviewed changes

hvitved added 2 commits August 28, 2025 09:34

Rust: Fallback crate resolution

dcaf4a7

Address review comments

f87f52d

hvitved force-pushed the rust/path-resolution-crate-fallback branch from 4a32fc0 to f87f52d Compare August 28, 2025 08:09

hvitved requested a review from paldepind August 28, 2025 08:09

paldepind approved these changes Aug 28, 2025

View reviewed changes

hvitved merged commit 0cc9ff8 into github:main Aug 28, 2025
18 checks passed

hvitved deleted the rust/path-resolution-crate-fallback branch August 28, 2025 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rust: Fallback crate resolution #20225

Rust: Fallback crate resolution #20225

hvitved commented Aug 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 25, 2025

Uh oh!

Copilot AI Aug 25, 2025

Uh oh!

paldepind left a comment

Uh oh!

paldepind Aug 27, 2025

Uh oh!

hvitved Aug 28, 2025

Uh oh!

paldepind Aug 27, 2025

Uh oh!

hvitved Aug 28, 2025

Uh oh!

paldepind Aug 27, 2025

Uh oh!

hvitved Aug 28, 2025

Uh oh!

paldepind Aug 27, 2025

Uh oh!

paldepind left a comment

Uh oh!

Uh oh!

Uh oh!

	not declaresDirectly(file, TTypeNamespace(), name) and
	not declaresDirectly(file, dep.getNamespace(), name) and

+    not isQualifiedViaUseTree(this) and
+    name = this.getText()
+  }
+  /**
+   * Helper predicate to check if `path` is reachable via qualifiers from any UseTreeList,
+   * using bounded recursion to avoid infinite loops in case of cycles.
+   */
+  private predicate isQualifiedViaUseTree(Path path) {
+    exists(UseTreeList list, UseTree ut, Path p |
+      ut = list.getAUseTree() and
+      p = ut.getPath() and
+      isReachableQualifier(p, path, [])
+    )
+  }
+  /**
+   * Bounded recursion: holds if `target` is reachable from `current` via qualifiers,
+   * without revisiting any path in `visited`.
+   */
+  private predicate isReachableQualifier(Path current, Path target, Path[] visited) {
+    current = target
+    or
+    not visited.contains(current) and
+    exists(Path next |
+      next = current.getQualifier() and
+      isReachableQualifier(next, target, visited + [current])
+    )
+  }

	resolvePathCandQualified(path, result, parent, _) = resolvePathParent(path, parent)
	resolvePathCandQualified(path, result, parent, _) = resolvePath(parent)

Rust: Fallback crate resolution #20225

Rust: Fallback crate resolution #20225

Conversation

hvitved commented Aug 14, 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 Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind left a comment

Choose a reason for hiding this comment

Uh oh!

paldepind Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hvitved commented Aug 14, 2025 •

edited

Loading