-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Support blanket implementations #20133
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
base: main
Are you sure you want to change the base?
Conversation
983257d
to
2e98c24
Compare
2e98c24
to
f130198
Compare
f130198
to
3b9f683
Compare
bea1e3b
to
8fa4d0d
Compare
8fa4d0d
to
0efa72e
Compare
0314813
to
8c91ef0
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 implements method resolution for blanket implementations in Rust type inference, specifically targeting blanket implementations where the type parameter has trait bounds. The implementation enables CodeQL to better resolve method calls that depend on blanket trait implementations.
Key changes:
- Added support for blanket implementation method resolution with trait bounds
- Enhanced type inference to handle universally quantified type parameters in constraints
- Updated test cases to reflect improved method resolution capabilities
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
shared/typeinference/codeql/typeinference/internal/TypeInference.qll | Added useUniversalConditions predicate to control constraint satisfaction for universally quantified type parameters |
rust/ql/lib/codeql/rust/internal/TypeInference.qll | Core implementation of blanket implementation support including detection, canonicalization, and method resolution |
rust/ql/lib/codeql/rust/internal/PathResolution.qll | Added resolveBound method to resolve type parameter bounds by index |
rust/ql/test/library-tests/type-inference/blanket_impl.rs | New test file demonstrating blanket implementation scenarios |
Multiple test expected files | Updated to reflect improved method resolution results from blanket implementation support |
not trait.getName().getText() = | ||
[ | ||
"Sized", "Clone", | ||
// The auto traits | ||
"Send", "Sync", "Unpin", "UnwindSafe", "RefUnwindSafe" | ||
] |
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.
The hardcoded list of excluded trait names should be extracted to a constant or configuration to improve maintainability and make it easier to extend.
Copilot uses AI. Check for mistakes.
* We detect these duplicates based on some simple heuristics (same trait | ||
* name, file name, etc.). For these duplicates we select the one with the | ||
* greatest file name (which usually is also the one with the greatest library | ||
* version in the path) |
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.
The heuristic for selecting the 'canonical' implementation based on greatest file name seems fragile and may not always correspond to the greatest library version. Consider documenting the limitations of this approach or exploring more robust versioning detection.
* version in the path) | |
* version in the path). **Note:** This heuristic is fragile and may not always | |
* correspond to the greatest library version, as file names may not reliably | |
* encode version information and lexicographical order does not match semantic | |
* versioning. Users should be aware of this limitation and consider more robust | |
* version detection if precise canonicalization is required. |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
This PR implements method resolution for blanket implementations that satisfies the criteria: the type parameter that the implementation targets has a trait bound.
For instance, this
impl
blockis handled as there is a trait bound
AsyncRead
on the type parameterR
.On the other hand, this
impl
blockis not handled since there is not trait bound for
T
.At a high level, the implementation works as follows:
foo.bar
check if there's a blanket implementation that has the methodbar
.foo
satisfies the trait bound.foo.bar
to the method from the blanket implementation.If there are multiple trait bounds on the type parameter, then only one is checked to be satisfied. This could give false results, but from looking at some results in doesn't look too bad after filtering away some trivial traits (
Clone
,Send
, etc.).The DCA report seems great. A 0.819% point increase in resolved calls with little to no change to performance. Some inconsistencies are up, but a good chunk of that is probably explained by more type information in general.