-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Type inference for impl trait types with type parameters #20119
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?
Rust: Type inference for impl trait types with type parameters #20119
Conversation
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 type inference for impl
trait types that contain type parameters from their enclosing function. The implementation allows proper handling of return position impl
types that reference function type parameters, such as fn get<T: Clone>(x: T) -> impl MyTrait<T>
.
Key changes:
- Adds support for type parameters within
impl
trait types by creatingImplTraitTypeParameter
objects - Updates type inference logic to properly handle and rank these new type parameters
- Extends test coverage with various scenarios of parameterized
impl
trait return types
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
main.rs | Adds comprehensive test cases for impl trait types with type parameters |
PathResolutionConsistency.expected | Updates line number references due to test additions |
TypeMention.qll | Implements type resolution for ImplTraitTypeParameter |
TypeInference.qll | Extends type parameter ranking system to handle ImplTraitTypeParameter |
Type.qll | Core implementation of ImplTraitTypeParameter class and related logic |
ImplTraitTypeReprImpl.qll | Adds getFunctionReturnPos() method to locate containing function |
.gitattributes & .generated.list | Removes generated file markers for ImplTraitTypeReprImpl.qll |
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.
Mostly questions to aid my own understanding...
let e = get_a_my_trait2(S1).get_a(); // $ target=get_a_my_trait2 target=MyTrait::get_a type=e:S1 | ||
// For this function the `impl` type does not appear in the root of the return type | ||
let f = get_a_my_trait3(S1).unwrap().get_a(); // $ target=get_a_my_trait3 target=unwrap target=MyTrait::get_a type=f:S1 | ||
let g = get_a_my_trait4(S1).0.get_a(); // $ target=get_a_my_trait4 target=MyTrait::get_a type=g:S1 |
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.
I'm interested in what happens when we have nested impl
types, for example I've seen:
impl Iterator<Item = (impl Into<String>, Resource)>
I haven't tested anything but it feels like there's a danger the type parameter String
could become associated with both impl
types on this line due to the way implTraitTypeParam
is written? Is it even wrong if it does???
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 a good question that I haven't thought of.
With how it works right now they would become type parameters of both the impl
types. Whether that's right or wrong I'm not sure about. My guess is that it's more correct to only create a type parameter of the inner impl
. I can try and write some tests for that in a follow up PR and take a look?
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.
In a follow-up PR is fine, if you're keeping track. :)
* Gets the function for which this impl trait type occurs in the return | ||
* type, if any. | ||
*/ | ||
Function getFunctionReturnPos() { this.getParentNode*() = result.getRetType().getTypeRepr() } |
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.
I'd like us to come up with a better name for this predicate - in particular the pattern get...Pos
makes my brain expect this to return a position / index, which it doesn't. On the other hand "return position" is established phrasing so we don't want to lose that. Perhaps just re-order it to getReturnPosFunction
???
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.
What about getReturnPosOfFunction
? We could also split this into two predicates getFunction
and isReturnPos
. Both of those seem clear, but we don't otherwise have a need for the general getFunction
that also works for impl
in arguments.
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.
From a readability point of view I quite like the two predicates solution. Assuming it doesn't make the code much more messy elsewhere.
We currently can't handle
impl
types with type parameters inside them, such as this example whereT
occurs within theimpl
:The example is from the draft PR: #19954
This PR fixes the problem by letting type parameters of the function where the return position
impl
occurs also induce a type parameter of theimpl
type.In the above example
get
has a type parameterT
which occurs in theimpl
in return position. Hence a type parameter corresponding toT
is added to theimpl
type. Whenget
is called an a specific type is known forT
then that type will be instantiated inside the returnedimpl
type.This implementation is a bit different from the draft one in #19954. That implementation relies on inferring trait types directly. I.e. elements of
impl Foo + Bar
will have both the typeFoo
andBar
. That is a perfectly fine implementation, but I believe this approach has two advantages:impl
type.