Skip to content

Conversation

paldepind
Copy link
Contributor

@paldepind paldepind commented Sep 17, 2025

Note, since getImmediateParent finds the enclosing ItemNode wherever 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.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Sep 17, 2025
@paldepind paldepind force-pushed the rust/path-resolution-attribute-expansion branch 3 times, most recently from b31d5db to 261134a Compare September 19, 2025 11:27
@paldepind paldepind marked this pull request as ready for review September 19, 2025 11:38
@paldepind paldepind requested a review from a team as a code owner September 19, 2025 11:38
@Copilot Copilot AI review requested due to automatic review settings September 19, 2025 11:38
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]`.
Copy link
Preview

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]`.

Copilot uses AI. Check for mistakes.

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)
Copy link
Preview

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name 'commmentAt' has an extra 'm' and should be 'commentAt'.

Copilot uses AI. Check for mistakes.

*/
abstract class ItemNode extends Locatable {
ItemNode() {
// Exclude items that are superceded by the expansion of an attribute macro.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: superseded

path = p.toStringDebug()
}

predicate debugItemNode(ItemNode item) { item = getRelevantLocatable() }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think it is easier just to make getRelevantLocatable public (I already do that in on of my PRs).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I wanted to see the ItemNodes in a given range. getRelevantLocatable would give all Locateables wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; perhaps your use case was different from mine; I would add calls to Debug::getRelevantLocatable throughout the source code when debugging.

@paldepind paldepind force-pushed the rust/path-resolution-attribute-expansion branch from 261134a to afb6d30 Compare September 19, 2025 12:27
@paldepind paldepind added the no-change-note-required This PR does not need a change note label Sep 19, 2025
@hvitved
Copy link
Contributor

hvitved commented Sep 19, 2025

We should do the same for the CFG (on a separate PR).

@hvitved hvitved merged commit fdb0c6e into github:main Sep 20, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants