-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Fix type inference for trait objects for traits with associated types #20122
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
2348657
to
466bf85
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 fixes type inference for trait objects (dyn Trait
) when traits have associated types, specifically addressing two issues: handling associated types for subtraits of supertraits with associated types, and properly handling associated types in dyn Trait
types. The fix involves extending the type parameter system to include both generic type parameters and associated type parameters for trait objects.
- Refactors the
DynTraitTypeParameter
class to handle bothTypeParam
andTypeAlias
nodes - Adds comprehensive test coverage for associated types in trait objects and supertrait scenarios
- Updates type mention resolution logic to properly handle the new type parameter structure
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
rust/ql/test/library-tests/type-inference/main.rs | Adds test module for supertrait associated types and renames existing module |
rust/ql/test/library-tests/type-inference/dyn_type.rs | Adds comprehensive tests for trait objects with associated types |
rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected | Updates line numbers in consistency test expectations |
rust/ql/lib/codeql/rust/internal/TypeMention.qll | Updates type resolution logic for dyn trait type parameters |
rust/ql/lib/codeql/rust/internal/TypeInference.qll | Extends type parameter ID generation to handle associated types |
rust/ql/lib/codeql/rust/internal/Type.qll | Major refactor of DynTraitTypeParameter to support both generic and associated type parameters |
Comments suppressed due to low confidence (1)
rust/ql/lib/codeql/rust/internal/Type.qll:438
- [nitpick] The method name
toStringInner
is ambiguous. Consider renaming it togetNodeDisplayName
orformatNodeName
to better indicate its purpose of formatting the underlying node for display.
private string toStringInner() {
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.
Looks good, couple of minor suggestions.
id = idOfTypeParameterAstNode(tp0.(DynTraitTypeParameter).getTypeParam()) | ||
id = | ||
idOfTypeParameterAstNode([ | ||
tp0.(DynTraitTypeParameter).getTypeParam().(AstNode), |
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.
.(AstNode)
appears to be redundant.
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.
This is to help QL figure out what the element type of the set is. It complains without the .(AstNode)
:)
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.
That's surprising, but fair enough.
This PR adds tests that shows two problems:
dyn Trait
type we don't handle any associated types thatTrait
might have.I stumbled into both of these issues while trying to implement type inference for closures since the traits for functions (
Fn
,FnOnce
,FnMut
) use associated types.This PR also fixes the second issue which is done by letting
dyn Trait
have matching type parameters for the associated types ofTrait
similar to what is done for the normal type parameters.