Uh oh!
There was an error while loading. Please reload this page.
Rust: Remove elements superseded by attribute macro expansions - #20770
Conversation
e305f21 to
20bb58dCompare20bb58d to
c81f5f5CompareThere was a problem hiding this comment.
Pull Request Overview
This PR refactors macro expansion handling in the Rust QL library by consolidating logic into a central MacroExpansion module and introducing the isFromMacroExpansion() predicate to better distinguish between macro arguments and macro-generated code. The changes improve code organization, reduce duplication, and update test expectations to reflect the new behavior.
- Centralized macro expansion logic from
MacroCallImplandPathResolutionintoElementImpl::MacroExpansion - Introduced
isFromMacroExpansion()to exclude macro arguments from expansion checks - Updated queries to use the new predicate where appropriate
- Added custom
toString()for items with attribute macro expansions
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
rust/ql/lib/codeql/rust/elements/internal/ElementImpl.qll | Adds centralized MacroExpansion module with logic for classifying macro-related elements |
rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll | Updates isInMacroExpansion() and isFromMacroExpansion() to delegate to MacroExpansion module |
rust/ql/lib/codeql/rust/elements/internal/MacroCallImpl.qll | Removes isInMacroExpansion() and getATokenTreeNode() logic, now in ElementImpl |
rust/ql/lib/codeql/rust/elements/internal/ItemImpl.qll | Adds custom toString() for items with attribute macro expansions |
rust/ql/lib/codeql/rust/elements/internal/LocatableImpl.qll | Updates to use new getImmediatelyEnclosingMacroInvocation() helper |
rust/ql/lib/codeql/rust/internal/PathResolution.qll | Simplifies ItemNode constructor by removing supersededByAttributeMacroExpansion() |
rust/ql/src/queries/unusedentities/UnusedValue.ql | Updates to use isFromMacroExpansion() instead of isInMacroExpansion() |
rust/ql/src/queries/unusedentities/UnreachableCode.ql | Updates to use isFromMacroExpansion() instead of isInMacroExpansion() |
rust/ql/src/queries/unusedentities/UnusedVariable.qll | Adds TODO comment noting future migration to isFromMacroExpansion() |
rust/ql/lib/utils/test/PathResolutionInlineExpectationsTest.qll | Simplifies itemAt() by removing unused inMacro parameter |
rust/ql/test/library-tests/variables/variables.ql | Updates to use isFromMacroExpansion() instead of isInMacroExpansion() |
| Test expectation files | Updates expected test outputs to reflect deduplication and new behavior |
.gitattributes and .generated.list | Removes ItemImpl.qll from generated file list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| let a: u16; | ||
| let b: u16 = 2; | ||
| set_value!(a, 1); | ||
| set_value!(a, 1); // $ Alert[rust/unused-value] |
There was a problem hiding this comment.
👍
I'm happy with all of the changes to test results in this PR, and I've made a comment in the DCA run about the results there. I haven't reviewed much of the actual QL (yet).
There was a problem hiding this comment.
Update: happy with the DCA results as well (following some investigation).
geoffw0
left a comment
There was a problem hiding this comment.
Code looks good, though its complex enough I think we're leaning on test coverage for confidence here. And test results / DCA LGTM.
Uh oh!
There was an error while loading. Please reload this page.
Follows up on #20454 and removes elements superseded by attribute macro expansions entirely from the
Elementtype. We only ever care about the expanded version, which will typically contain a copy of the original definition.DCA looks good: we reduce inconsistencies, increase call resolution rates, and eliminate
rust/unused-variablefalse positives.