Rust: Take prelude into account when resolving paths #19157
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hvitved
force-pushed
the
rust/path-resolution-prelude
branch
7 times, most recently
from
f5a7f97 to
905ee47
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances path resolution in Rust by incorporating the standard library prelude and improved handling of $crate macro paths.
- Introduces additional test cases in a new module (m21) that exercise the resolution of unit variant enums and unit structs.
- Adjusts usage patterns in the test code to reflect QL resolution expectations regarding the Rust prelude and macro expansion.
Files not reviewed (19)
- rust/ql/integration-tests/hello-project/summary.expected: Language not supported
- rust/ql/integration-tests/hello-workspace/summary.cargo.expected: Language not supported
- rust/ql/integration-tests/hello-workspace/summary.rust-project.expected: Language not supported
- rust/ql/lib/codeql/rust/dataflow/internal/Content.qll: Language not supported
- rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/elements/internal/EnumImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/elements/internal/PathImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll: Language not supported
- rust/ql/lib/codeql/rust/internal/CachedStages.qll: Language not supported
- rust/ql/lib/codeql/rust/internal/PathResolution.qll: Language not supported
- rust/ql/lib/codeql/rust/internal/PathResolutionConsistency.qll: Language not supported
- rust/ql/lib/codeql/rust/internal/Type.qll: Language not supported
- rust/ql/lib/codeql/rust/internal/TypeInference.qll: Language not supported
- rust/ql/test/extractor-tests/canonical_path/CONSISTENCY/PathResolutionConsistency.expected: Language not supported
- rust/ql/test/extractor-tests/canonical_path_disabled/CONSISTENCY/PathResolutionConsistency.expected: Language not supported
- rust/ql/test/library-tests/path-resolution/CONSISTENCY/PathResolutionConsistency.expected: Language not supported
- rust/ql/test/library-tests/path-resolution/path-resolution.expected: Language not supported
- rust/ql/test/library-tests/path-resolution/path-resolution.ql: Language not supported
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
|
|
||
| fn f() { | ||
| let _ = MyEnum::A; // $ item=I104 | ||
| let _ = MyStruct {}; // $ item=I106 |
There was a problem hiding this comment.
MyStruct is defined as a unit struct; therefore, it should be instantiated without braces. Please update the instantiation to 'let _ = MyStruct;'.
Suggested change
| let _ = MyStruct {}; // $ item=I106 | |
| let _ = MyStruct; // $ item=I106 |
paldepind
reviewed
Apr 2, 2025
hvitved
force-pushed
the
rust/path-resolution-prelude
branch
from
905ee47 to
02e9795
Compare
hvitved
force-pushed
the
rust/path-resolution-prelude
branch
from
02e9795 to
f4e9382
Compare
paldepind
requested changes
Apr 4, 2025
Contributor
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
Looks great. I only have one small comment and a question.
|
|
||
| /** Gets a successor named `name` of this item, if any. */ | ||
| pragma[nomagic] | ||
| cached |
Contributor
There was a problem hiding this comment.
Did something change in this PR that means getASuccessor should now be cached? Or would it already have made sense to cache it prior to this PR?
Contributor
Author
There was a problem hiding this comment.
I think it already made sense prior to this PR since the type inference stage relies on the predicate.
Co-authored-by: Simon Friis Vindum <paldepind@github.com>
paldepind
approved these changes
Apr 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR started out with me wanting to get rid of the
VariantInLibworkaround, which was needed prior to extracting the crate graph.Removing
VariantLibmeans we will be using our QL logic for resolvingoption::Optionandresult::Resultpaths, instead of relying ongetExtendedCanonicalPathprovided by the extractor. However, in order to resolve those paths, we need to take the standard library prelude into account, which is exactly what this PR does.Since we don't know which edition of Rust is being used, we simply include all the editions; 2015, 2018, 2021, and 2024.
Removing
VariantLibalso revealed that we need to handle$cratepaths inside expanded macros. For now, we over-approximate resolution of$cratepaths by considering all transitive dependencies.DCA shows that, while we mostly maintain performance, we gain an additional 10% true positive call edges (up 475,373 from 430,130) and an additional 14% true positive resolved paths (up 368,369 from 324,251), all numbers computed across the entire DCA suite.