Rust: Path resolution before variable resolution

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/AstNodeImpl.qll

@@ -29,6 +29,24 @@ module Impl {
     result = getImmediateParent(e)
   }
 
+  /**
+   * Holds if `n` is superseded by an attribute macro expansion. That is, `n` is
+   * an item or a transitive child of an item with an attribute macro expansion.
+   */
+  predicate supersededByAttributeMacroExpansion(AstNode n) {
+    n.(Item).hasAttributeMacroExpansion()
+    or
+    exists(AstNode parent |
+      n.getParentNode() = parent and
+      supersededByAttributeMacroExpansion(parent) and
+      // Don't exclude expansions themselves as they supercede other nodes.
+      not n = parent.(Item).getAttributeMacroExpansion() and
+      // Don't consider attributes themselves to be superseded.  E.g., in `#[a] fn
+      // f() {}` the macro expansion supercedes `fn f() {}` but not `#[a]`.
+      not n instanceof Attr
+    )
+  }
+
   class AstNode extends Generated::AstNode {
     /**
      * Gets the nearest enclosing parent of this node, which is also an `AstNode`,

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/ConstImpl.qll

@@ -5,6 +5,8 @@
  */
 
 private import codeql.rust.elements.internal.generated.Const
+private import codeql.rust.elements.internal.AstNodeImpl::Impl as AstNodeImpl
+private import codeql.rust.elements.internal.IdentPatImpl::Impl as IdentPatImpl
 private import codeql.rust.elements.internal.PathExprImpl::Impl as PathExprImpl
 private import codeql.rust.internal.PathResolution
 
@@ -36,14 +38,30 @@ module Impl {
    * }
    * ```
    */
-  class ConstAccess extends PathExprImpl::PathExpr {
+  abstract class ConstAccess extends AstNodeImpl::AstNode {
+    /** Gets the constant being accessed. */
+    abstract Const getConst();
+
+    override string getAPrimaryQlClass() { result = "ConstAccess" }
+  }
+
+  private class PathExprConstAccess extends ConstAccess, PathExprImpl::PathExpr {
     private Const c;
 
-    ConstAccess() { c = resolvePath(this.getPath()) }
+    PathExprConstAccess() { c = resolvePath(this.getPath()) }
 
-    /** Gets the constant being accessed. */
-    Const getConst() { result = c }
+    override Const getConst() { result = c }
 
-    override string getAPrimaryQlClass() { result = "ConstAccess" }
+    override string getAPrimaryQlClass() { result = ConstAccess.super.getAPrimaryQlClass() }
+  }
+
+  private class IdentPatConstAccess extends ConstAccess, IdentPatImpl::IdentPat {
+    private Const c;
+
+    IdentPatConstAccess() { c = resolvePath(this) }
+
+    override Const getConst() { result = c }
+
+    override string getAPrimaryQlClass() { result = ConstAccess.super.getAPrimaryQlClass() }
   }
 }

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

@@ -20,8 +20,6 @@ module Impl {
     or
     n = root.(Adt).getDeriveMacroExpansion(_)
     or
-    n = root.(Item).getAttributeMacroExpansion()
-    or
     isInMacroExpansion(root, n.getParentNode())
   }
 

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

@@ -4,6 +4,7 @@
  * INTERNAL: Do not use.
  */
 
+private import rust
 private import codeql.rust.elements.internal.generated.PathExpr
 
 /**
@@ -25,5 +26,10 @@ module Impl {
     override string toStringImpl() { result = this.toAbbreviatedString() }
 
     override string toAbbreviatedString() { result = this.getPath().toStringImpl() }
+
+    override string getAPrimaryQlClass() {
+      result = super.getAPrimaryQlClass() and
+      not this instanceof VariableAccess
+    }
   }
 }

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

@@ -1,6 +1,8 @@
 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.AstNodeImpl::Impl as AstNodeImpl
 private import codeql.rust.elements.internal.PathImpl::Impl as PathImpl
 private import codeql.rust.elements.internal.PathExprBaseImpl::Impl as PathExprBaseImpl
 private import codeql.rust.elements.internal.FormatTemplateVariableAccessImpl::Impl as FormatTemplateVariableAccessImpl
@@ -15,21 +17,12 @@ module Impl {
 
   class BlockExprScope extends VariableScope, BlockExpr { }
 
-  abstract class MatchArmScope extends VariableScope {
-    MatchArm arm;
-
-    bindingset[arm]
-    MatchArmScope() { exists(arm) }
-
-    Pat getPat() { result = arm.getPat() }
+  class MatchArmExprScope extends VariableScope {
+    MatchArmExprScope() { this = any(MatchArm arm).getExpr() }
   }
 
-  class MatchArmExprScope extends MatchArmScope {
-    MatchArmExprScope() { this = arm.getExpr() }
-  }
-
-  class MatchArmGuardScope extends MatchArmScope {
-    MatchArmGuardScope() { this = arm.getGuard() }
+  class MatchArmGuardScope extends VariableScope {
+    MatchArmGuardScope() { this = any(MatchArm arm).getGuard() }
   }
 
   class ClosureBodyScope extends VariableScope {
@@ -41,7 +34,7 @@ module Impl {
    *
    * Such variables are only available in the body guarded by the condition.
    */
-  class ConditionScope extends VariableScope, Expr {
+  class ConditionScope extends VariableScope {
     private AstNode parent;
     private AstNode body;
 
@@ -57,6 +50,12 @@ module Impl {
           this = we.getCondition() and
           body = we.getLoopBody()
         )
+      or
+      parent =
+        any(MatchArm ma |
+          this = ma.getGuard() and
+          body = ma.getExpr()
+        )
     }
 
     /** Gets the parent of this condition. */
@@ -101,35 +100,34 @@ 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
-      definingNode = name and
-      text = name.getText() and
-      // exclude self parameters from functions without a body as these are
-      // trait method declarations without implementations
-      not exists(Function f | not f.hasBody() and f.getSelfParam() = sp)
-    )
-    or
-    exists(IdentPat pat |
-      name = pat.getName() and
-      (
-        definingNode = getOutermostEnclosingOrPat(pat)
-        or
-        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
-      // 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
-      // exclude parameters from function pointer types (e.g. `x` in `fn(x: i32) -> i32`)
-      not exists(FnPtrTypeRepr fp | fp.getParamList().getAParam().getPat() = pat)
+    not AstNodeImpl::supersededByAttributeMacroExpansion(definingNode) and
+    (
+      exists(SelfParam sp |
+        name = sp.getName() and
+        definingNode = name and
+        text = name.getText() and
+        // exclude self parameters from functions without a body as these are
+        // trait method declarations without implementations
+        not exists(Function f | not f.hasBody() and f.getSelfParam() = sp)
+      )
+      or
+      exists(IdentPat pat |
+        name = pat.getName() and
+        (
+          definingNode = getOutermostEnclosingOrPat(pat)
+          or
+          not exists(getOutermostEnclosingOrPat(pat)) and definingNode = name
+        ) and
+        text = name.getText() and
+        not PathResolution::identPatIsResolvable(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
+        // exclude parameters from function pointer types (e.g. `x` in `fn(x: i32) -> i32`)
+        not exists(FnPtrTypeRepr fp | fp.getParamList().getAParam().getPat() = pat)
+      )
     )
   }
 
@@ -417,11 +415,14 @@ module Impl {
       ord = getPreOrderNumbering(scope, scope)
       or
       exists(Pat pat | pat = getAVariablePatAncestor(v) |
-        scope =
-          any(MatchArmScope arm |
-            arm.getPat() = pat and
-            ord = getPreOrderNumbering(scope, arm)
-          )
+        exists(MatchArm arm |
+          pat = arm.getPat() and
+          ord = getPreOrderNumbering(scope, scope)
+        |
+          scope = arm.getGuard()
+          or
+          not arm.hasGuard() and scope = arm.getExpr()
+        )
         or
         exists(LetStmt let |
           let.getPat() = pat and

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

@@ -37,14 +37,17 @@ private module Cached {
     TFormatArgsArgIndex(Expr e) { e = any(FormatArgsArg a).getExpr() } or
     TItemNode(ItemNode i)
 
+  pragma[nomagic]
+  private predicate isMacroCallLocation(Location loc) { loc = any(MacroCall m).getLocation() }
+
   /**
    * Gets an element, of kind `kind`, that element `use` uses, if any.
    */
   cached
   Definition definitionOf(Use use, string kind) {
     result = use.getDefinition() and
     kind = use.getUseType() and
-    not result.getLocation() = any(MacroCall m).getLocation()
+    not isMacroCallLocation(result.getLocation())
   }
 }