Rust: Account for attribute expansions in path resolution#20454
Merged
hvitved merged 7 commits intogithub:mainfrom Sep 20, 2025
Merged
Rust: Account for attribute expansions in path resolution#20454hvitved merged 7 commits intogithub:mainfrom
hvitved merged 7 commits intogithub:mainfrom
Conversation
paldepind
force-pushed
the
rust/path-resolution-attribute-expansion
branch
3 times, most recently
from
b31d5db to
261134a
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR improves Rust path resolution by accounting for attribute macro expansions. When items have attribute macros applied, they may be expanded/transformed, and the original items should be excluded from path resolution to prevent duplicate or incorrect resolution targets.
- Introduces a new predicate to identify items superceded by attribute macro expansions
- Filters out superceded items from the ItemNode class hierarchy
- Updates test expectations to reflect improved path resolution accuracy
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
rust/ql/lib/codeql/rust/internal/PathResolution.qll |
Adds logic to exclude items superceded by attribute macro expansions from path resolution |
rust/ql/lib/utils/test/PathResolutionInlineExpectationsTest.qll |
Removes macro filtering from test expectations |
rust/ql/test/library-tests/path-resolution/main.rs |
Adds test cases for attribute macro scenarios |
rust/ql/test/library-tests/path-resolution/proc_macro.rs |
Adds identity macro for testing |
.expected files |
Updated test expectations reflecting improved path resolution |
Comment on lines
82
to
94
| * Holds if `n` is superceded by an attribute macro expansion. That is, `n` is | ||
| * an item or a transitive child of an item with an attribute macro expansion. | ||
| */ | ||
| predicate supercededByAttributeMacroExpansion(AstNode n) { | ||
| n.(Item).hasAttributeMacroExpansion() | ||
| or | ||
| exists(AstNode parent | | ||
| n.getParentNode() = parent and | ||
| supercededByAttributeMacroExpansion(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 superceded. E.g., in `#[a] fn | ||
| // f() {}` the macro expansion supercedes `fn f() {}` but not `#[a]`. |
There was a problem hiding this comment.
The word 'superceded' should be spelled 'superseded' in the predicate name, comments, and documentation.
Suggested change
| * Holds if `n` is superceded by an attribute macro expansion. That is, `n` is | |
| * an item or a transitive child of an item with an attribute macro expansion. | |
| */ | |
| predicate supercededByAttributeMacroExpansion(AstNode n) { | |
| n.(Item).hasAttributeMacroExpansion() | |
| or | |
| exists(AstNode parent | | |
| n.getParentNode() = parent and | |
| supercededByAttributeMacroExpansion(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 superceded. E.g., in `#[a] fn | |
| // f() {}` the macro expansion supercedes `fn f() {}` but not `#[a]`. | |
| * 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 supersede 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 supersedes `fn f() {}` but not `#[a]`. |
| private predicate item(ItemNode i, string value) { | ||
| exists(string filepath, int line, boolean inMacro | itemAt(i, filepath, line, inMacro) | | ||
| commmentAt(value, filepath, line) and inMacro = false | ||
| commmentAt(value, filepath, line) |
There was a problem hiding this comment.
The function name 'commmentAt' has an extra 'm' and should be 'commentAt'.
hvitved
reviewed
Sep 19, 2025
| */ | ||
| abstract class ItemNode extends Locatable { | ||
| ItemNode() { | ||
| // Exclude items that are superceded by the expansion of an attribute macro. |
| path = p.toStringDebug() | ||
| } | ||
|
|
||
| predicate debugItemNode(ItemNode item) { item = getRelevantLocatable() } |
Contributor
There was a problem hiding this comment.
Actually, I think it is easier just to make getRelevantLocatable public (I already do that in on of my PRs).
Contributor
Author
There was a problem hiding this comment.
In this case I wanted to see the ItemNodes in a given range. getRelevantLocatable would give all Locateables wouldn't it?
Contributor
There was a problem hiding this comment.
Yes; perhaps your use case was different from mine; I would add calls to Debug::getRelevantLocatable throughout the source code when debugging.
The annotated impl block was filtered away, but it's children where not. This caused the associated type `Foo` to appear as if it was an item in the scope outside of the impl block.
paldepind
force-pushed
the
rust/path-resolution-attribute-expansion
branch
from
261134a to
afb6d30
Compare
Contributor
|
We should do the same for the CFG (on a separate PR). |
hvitved
approved these changes
Sep 19, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note, since
getImmediateParentfinds the enclosingItemNodewherever it may be, simply removing items that have an expanded macro attribute is enough to enable path resolution to find things in inside the expansion.Going per-commit shows a bug that my first approach introduced, and then a test and fix for that.