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

@@ -13,9 +13,9 @@ private import codeql.rust.controlflow.ControlFlowGraph
  */
 module Impl {
   private import rust
+  private import codeql.rust.elements.internal.ElementImpl::Impl as ElementImpl
   private import codeql.rust.elements.internal.generated.ParentChild
   private import codeql.rust.controlflow.ControlFlowGraph
-  private import codeql.rust.elements.internal.MacroCallImpl::Impl as MacroCallImpl
 
   /**
    * Gets the immediate parent of a non-`AstNode` element `e`.
@@ -71,21 +71,15 @@ module Impl {
     }
 
     /** Holds if this node is inside a macro expansion. */
-    predicate isInMacroExpansion() { MacroCallImpl::isInMacroExpansion(_, this) }
+    predicate isInMacroExpansion() { ElementImpl::MacroExpansion::isInMacroExpansion(this) }
 
     /**
      * Holds if this node exists only as the result of a macro expansion.
      *
      * This is the same as `isInMacroExpansion()`, but excludes AST nodes corresponding
-     * to macro arguments.
+     * to macro arguments, including attribute macro targets.
      */
-    pragma[nomagic]
-    predicate isFromMacroExpansion() {
-      exists(AstNode root |
-        MacroCallImpl::isInMacroExpansion(root, this) and
-        not this = root.(MacroCall).getATokenTreeNode()
-      )
-    }
+    predicate isFromMacroExpansion() { ElementImpl::MacroExpansion::isFromMacroExpansion(this) }
 
     /**
      * Gets a control flow node for this AST node, if any.

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

@@ -11,7 +11,118 @@ private import codeql.rust.elements.internal.generated.Element
  * be referenced directly.
  */
 module Impl {
+  private import rust
+  private import codeql.rust.elements.internal.generated.ParentChild
+  private import codeql.rust.elements.internal.generated.Synth
+  private import codeql.rust.elements.internal.generated.Raw
+  private import codeql.rust.elements.internal.LocationImpl
+
+  /**
+   * Provides logic for classifying elements with respect to macro expansions.
+   */
+  cached
+  module MacroExpansion {
+    /**
+     * Holds if `e` is superseded by an attribute macro expansion. That is, `e` is
+     * a transitive child of an item with an attribute macro expansion.
+     *
+     * Since this predicate is referenced in the charpred of `Element`, we need to
+     * use the parent-child relation on raw elements to avoid non-monotonicity.
+     */
+    private predicate supersededByAttributeMacroExpansionRaw(Raw::Item item, Raw::Element e) {
+      exists(item.getAttributeMacroExpansion()) and
+      e = Raw::getImmediateChild(item, _) and
+      not e = item.getAttributeMacroExpansion() and
+      // Don't consider attributes themselves to be superseded.  E.g., in `#[a] fn
+      // f() {}` the macro expansion supersedes `fn f() {}` but not `#[a]`.
+      not e instanceof Raw::Attr
+      or
+      exists(Raw::Element parent |
+        e = Raw::getImmediateChild(parent, _) and
+        supersededByAttributeMacroExpansionRaw(item, parent)
+      )
+    }
+
+    private predicate isMacroExpansion(AstNode macro, AstNode expansion) {
+      expansion = macro.(MacroCall).getMacroCallExpansion()
+      or
+      expansion = macro.(Adt).getDeriveMacroExpansion(_)
+      or
+      expansion = macro.(Item).getAttributeMacroExpansion()
+    }
+
+    /**
+     * Gets the immediately enclosing macro invocation for element `e`, if any.
+     *
+     * The result is either a `MacroCall`, and `Adt` with a derive macro expansion, or
+     * an `Item` with an attribute macro expansion.
+     */
+    cached
+    AstNode getImmediatelyEnclosingMacroInvocation(Element e) {
+      isMacroExpansion(result, e)
+      or
+      exists(Element mid |
+        result = getImmediatelyEnclosingMacroInvocation(mid) and
+        mid = getImmediateParent(e) and
+        not isMacroExpansion(mid, e)
+      )
+    }
+
+    pragma[nomagic]
+    private predicate isAttributeMacroExpansionSourceLocation(Item i, Location l) {
+      exists(Raw::Locatable e, @location_default loc |
+        supersededByAttributeMacroExpansionRaw(Synth::convertElementToRaw(i), e) and
+        locatable_locations(e, loc) and
+        l = LocationImpl::TLocationDefault(loc)
+      )
+    }
+
+    /** Gets an AST node whose location is inside the token tree belonging to `mc`. */
+    pragma[nomagic]
+    private AstNode getATokenTreeNode(MacroCall mc) {
+      mc = getImmediatelyEnclosingMacroInvocation(result) and
+      mc.getTokenTree().getLocation().contains(result.getLocation())
+    }
+
+    /** Holds if `n` is inside a macro expansion. */
+    cached
+    predicate isInMacroExpansion(AstNode n) { exists(getImmediatelyEnclosingMacroInvocation(n)) }
+
+    /**
+     * Holds if `n` exists only as the result of a macro expansion.
+     *
+     * This is the same as `isInMacroExpansion(n)`, but excludes AST nodes corresponding
+     * to macro arguments, including attribute macro targets.
+     *
+     * Note: This predicate is a heuristic based on location information and may not be
+     * accurate in all cases.
+     */
+    cached
+    predicate isFromMacroExpansion(AstNode n) {
+      exists(AstNode macro |
+        macro = getImmediatelyEnclosingMacroInvocation(n) and
+        not n = getATokenTreeNode(macro) and
+        not isAttributeMacroExpansionSourceLocation(macro, n.getLocation())
+      )
+      or
+      isFromMacroExpansion(getImmediatelyEnclosingMacroInvocation(n))
+    }
+
+    cached
+    predicate isRelevantElement(Generated::Element e) {
+      exists(Raw::Element raw |
+        raw = Synth::convertElementToRaw(e) and
+        not supersededByAttributeMacroExpansionRaw(_, raw)
+      )
+      or
+      // Synthetic elements are relevant when their parents are
+      Synth::convertFormatArgsExprToRaw(_) = Synth::getSynthParent(e)
+    }
+  }
+
   class Element extends Generated::Element {
+    Element() { MacroExpansion::isRelevantElement(this) }
+
     override string toStringImpl() { result = this.getAPrimaryQlClass() }
 
     /**

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

@@ -1,4 +1,3 @@
-// generated by codegen, remove this comment if you wish to edit this file
 /**
  * This module provides a hand-modifiable wrapper around the generated class `Item`.
  *
@@ -12,6 +11,7 @@ private import codeql.rust.elements.internal.generated.Item
  * be referenced directly.
  */
 module Impl {
+  // the following QLdoc is generated: if you need to edit it, do it in the schema file
   /**
    * An item such as a function, struct, enum, etc.
    *
@@ -23,4 +23,10 @@ module Impl {
    * ```
    */
   class Item extends Generated::Item { }
+
+  private class ItemWithAttributeMacroExpansion extends Item {
+    ItemWithAttributeMacroExpansion() { this.hasAttributeMacroExpansion() }
+
+    override string toStringImpl() { result = "(item with attribute macro expansion)" }
+  }
 }

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

@@ -5,6 +5,7 @@
  */
 
 import codeql.Locations
+private import codeql.rust.elements.internal.ElementImpl::Impl as ElementImpl
 private import codeql.rust.elements.internal.LocationImpl
 private import codeql.rust.elements.internal.generated.Locatable
 private import codeql.rust.elements.internal.generated.Synth
@@ -50,21 +51,12 @@ module Impl {
     locatable_locations(Synth::convertLocatableToRaw(l), result)
   }
 
-  private MacroCall getImmediatelyEnclosingMacroCall(AstNode n) {
-    result = n.getParentNode()
-    or
-    exists(AstNode mid |
-      result = getImmediatelyEnclosingMacroCall(mid) and
-      n.getParentNode() = mid and
-      not mid instanceof MacroCall
-    )
-  }
-
   /** Gets the non-synthesized location of `l`, if any. */
   LocationImpl::LocationDefault getLocationDefault(Locatable l) {
     result = LocationImpl::TLocationDefault(getDbLocation(l))
     or
     not exists(getDbLocation(l)) and
-    result = getLocationDefault(getImmediatelyEnclosingMacroCall(l))
+    result =
+      getLocationDefault(ElementImpl::MacroExpansion::getImmediatelyEnclosingMacroInvocation(l))
   }
 }

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

@@ -14,17 +14,6 @@ module Impl {
   private import rust
   private import codeql.rust.internal.PathResolution
 
-  pragma[nomagic]
-  predicate isInMacroExpansion(AstNode root, AstNode n) {
-    n = root.(MacroCall).getMacroCallExpansion()
-    or
-    n = root.(Adt).getDeriveMacroExpansion(_)
-    or
-    n = root.(Item).getAttributeMacroExpansion()
-    or
-    isInMacroExpansion(root, n.getParentNode())
-  }
-
   // the following QLdoc is generated: if you need to edit it, do it in the schema file
   /**
    * A macro invocation.
@@ -35,16 +24,7 @@ module Impl {
    * ```
    */
   class MacroCall extends Generated::MacroCall {
-    override string toStringImpl() {
-      if this.hasPath() then result = this.getPath().toAbbreviatedString() + "!..." else result = ""
-    }
-
-    /** Gets an AST node whose location is inside the token tree belonging to this macro call. */
-    pragma[nomagic]
-    AstNode getATokenTreeNode() {
-      isInMacroExpansion(this, result) and
-      this.getTokenTree().getLocation().contains(result.getLocation())
-    }
+    override string toStringImpl() { result = this.getPath().toAbbreviatedString() + "!..." }
 
     /**
      * Gets the macro definition that this macro call resolves to.

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
@@ -98,7 +100,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
@@ -117,11 +119,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::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