Rust: Macro call resolution by hvitved · Pull Request #20548 · github/codeql

hvitved · 2025-09-29T13:05:48Z

This PR implements resolution of macro calls (macro expansion is still done in the extractor). The motivation for doing this, in addition to having the resolution information available, is to provide more accurate resolution of $crate paths, which in turn means fewer inconsistencies.

DCA looks great; a significant reduction in type inference inconsistencies (down 20,267 from 83,257) and a small performance improvement, at the cost of only a small reduction in resolved calls.

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

Copilot

Pull Request Overview

This PR implements resolution of macro calls in Rust, providing better accuracy for $crate path resolution and reducing type inference inconsistencies. The changes introduce macro namespace support and improve path resolution across the codebase.

Key Changes:

Added macro namespace support alongside existing value and type namespaces
Implemented macro call resolution with new MacroItemNode classes
Enhanced $crate path resolution within macro expansions
Updated path resolution logic to handle macro-specific visibility and scoping rules

Reviewed Changes

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

Show a summary per file

File	Description
PathResolution.qll	Core implementation of macro namespace, macro item nodes, and enhanced path resolution logic
PathResolutionConsistency.qll	Updated consistency checks to handle macro resolution improvements
MacroCallImpl.qll	Added macro resolution capability to MacroCall AST nodes
PathResolutionInlineExpectationsTest.qll	Enhanced test filtering to exclude macro-specific test markers
Various .expected files	Updated test expectations reflecting improved macro resolution accuracy
Various test source files	Added macro resolution test annotations and examples

Copilot · 2025-09-30T14:26:29Z

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


+  abstract Attr getAnAttr();
+
+  pragma[nomagic]
+  final Attr getAttr(string name) {
+    result = this.getAnAttr() and
+    result.getMeta().getPath().(RelevantPath).isUnqualified(name)
+  }
+


The new attribute-related methods (getAnAttr, getAttr, hasAttr) lack documentation explaining their purpose and usage. These methods appear to be fundamental for macro resolution but need proper documentation.

Suggested change

abstract Attr getAnAttr();

pragma[nomagic]

final Attr getAttr(string name) {

result = this.getAnAttr() and

result.getMeta().getPath().(RelevantPath).isUnqualified(name)

}

/**

* Gets an attribute attached to this item.

*

* Returns an attribute (`Attr`) that is associated with this item, if any.

* This method may return any one of the attributes present; to retrieve a specific

* attribute by name, use `getAttr`.

*/

abstract Attr getAnAttr();

/**

* Gets an attribute with the given name attached to this item.

*

* @param name The unqualified name of the attribute to retrieve.

* @return An attribute (`Attr`) with the specified name, if present on this item.

* If multiple attributes with the same name are present, returns one of them.

*/

pragma[nomagic]

final Attr getAttr(string name) {

result = this.getAnAttr() and

result.getMeta().getPath().(RelevantPath).isUnqualified(name)

}

/**

* Holds if this item has an attribute with the given name.

*

* @param name The unqualified name of the attribute to check for.

* @return True if this item has at least one attribute with the specified name.

*/

Copilot · 2025-09-30T14:26:29Z

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

+  override Namespace getNamespace() {
+    // see https://doc.rust-lang.org/reference/procedural-macros.html
+    if this.hasAttr(["proc_macro", "proc_macro_attribute", "proc_macro_derive"])
+    then result.isMacro()
+    else result.isValue()
+  }


The logic for determining function namespace based on procedural macro attributes needs more detailed explanation. The comment references external documentation but doesn't explain the specific behavior being implemented.

Copilot · 2025-09-30T14:26:30Z

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

+private predicate macroUseEdge(
+  ItemNode i, string name, SuccessorKind kind, UseOption useOpt, MacroItemNode macro
+) {
+  exists(ItemNode m |
+    m = i.getASuccessor(_, _, useOpt) and
+    m.hasAttr("macro_use")
+  |
+    macro = m.(ModuleItemNode).getASuccessor(name, kind, _)
+    or
+    macro = m.(ExternCrateItemNode).getASuccessor(_, _, _).getASuccessor(name, kind, _)
+  )
 }


The macroUseEdge predicate implements complex macro visibility rules but lacks documentation explaining the two different resolution paths (ModuleItemNode vs ExternCrateItemNode) and when each applies.

Copilot · 2025-09-30T14:26:30Z

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

