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.

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


+ 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
abstractAttrgetAnAttr();
pragma[nomagic]
finalAttrgetAttr(stringname){
result=this.getAnAttr()and
result.getMeta().getPath().(RelevantPath).isUnqualified(name)
}
/**
*Getsan attribute attached to this item.
*
*Returnsan attribute (`Attr`)that is associated with this item,ifany.
*Thismethod may return any one of the attributes present; to retrieve a specific
*attributebyname,use `getAttr`.
*/
abstractAttrgetAnAttr();
/**
*Getsan attribute with the given name attached to this item.
*
* @param nameTheunqualified name of the attribute to retrieve.
* @return Anattribute(`Attr`)with the specified name,if present on this item.
*Ifmultiple attributes with the same name are present, returns one of them.
*/
pragma[nomagic]
finalAttrgetAttr(stringname){
result=this.getAnAttr()and
result.getMeta().getPath().(RelevantPath).isUnqualified(name)
}
/**
*Holdsif this item has an attribute with the given name.
*
* @param nameTheunqualified name of the attribute to check for.
* @return Trueif this item has at least one attribute with the specified name.
*/

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

+ 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

+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

+ 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(PathmacroDefPath|
isInMacroExpansion(macroDefPath, n)and
crate.getASourceFile().getFile()=resolvePathCand(macroDefPath).getFile()
exists(PathmacroDefPath,ItemNodemacroDefItem|
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

 }

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


The argument order seems inconsistent with isInMacroExpansion below.
Suggested change
privatepredicateisMacroExpansion(AstNodeexpansion,PathmacroDefPath){
privatepredicateisMacroExpansion(PathmacroDefPath,AstNodeexpansion){

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

+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
predicateisInMacroFromCrateExpansion(CrateItemNodecrate,AstNoden){
exists(PathmacroDefPath|
isInMacroExpansion(macroDefPath, n)and
crate.getASourceFile().getFile()=resolvePathCand(macroDefPath).getFile()
CrateItemNodegetDollarCratePathCrate(RelevantPathdollarCratePath){
dollarCratePath.isDollarCrate()and
exists(PathmacroDefPath|
isInMacroExpansion(macroDefPath, dollarCratePath)and
result.getASourceFile().getFile()=resolvePathCand(macroDefPath).getFile()

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

+ 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

+ |
+ 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
notpathUsesNamespace(path, _)
notpathUsesNamespace(path, _)andexists(ns)

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

- // 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

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


Suggested change
predicateisInDollarCrateSupportedMacroExpansion(FilemacroDefFile,AstNoden){
privatepredicateisInDollarCrateSupportedMacroExpansion(FilemacroDefFile,AstNoden){

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

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


Suggested change
predicateresolveDollarCrate(RelevantPathp,CrateItemNodecrate){
privatepredicateresolveDollarCrate(RelevantPathp,CrateItemNodecrate){

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

+ 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-actionsBot 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 ab4e8bcCompare September 29, 2025 18:41

github-advanced-securityAI found potential problems Sep 29, 2025
View reviewed changes

Comment threadrust/ql/lib/codeql/rust/internal/PathResolution.qll Fixed

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

Rust: Macro call resolution
701cff3

hvitved force-pushed the rust/macro-call-resolution branch from 89b33ab to 701cff3Compare 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

CopilotAI 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:mainOct 2, 2025
19 checks passed

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

-abstractAttrgetAnAttr();
-pragma[nomagic]
-finalAttrgetAttr(stringname){
-result=this.getAnAttr()and
-result.getMeta().getPath().(RelevantPath).isUnqualified(name)
-}
+/**
+*Getsan attribute attached to this item.
+*
+*Returnsan attribute (`Attr`)that is associated with this item,ifany.
+*Thismethod may return any one of the attributes present; to retrieve a specific
+*attributebyname,use `getAttr`.
+*/
+abstractAttrgetAnAttr();
+/**
+*Getsan attribute with the given name attached to this item.
+*
+* @param nameTheunqualified name of the attribute to retrieve.
+* @return Anattribute(`Attr`)with the specified name,if present on this item.
+*Ifmultiple attributes with the same name are present, returns one of them.
+*/
+pragma[nomagic]
+finalAttrgetAttr(stringname){
+result=this.getAnAttr()and
+result.getMeta().getPath().(RelevantPath).isUnqualified(name)
+}
+/**
+*Holdsif this item has an attribute with the given name.
+*
+* @param nameTheunqualified name of the attribute to check for.
+* @return Trueif this item has at least one attribute with the specified name.
+*/

-exists(PathmacroDefPath|
-isInMacroExpansion(macroDefPath, n)and
-crate.getASourceFile().getFile()=resolvePathCand(macroDefPath).getFile()
+exists(PathmacroDefPath,ItemNodemacroDefItem|
+isInMacroExpansion(macroDefPath, n)and
+macroDefItem=resolvePathCand(macroDefPath)and
+macroDefItem.getEnclosingCrate()= crate

	privatepredicateisMacroExpansion(AstNodeexpansion,PathmacroDefPath){
	privatepredicateisMacroExpansion(PathmacroDefPath,AstNodeexpansion){

-predicateisInMacroFromCrateExpansion(CrateItemNodecrate,AstNoden){
-exists(PathmacroDefPath|
-isInMacroExpansion(macroDefPath, n)and
-crate.getASourceFile().getFile()=resolvePathCand(macroDefPath).getFile()
+CrateItemNodegetDollarCratePathCrate(RelevantPathdollarCratePath){
+dollarCratePath.isDollarCrate()and
+exists(PathmacroDefPath|
+isInMacroExpansion(macroDefPath, dollarCratePath)and
+result.getASourceFile().getFile()=resolvePathCand(macroDefPath).getFile()

	notpathUsesNamespace(path, _)
	notpathUsesNamespace(path, _)andexists(ns)

	predicateisInDollarCrateSupportedMacroExpansion(FilemacroDefFile,AstNoden){
	privatepredicateisInDollarCrateSupportedMacroExpansion(FilemacroDefFile,AstNoden){

	predicateresolveDollarCrate(RelevantPathp,CrateItemNodecrate){
	privatepredicateresolveDollarCrate(RelevantPathp,CrateItemNodecrate){

Uh oh!

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!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes:

Reviewed Changes

Uh oh!

CopilotAISep 30, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAISep 30, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAISep 30, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAISep 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!

hvitvedOct 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!

4 participants

hvitved commented Sep 29, 2025•
edited
Loading

hvitvedOct 2, 2025•
edited
Loading