Rust: Fix type inference for trait objects for traits with associated types#20122
Conversation
2348657 to
466bf85
Compare
There was a problem hiding this comment.
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
DynTraitTypeParameterclass to handle bothTypeParamandTypeAliasnodes - 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
toStringInneris ambiguous. Consider renaming it togetNodeDisplayNameorformatNodeNameto better indicate its purpose of formatting the underlying node for display.
private string toStringInner() {
geoffw0
left a comment
There was a problem hiding this comment.
Looks good, couple of minor suggestions.
| id = idOfTypeParameterAstNode(tp0.(DynTraitTypeParameter).getTypeParam()) | ||
| id = | ||
| idOfTypeParameterAstNode([ | ||
| tp0.(DynTraitTypeParameter).getTypeParam().(AstNode), |
There was a problem hiding this comment.
.(AstNode) appears to be redundant.
There was a problem hiding this comment.
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.
That's surprising, but fair enough.
This PR adds tests that shows two problems:
dyn Traittype we don't handle any associated types thatTraitmight 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 Traithave matching type parameters for the associated types ofTraitsimilar to what is done for the normal type parameters.