+  exists(Path macroDefPath |
+    isInMacroExpansion(macroDefPath, n) and
+    crate.getASourceFile().getFile() = resolvePathCand(macroDefPath).getFile()


The isInMacroFromCrateExpansion predicate has a documented limitation ('may originate from crate') but the implementation logic for determining crate origin is not clearly explained. The file-based comparison seems fragile and needs clarification.

Suggested change

exists(Path macroDefPath |

isInMacroExpansion(macroDefPath, n) and

crate.getASourceFile().getFile() = resolvePathCand(macroDefPath).getFile()

exists(Path macroDefPath, ItemNode macroDefItem |

isInMacroExpansion(macroDefPath, n) and

macroDefItem = resolvePathCand(macroDefPath) and

macroDefItem.getEnclosingCrate() = crate

paldepind

Looks good!

If it's not too hard, I think it would be worthwhile adding tests for $crate as the implementation is not trivial, especially when $crate is in an expansion within an expansion.

paldepind · 2025-10-01T08:28:34Z

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

 }

+pragma[nomagic]
+private predicate isMacroExpansion(AstNode expansion, Path macroDefPath) {


The argument order seems inconsistent with isInMacroExpansion below.

Suggested change

private predicate isMacroExpansion(AstNode expansion, Path macroDefPath) {

private predicate isMacroExpansion(Path macroDefPath, AstNode expansion) {

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

paldepind · 2025-10-01T08:35:10Z

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

+predicate isInMacroFromCrateExpansion(CrateItemNode crate, AstNode n) {
+  exists(Path macroDefPath |
+    isInMacroExpansion(macroDefPath, n) and
+    crate.getASourceFile().getFile() = resolvePathCand(macroDefPath).getFile()


This predicate is only used with n being a $crate path. Inlining that as below changes the predicate size from 1.354.637 to 4.683 on the path resolution test DB.

Making it a predicate with result might also be more natural?

Suggested change

predicate isInMacroFromCrateExpansion(CrateItemNode crate, AstNode n) {

exists(Path macroDefPath |

isInMacroExpansion(macroDefPath, n) and

crate.getASourceFile().getFile() = resolvePathCand(macroDefPath).getFile()

CrateItemNode getDollarCratePathCrate(RelevantPath dollarCratePath) {

dollarCratePath.isDollarCrate() and

exists(Path macroDefPath |

isInMacroExpansion(macroDefPath, dollarCratePath) and

result.getASourceFile().getFile() = resolvePathCand(macroDefPath).getFile()

paldepind · 2025-10-01T08:41:29Z

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

+  exists(RelevantPath path |
+    path.isUnqualified(name, encl) and
+    ancestor = encl and
+    exists(ns) and


This is not needed and ns is restricted in one of the cases below.

Suggested change

exists(ns) and

paldepind · 2025-10-01T08:42:01Z

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

+  |
+    pathUsesNamespace(path, ns)
+    or
+    not pathUsesNamespace(path, _)


We could add the exists(ns) here for clarity, but it doesn't change the semantics.

Suggested change

not pathUsesNamespace(path, _)

not pathUsesNamespace(path, _) and exists(ns)

paldepind · 2025-10-01T08:45:34Z

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

-  // known limitation for `$crate`
-  not p.getQualifier*().(RelevantPath).isUnqualified("$crate") and
+  // `panic` is defined in both `std` and `core`; both are included in the prelude
+  not p.getText() = "panic" and


Is panic the only thing where this the case?

It's the only thing I saw in the tests, at least.

paldepind

Why do path resolution inconsistencies increase for some projects? For instance, Tokio increases from 655 to 9030.

paldepind · 2025-10-02T08:33:44Z

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

+}
+
+pragma[nomagic]
+predicate isInDollarCrateSupportedMacroExpansion(File macroDefFile, AstNode n) {


Suggested change

predicate isInDollarCrateSupportedMacroExpansion(File macroDefFile, AstNode n) {

private predicate isInDollarCrateSupportedMacroExpansion(File macroDefFile, AstNode n) {

paldepind · 2025-10-02T08:34:17Z

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

+ * calls.
+ */
+pragma[nomagic]
+predicate resolveDollarCrate(RelevantPath p, CrateItemNode crate) {


Suggested change

predicate resolveDollarCrate(RelevantPath p, CrateItemNode crate) {

private predicate resolveDollarCrate(RelevantPath p, CrateItemNode crate) {

paldepind · 2025-10-02T08:43:51Z

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

+    macroDefFile = resolvePathCand(macroDefPath).getFile()
+  )
+  or
+  isInDollarCrateSupportedMacroExpansion(macroDefFile, n.getParentNode())


This predicate holds for all AST nodes within an expansion, but we only care about $crate notes. And for expansions within expansions we end up with 2 * nrOfAstNodes tuples?

Wouldn't it have been more efficient to have

a predicate from $crate paths to their enclosing expansion and

a predicate from a macro call expansion to its enclosing macro call expansion.

And then use these in resolveDollarCrate to find all the potential crates? That would avoid a predicate that holds for all AST nodes within expansions.

I'll use doublyBoundedFastTC.

Thanks, after looking up what doublyBoundedFastTC I think that approach is really clean!

hvitved · 2025-10-02T09:29:51Z

Why do path resolution inconsistencies increase for some projects? For instance, Tokio increases from 655 to 9030.

This happens because we are now also resolving paths to macros, and in some cases those are not unique. We could investigate follow-up.

paldepind · 2025-10-02T09:44:04Z

This happens because we are now also resolving paths to macros, and in some cases those are not unique. We could investigate follow-up.

Thanks. That seems reasonable :)

paldepind · 2025-10-02T09:44:46Z

I've approved, but maybe we need another DCA run?

github-actions bot added the Rust Pull requests that update Rust code label Sep 29, 2025

hvitved force-pushed the rust/macro-call-resolution branch 4 times, most recently from fd9f2fb to ab4e8bc Compare September 29, 2025 18:41

github-advanced-security bot found potential problems Sep 29, 2025

View reviewed changes

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

hvitved force-pushed the rust/macro-call-resolution branch 4 times, most recently from d43b6d2 to 89b33ab Compare September 30, 2025 13:43

Rust: Macro call resolution

701cff3

hvitved force-pushed the rust/macro-call-resolution branch from 89b33ab to 701cff3 Compare September 30, 2025 14:22

Rust: Fix formatting

537e7a8

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

hvitved marked this pull request as ready for review September 30, 2025 14:25

hvitved requested a review from a team as a code owner September 30, 2025 14:25

hvitved requested review from Copilot and paldepind September 30, 2025 14:25

Copilot AI reviewed Sep 30, 2025

View reviewed changes

paldepind requested changes Oct 1, 2025

View reviewed changes

Address review comments

dd3debc

hvitved requested a review from paldepind October 1, 2025 12:46

paldepind requested changes Oct 2, 2025

View reviewed changes

Rust: Use doubleBoundedFastTC for resolving $crate paths

f8b104d

paldepind approved these changes Oct 2, 2025

View reviewed changes

hvitved merged commit b4c979f into github:main Oct 2, 2025
19 checks passed

hvitved deleted the rust/macro-call-resolution branch October 2, 2025 12:54

-  abstract Attr getAnAttr();
-  pragma[nomagic]
-  final Attr getAttr(string name) {
-    result = this.getAnAttr() and
-    result.getMeta().getPath().(RelevantPath).isUnqualified(name)
-  }
+  /**
+   * Gets an attribute attached to this item.
+   *
+   * Returns an attribute (`Attr`) that is associated with this item, if any.
+   * This method may return any one of the attributes present; to retrieve a specific
+   * attribute by name, use `getAttr`.
+   */
+  abstract Attr getAnAttr();
+  /**
+   * Gets an attribute with the given name attached to this item.
+   *
+   * @param name The unqualified name of the attribute to retrieve.
+   * @return An attribute (`Attr`) with the specified name, if present on this item.
+   *         If multiple attributes with the same name are present, returns one of them.
+   */
+  pragma[nomagic]
+  final Attr getAttr(string name) {
+    result = this.getAnAttr() and
+    result.getMeta().getPath().(RelevantPath).isUnqualified(name)
+  }
+  /**
+   * Holds if this item has an attribute with the given name.
+   *
+   * @param name The unqualified name of the attribute to check for.
+   * @return True if this item has at least one attribute with the specified name.
+   */

	private predicate isMacroExpansion(AstNode expansion, Path macroDefPath) {
	private predicate isMacroExpansion(Path macroDefPath, AstNode expansion) {

	not pathUsesNamespace(path, _)
	not pathUsesNamespace(path, _) and exists(ns)

	predicate isInDollarCrateSupportedMacroExpansion(File macroDefFile, AstNode n) {
	private predicate isInDollarCrateSupportedMacroExpansion(File macroDefFile, AstNode n) {

	predicate resolveDollarCrate(RelevantPath p, CrateItemNode crate) {
	private predicate resolveDollarCrate(RelevantPath p, CrateItemNode crate) {

Conversation

hvitved commented Sep 29, 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

Key Changes:

Reviewed Changes

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paldepind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved Oct 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!

Choose a reason for hiding this comment

Uh oh!

hvitved commented Oct 2, 2025

Uh oh!

paldepind commented Oct 2, 2025

Uh oh!

paldepind commented Oct 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hvitved commented Sep 29, 2025 •

edited

Loading

hvitved Oct 2, 2025 •

edited

Loading