Rust: Path resolution before variable resolution

Author: hvitved

rust/ql/integration-tests/hello-workspace/path-resolution.ql

@@ -2,7 +2,7 @@ import rust
 import codeql.rust.internal.PathResolution
 import utils.test.PathResolutionInlineExpectationsTest
 
-query predicate resolveDollarCrate(RelevantPath p, Crate c) {
+query predicate resolveDollarCrate(PathExt p, Crate c) {
   c = resolvePath(p) and
   p.isDollarCrate() and
   p.fromSource() and

rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll

@@ -1,5 +1,6 @@
 private import codeql.util.Boolean
 private import codeql.rust.controlflow.ControlFlowGraph
+private import codeql.rust.elements.internal.VariableImpl::Impl as VariableImpl
 private import rust
 
 newtype TCompletion =
@@ -123,13 +124,7 @@ class BooleanCompletion extends ConditionalCompletion, TBooleanCompletion {
  */
 private predicate cannotCauseMatchFailure(Pat pat) {
   pat instanceof RangePat or
-  // Identifier patterns that are in fact path patterns can cause failures. For
-  // instance `None`. Only if an `@ ...` part is present can we be sure that
-  // it's an actual identifier pattern. As a heuristic, if the identifier starts
-  // with a lower case letter, then we assume that it's an identifier.  This
-  // works for code that follows the Rust naming convention for enums and
-  // constants.
-  pat = any(IdentPat p | p.hasPat() or p.getName().getText().charAt(0).isLowercase()) or
+  pat = any(IdentPat p | p.hasPat() or VariableImpl::variableDecl(_, p.getName(), _)) or
   pat instanceof WildcardPat or
   pat instanceof RestPat or
   pat instanceof RefPat or

rust/ql/lib/codeql/rust/elements/internal/CallImpl.qll

@@ -82,7 +82,7 @@ module Impl {
   }
 
   private predicate callHasTraitQualifier(CallExpr call, Trait qualifier) {
-    exists(RelevantPath qualifierPath |
+    exists(PathExt qualifierPath |
       callHasQualifier(call, _, qualifierPath) and
       qualifier = resolvePath(qualifierPath) and
       // When the qualifier is `Self` and resolves to a trait, it's inside a

rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll

@@ -1,5 +1,6 @@
 private import rust
 private import codeql.rust.controlflow.ControlFlowGraph
+private import codeql.rust.internal.PathResolution as PathResolution
 private import codeql.rust.elements.internal.generated.ParentChild as ParentChild
 private import codeql.rust.elements.internal.PathImpl::Impl as PathImpl
 private import codeql.rust.elements.internal.PathExprBaseImpl::Impl as PathExprBaseImpl
@@ -101,7 +102,7 @@ module Impl {
    * pattern.
    */
   cached
-  private predicate variableDecl(AstNode definingNode, Name name, string text) {
+  predicate variableDecl(AstNode definingNode, Name name, string text) {
     Cached::ref() and
     exists(SelfParam sp |
       name = sp.getName() and
@@ -120,11 +121,7 @@ module Impl {
         not exists(getOutermostEnclosingOrPat(pat)) and definingNode = name
       ) and
       text = name.getText() and
-      // exclude for now anything starting with an uppercase character, which may be a reference to
-      // an enum constant (e.g. `None`). This excludes static and constant variables (UPPERCASE),
-      // which we don't appear to recognize yet anyway. This also assumes programmers follow the
-      // naming guidelines, which they generally do, but they're not enforced.
-      not text.charAt(0).isUppercase() and
+      not PathResolution::isResolvablePath(pat) and
       // exclude parameters from functions without a body as these are trait method declarations
       // without implementations
       not exists(Function f | not f.hasBody() and f.getAParam().getPat() = pat) and

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

@@ -205,7 +205,7 @@ abstract class ItemNode extends Locatable {
   pragma[nomagic]
   final Attr getAttr(string name) {
     result = this.getAnAttr() and
-    result.getMeta().getPath().(RelevantPath).isUnqualified(name)
+    result.getMeta().getPath().(PathExt).isUnqualified(name)
   }
 
   final predicate hasAttr(string name) { exists(this.getAttr(name)) }
@@ -1503,17 +1503,22 @@ private predicate declares(ItemNode item, Namespace ns, string name) {
   )
 }
 
-/** A path that does not access a local variable. */
-class RelevantPath extends Path {
-  RelevantPath() { not this = any(VariableAccess va).(PathExpr).getPath() }
+/**
+ * A `Path` or an `IdentPat`.
+ *
+ * `IdentPat`s are included in order to resolve unqualified references
+ * to constructors in patterns.
+ */
+abstract class PathExt extends AstNode {
+  abstract string getText();
 
   /** Holds if this is an unqualified path with the textual value `name`. */
   pragma[nomagic]
-  predicate isUnqualified(string name) {
-    not exists(this.getQualifier()) and
-    not this = any(UseTreeList list).getAUseTree().getPath().getQualifier*() and
-    name = this.getText()
-  }
+  abstract predicate isUnqualified(string name);
+
+  abstract Path getQualifier();
+
+  abstract string toStringDebug();
 
   /**
    * Holds if this is an unqualified path with the textual value `name` and
@@ -1535,6 +1540,30 @@ class RelevantPath extends Path {
   predicate isDollarCrate() { this.isUnqualified("$crate", _) }
 }
 
+private class PathExtPath extends PathExt instanceof Path {
+  override string getText() { result = Path.super.getText() }
+
+  override predicate isUnqualified(string name) {
+    not exists(Path.super.getQualifier()) and
+    not this = any(UseTreeList list).getAUseTree().getPath().getQualifier*() and
+    name = Path.super.getText()
+  }
+
+  override Path getQualifier() { result = Path.super.getQualifier() }
+
+  override string toStringDebug() { result = Path.super.toStringDebug() }
+}
+
+private class PathExtIdentPat extends PathExt, IdentPat {
+  override string getText() { result = this.getName().getText() }
+
+  override predicate isUnqualified(string name) { name = this.getText() }
+
+  override Path getQualifier() { none() }
+
+  override string toStringDebug() { result = this.getText() }
+}
+
 private predicate isModule(ItemNode m) { m instanceof Module }
 
 /** Holds if source file `source` contains the module `m`. */
@@ -1558,7 +1587,7 @@ private ItemNode getOuterScope(ItemNode i) {
 pragma[nomagic]
 private predicate unqualifiedPathLookup(ItemNode ancestor, string name, Namespace ns, ItemNode encl) {
   // lookup in the immediately enclosing item
-  exists(RelevantPath path |
+  exists(PathExt path |
     path.isUnqualified(name, encl) and
     ancestor = encl and
     not name = ["crate", "$crate", "super", "self"]
@@ -1594,7 +1623,7 @@ private ItemNode getASuccessor(
 
 private predicate isSourceFile(ItemNode source) { source instanceof SourceFileItemNode }
 
-private predicate hasCratePath(ItemNode i) { any(RelevantPath path).isCratePath(_, i) }
+private predicate hasCratePath(ItemNode i) { any(PathExt path).isCratePath(_, i) }
 
 private predicate hasChild(ItemNode parent, ItemNode child) { child.getImmediateParent() = parent }
 
@@ -1606,7 +1635,7 @@ private predicate sourceFileHasCratePathTc(ItemNode i1, ItemNode i2) =
  * `name` may be looked up inside `ancestor`.
  */
 pragma[nomagic]
-private predicate keywordLookup(ItemNode ancestor, string name, RelevantPath p) {
+private predicate keywordLookup(ItemNode ancestor, string name, PathExt p) {
   // For `crate`, jump directly to the root module
   exists(ItemNode i | p.isCratePath(name, i) |
     ancestor instanceof SourceFile and
@@ -1620,7 +1649,7 @@ private predicate keywordLookup(ItemNode ancestor, string name, RelevantPath p)
 }
 
 pragma[nomagic]
-private ItemNode unqualifiedPathLookup(RelevantPath p, Namespace ns, SuccessorKind kind) {
+private ItemNode unqualifiedPathLookup(PathExt p, Namespace ns, SuccessorKind kind) {
   exists(ItemNode ancestor, string name |
     result = getASuccessor(ancestor, pragma[only_bind_into](name), ns, kind, _) and
     kind.isInternalOrBoth()
@@ -1635,7 +1664,7 @@ private ItemNode unqualifiedPathLookup(RelevantPath p, Namespace ns, SuccessorKi
 }
 
 pragma[nomagic]
-private predicate isUnqualifiedSelfPath(RelevantPath path) { path.isUnqualified("Self") }
+private predicate isUnqualifiedSelfPath(PathExt path) { path.isUnqualified("Self") }
 
 /** Provides the input to `TraitIsVisible`. */
 signature predicate relevantTraitVisibleSig(Element element, Trait trait);
@@ -1718,14 +1747,14 @@ private module DollarCrateResolution {
     isDollarCrateSupportedMacroExpansion(_, expansion)
   }
 
-  private predicate isDollarCratePath(RelevantPath p) { p.isDollarCrate() }
+  private predicate isDollarCratePath(PathExt p) { p.isDollarCrate() }
 
-  private predicate isInDollarCrateMacroExpansion(RelevantPath p, AstNode expansion) =
+  private predicate isInDollarCrateMacroExpansion(PathExt p, AstNode expansion) =
     doublyBoundedFastTC(hasParent/2, isDollarCratePath/1, isDollarCrateSupportedMacroExpansion/1)(p,
       expansion)
 
   pragma[nomagic]
-  private predicate isInDollarCrateMacroExpansionFromFile(File macroDefFile, RelevantPath p) {
+  private predicate isInDollarCrateMacroExpansionFromFile(File macroDefFile, PathExt p) {
     exists(Path macroDefPath, AstNode expansion |
       isDollarCrateSupportedMacroExpansion(macroDefPath, expansion) and
       isInDollarCrateMacroExpansion(p, expansion) and
@@ -1740,17 +1769,17 @@ private module DollarCrateResolution {
    * calls.
    */
   pragma[nomagic]
-  predicate resolveDollarCrate(RelevantPath p, CrateItemNode crate) {
+  predicate resolveDollarCrate(PathExt p, CrateItemNode crate) {
     isInDollarCrateMacroExpansionFromFile(crate.getASourceFile().getFile(), p)
   }
 }
 
 pragma[nomagic]
-private ItemNode resolvePathCand0(RelevantPath path, Namespace ns) {
+private ItemNode resolvePathCand0(PathExt path, Namespace ns) {
   exists(ItemNode res |
     res = unqualifiedPathLookup(path, ns, _) and
     if
-      not any(RelevantPath parent).getQualifier() = path and
+      not any(PathExt parent).getQualifier() = path and
       isUnqualifiedSelfPath(path) and
       res instanceof ImplItemNode
     then result = res.(ImplItemNodeImpl).resolveSelfTyCand()
@@ -1767,7 +1796,7 @@ private ItemNode resolvePathCand0(RelevantPath path, Namespace ns) {
 }
 
 pragma[nomagic]
-private ItemNode resolvePathCandQualifier(RelevantPath qualifier, RelevantPath path, string name) {
+private ItemNode resolvePathCandQualifier(PathExt qualifier, PathExt path, string name) {
   qualifier = path.getQualifier() and
   result = resolvePathCand(qualifier) and
   name = path.getText()
@@ -1815,9 +1844,7 @@ private predicate checkQualifiedVisibility(
  * qualifier of `path` and `qualifier` resolves to `q`, if any.
  */
 pragma[nomagic]
-private ItemNode resolvePathCandQualified(
-  RelevantPath qualifier, ItemNode q, RelevantPath path, Namespace ns
-) {
+private ItemNode resolvePathCandQualified(PathExt qualifier, ItemNode q, PathExt path, Namespace ns) {
   exists(string name, SuccessorKind kind, UseOption useOpt |
     q = resolvePathCandQualifier(qualifier, path, name) and
     result = getASuccessor(q, name, ns, kind, useOpt) and
@@ -1826,12 +1853,14 @@ private ItemNode resolvePathCandQualified(
 }
 
 /** Holds if path `p` must be looked up in namespace `n`. */
-private predicate pathUsesNamespace(Path p, Namespace n) {
+private predicate pathUsesNamespace(PathExt p, Namespace n) {
   n.isValue() and
   (
     p = any(PathExpr pe).getPath()
     or
     p = any(TupleStructPat tsp).getPath()
+    or
+    p instanceof PathExtIdentPat
   )
   or
   n.isType() and
@@ -1907,7 +1936,7 @@ private predicate macroUseEdge(
  * result in non-monotonic recursion.
  */
 pragma[nomagic]
-private ItemNode resolvePathCand(RelevantPath path) {
+private ItemNode resolvePathCand(PathExt path) {
   exists(Namespace ns |
     result = resolvePathCand0(path, ns) and
     if path = any(ImplItemNode i).getSelfPath()
@@ -1920,7 +1949,10 @@ private ItemNode resolvePathCand(RelevantPath path) {
       else
         if path = any(PathTypeRepr p).getPath()
         then result instanceof TypeItemNode
-        else any()
+        else
+          if path instanceof IdentPat
+          then result instanceof VariantItemNode or result instanceof StructItemNode
+          else any()
   |
     pathUsesNamespace(path, ns)
     or
@@ -1937,7 +1969,7 @@ private ItemNode resolvePathCand(RelevantPath path) {
 }
 
 /** Get a trait that should be visible when `path` resolves to `node`, if any. */
-private Trait getResolvePathTraitUsed(RelevantPath path, AssocItemNode node) {
+private Trait getResolvePathTraitUsed(PathExt path, AssocItemNode node) {
   exists(TypeItemNode type, ImplItemNodeImpl impl |
     node = resolvePathCandQualified(_, type, path, _) and
     typeImplEdge(type, impl, _, _, node, _) and
@@ -1951,7 +1983,7 @@ private predicate pathTraitUsed(Element path, Trait trait) {
 
 /** Gets the item that `path` resolves to, if any. */
 cached
-ItemNode resolvePath(RelevantPath path) {
+ItemNode resolvePath(PathExt path) {
   result = resolvePathCand(path) and
   not path = any(Path parent | exists(resolvePathCand(parent))).getQualifier() and
   (
@@ -1964,29 +1996,30 @@ ItemNode resolvePath(RelevantPath path) {
   or
   // 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, _) = resolvePath(parent)
-  )
+  exists(PathExt parent | resolvePathCandQualified(path, result, parent, _) = resolvePath(parent))
 }
 
-private predicate isUseTreeSubPath(UseTree tree, RelevantPath path) {
+// use `forceLocal` once we implement overlay support
+predicate isResolvablePath(PathExt path) { exists(resolvePath(path)) }
+
+private predicate isUseTreeSubPath(UseTree tree, PathExt path) {
   path = tree.getPath()
   or
-  exists(RelevantPath mid |
+  exists(PathExt mid |
     isUseTreeSubPath(tree, mid) and
     path = mid.getQualifier()
   )
 }
 
 pragma[nomagic]
-private predicate isUseTreeSubPathUnqualified(UseTree tree, RelevantPath path, string name) {
+private predicate isUseTreeSubPathUnqualified(UseTree tree, PathExt path, string name) {
   isUseTreeSubPath(tree, path) and
   not exists(path.getQualifier()) and
   name = path.getText()
 }
 
 pragma[nomagic]
-private ItemNode resolveUseTreeListItem(Use use, UseTree tree, RelevantPath path, SuccessorKind kind) {
+private ItemNode resolveUseTreeListItem(Use use, UseTree tree, PathExt path, SuccessorKind kind) {
   exists(UseOption useOpt | checkQualifiedVisibility(use, result, kind, useOpt) |
     exists(UseTree midTree, ItemNode mid, string name |
       mid = resolveUseTreeListItem(use, midTree) and
@@ -2003,9 +2036,7 @@ private ItemNode resolveUseTreeListItem(Use use, UseTree tree, RelevantPath path
 }
 
 pragma[nomagic]
-private ItemNode resolveUseTreeListItemQualifier(
-  Use use, UseTree tree, RelevantPath path, string name
-) {
+private ItemNode resolveUseTreeListItemQualifier(Use use, UseTree tree, PathExt path, string name) {
   result = resolveUseTreeListItem(use, tree, path.getQualifier(), _) and
   name = path.getText()
 }
@@ -2137,12 +2168,12 @@ private module Debug {
     exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |
       result.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) and
       filepath.matches("%/main.rs") and
-      startline = 52
+      startline = 800
     )
   }
 
   predicate debugUnqualifiedPathLookup(
-    RelevantPath p, string name, Namespace ns, ItemNode ancestor, string path
+    PathExt p, string name, Namespace ns, ItemNode ancestor, string path
   ) {
     p = getRelevantLocatable() and
     exists(ItemNode encl |
@@ -2154,7 +2185,7 @@ private module Debug {
 
   predicate debugItemNode(ItemNode item) { item = getRelevantLocatable() }
 
-  ItemNode debugResolvePath(RelevantPath path) {
+  ItemNode debugResolvePath(PathExt path) {
     path = getRelevantLocatable() and
     result = resolvePath(path)
   }

rust/ql/test/library-tests/path-resolution/main.rs

@@ -797,7 +797,7 @@ mod patterns {
             Some(y) => { // $ item=Some
                 None // $ item=None
             }
-            None => // $ MISSING: item=None
+            None => // $ item=None
                 None // $ item=None
         };
         match y {

rust/ql/test/library-tests/path-resolution/path-resolution.expected

@@ -404,6 +404,7 @@ resolvePath
 | main.rs:796:24:796:26 | i32 | {EXTERNAL LOCATION} | struct i32 |
 | main.rs:797:13:797:16 | Some | {EXTERNAL LOCATION} | Some |
 | main.rs:798:17:798:20 | None | {EXTERNAL LOCATION} | None |
+| main.rs:800:13:800:16 | None | {EXTERNAL LOCATION} | None |
 | main.rs:801:17:801:20 | None | {EXTERNAL LOCATION} | None |
 | main.rs:811:5:811:6 | my | main.rs:1:1:1:7 | mod my |
 | main.rs:811:5:811:14 | ...::nested | my.rs:1:1:1:15 | mod nested |

rust/ql/test/library-tests/path-resolution/path-resolution.ql

@@ -5,7 +5,7 @@ import TestUtils
 
 query predicate mod(Module m) { toBeTested(m) }
 
-query predicate resolvePath(Path p, ItemNode i) {
+query predicate resolvePath(PathExt p, ItemNode i) {
   toBeTested(p) and
   not p.isFromMacroExpansion() and
   i = resolvePath(p)