Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Jun 23, 2025

#19824 follow-up.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jun 23, 2025
@hvitved hvitved marked this pull request as ready for review June 23, 2025 13:29
Copilot AI review requested due to automatic review settings June 23, 2025 13:29
@hvitved hvitved requested a review from a team as a code owner June 23, 2025 13:29
Copy link
Contributor

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 refactors the macro-expansion checks to use a unified root node and adds support for derive-macro expansions in isInMacroExpansion, updating the corresponding isFromMacroExpansion predicate to match the new signature.

  • Changed isInMacroExpansion to accept a generic AstNode root and handle derive-macro expansions.
  • Updated isFromMacroExpansion to call the revised predicate and adjust the token-tree check accordingly.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
rust/ql/lib/codeql/rust/elements/internal/MacroCallImpl.qll Refactored isInMacroExpansion signature and added getDeriveMacroExpansion branch
rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll Updated isFromMacroExpansion to use new isInMacroExpansion(AstNode,…) and adjusted cast for token-tree check
Comments suppressed due to low confidence (3)

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

  • [nitpick] The parameter name root is quite generic; consider renaming it to something more descriptive like expansionRoot to clarify its role.
  predicate isInMacroExpansion(AstNode root, AstNode n) {

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

  • Add unit tests to verify that isInMacroExpansion correctly recognizes nodes produced by derive macros, ensuring the new branch is covered.
    n = root.(Adt).getDeriveMacroExpansion(_)

rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll:73

  • [nitpick] For consistency with MacroCallImpl.qll, consider renaming this root parameter to expansionRoot so its purpose remains clear.
      exists(AstNode root |

Comment on lines +20 to 23
n = root.(Adt).getDeriveMacroExpansion(_)
or
isInMacroExpansion(root, n.getParentNode())
}
Copy link

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Instead of using the wildcard _ for the derive expansion, bind it to a named variable (e.g. getDeriveMacroExpansion(deriveExp)) to improve readability.

Suggested change
n = root.(Adt).getDeriveMacroExpansion(_)
or
isInMacroExpansion(root, n.getParentNode())
}
n = root.(Adt).getDeriveMacroExpansion(deriveExp)
or
isInMacroExpansion(root, n.getParentNode())
}
AstNode deriveExp;

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for fixing this!

@hvitved hvitved merged commit 1f559b2 into github:main Jun 23, 2025
16 checks passed
@hvitved hvitved deleted the rust/in-derive-macro branch June 23, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants