-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: Take derive macros into account in is{In,From}MacroExpansion
#19850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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
isInMacroExpansionto accept a genericAstNoderoot and handle derive-macro expansions. - Updated
isFromMacroExpansionto 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
rootis quite generic; consider renaming it to something more descriptive likeexpansionRootto 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
isInMacroExpansioncorrectly 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 thisrootparameter toexpansionRootso its purpose remains clear.
exists(AstNode root |
| n = root.(Adt).getDeriveMacroExpansion(_) | ||
| or | ||
| isInMacroExpansion(root, n.getParentNode()) | ||
| } |
Copilot
AI
Jun 23, 2025
There was a problem hiding this comment.
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.
| n = root.(Adt).getDeriveMacroExpansion(_) | |
| or | |
| isInMacroExpansion(root, n.getParentNode()) | |
| } | |
| n = root.(Adt).getDeriveMacroExpansion(deriveExp) | |
| or | |
| isInMacroExpansion(root, n.getParentNode()) | |
| } | |
| AstNode deriveExp; |
geoffw0
left a comment
There was a problem hiding this 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!
#19824 follow-up.