-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Handle unqualified UseTrees in path resolution
#20744
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
Rust: Handle unqualified UseTrees in path resolution
#20744
Conversation
168b66d to
0bfc7cf
Compare
0bfc7cf to
50552da
Compare
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 enhances path resolution for Rust use statements to support complex nested use tree patterns. Specifically, it handles cases like use {{{my::{{self as my_alias, *}}}}} where braces are deeply nested without intervening path segments.
- Introduces
getAUseTreeUseTree()to recursively traverse nested use trees that lack path segments - Updates path resolution logic to handle nested glob imports and aliases within brace groups
- Adds test coverage for the new nested use tree pattern with
my_alias::nested_f()
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Core implementation adding getAUseTreeUseTree() and getAUseUseTree() helpers, and updating resolution logic to handle nested use trees without paths |
| rust/ql/test/library-tests/path-resolution/main.rs | Test case demonstrating nested braces with alias (use {{{my::{{self as my_alias, *}}}}}) and its usage |
| rust/ql/test/library-tests/path-resolution/path-resolution.expected | Updated expected output with new path resolutions and shifted line numbers |
| rust/ql/test/library-tests/path-resolution/CONSISTENCY/PathResolutionConsistency.expected | Updated line numbers for consistency checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
paldepind
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.
LGTM! One question that might need addressing, but I've improved in case it's intended.
| exists(UseTree root | root = use.getUseTree() | | ||
| result = root |
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.
Is the overlap in the disjunction here intended? If not we could do if root.hasPath() then ... else ....
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.
Yeah, we may as well; this predicate is only called in one place, and it expects the result to have a path.
Previously, we assumed that use trees
{ ... }always had a qualifier, but this need not be the case.DCA looks good; we gain an additional 0.4 %-point call edges, although at the cost of a slowdown on
rust(which happens because we compute more data flow, for example the predicateAccessAfterLifetime::AccessAfterLifetimeFlow::Flow::Stage3::fwdFlowThroughgrows from3,626,391,847tuples to4,361,916,386tuples.).