Rust: Visibility check for qualified path resolution #20496
Conversation
hvitved
force-pushed
the
rust/path-resolution-use-visibility
branch
2 times, most recently
from
9f931be to
e3b3e7c
Compare
hvitved
force-pushed
the
rust/path-resolution-use-visibility
branch
2 times, most recently
from
205c3a2 to
bfcda3b
Compare
```
Evaluated relational algebra for predicate _PathResolution::CrateItemNode.getName/0#dispred#91b4dd6b_PathResolution::SourceFileItemNode#bd8f490__#antijoin_rhs@e84aee8k with tuple counts:
35406180 ~0% {3} r1 = JOIN PathResolution::SourceFileItemNode#bd8f4905 WITH `PathResolution::CrateItemNode.getName/0#dispred#91b4dd6b` CARTESIAN PRODUCT OUTPUT Lhs.0, Rhs.1, Rhs.0
8455 ~2% {4} | JOIN WITH `PathResolution::declaresDirectly/3#7d0350fb_021#join_rhs` ON FIRST 2 OUTPUT Rhs.2, Lhs.0, Lhs.2, Lhs.1
3259 ~0% {3} | JOIN WITH num#PathResolution::TTypeNamespace#4897e416 ON FIRST 1 OUTPUT Lhs.1, Lhs.2, Lhs.3
return r1
```
hvitved
force-pushed
the
rust/path-resolution-use-visibility
branch
from
bfcda3b to
76ca5f8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
hvitved
force-pushed
the
rust/path-resolution-use-visibility
branch
from
76ca5f8 to
78641b4
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves the visibility checking for qualified path resolution in Rust code analysis by making the visibility checks more precise. The main purpose is to refactor path resolution to better track which items are visible based on their declaration scope and any use statements that bring them into scope.
Key changes:
- Added tracking of
usestatements that bring items into scope through a newUseOptiontype - Updated visibility checking to consider whether items or the
usestatements that import them are in ancestor modules - Modified various path resolution predicates to pass through and check visibility information more accurately
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/PathResolution.qll | Core implementation of the visibility checking improvements with new UseOption type and updated resolution predicates |
| rust/ql/lib/codeql/rust/internal/CachedStages.qll | Updated cached stage predicate to match the new getASuccessor signature |
| rust/ql/lib/utils/test/PathResolutionInlineExpectationsTest.qll | Enhanced test utility to handle SourceFile items differently in expectations |
| rust/ql/test/library-tests/path-resolution/my2/mod.rs | Added test case with std::ops::Deref import |
| rust/ql/test/library-tests/path-resolution/my2/my3/mod.rs | Added test cases for glob imports and trait bounds |
| rust/ql/test/library-tests/path-resolution/path-resolution.expected | Updated test expectations to reflect the improved path resolution |
| use super::*; // $ item=mod.rs | ||
|
|
||
| trait MyTrait: Deref {} // $ MISSING: item=Deref | ||
| trait MyTrait: Deref {} // $ item=Deref |
There was a problem hiding this comment.
I'm a bit nervous that we appear to have only one (explicit) test case for this fairly complicated change. Are there any edge cases we ought to be testing as well?
There was a problem hiding this comment.
We also have some tests from #20453, but they didn't cover the case where an item from another crate was made available via a private use statement, which the test case above does.
There was a problem hiding this comment.
I'm happy with this.
@paldepind might be a good idea for you to have a quick look over the changes as well at some point.
Follows up on #20453 and makes the visibility check for qualified path resolution more precise.
Commit-by-commit review is encouraged.
DCA looks good;
0.585 %point increase in resolvable calls.