Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

paldepind
Copy link
Contributor

@paldepind paldepind commented Jul 24, 2025

We currently can't handle impl types with type parameters inside them, such as this example where T occurs within the impl:

fn get<T: Clone>(x: T) -> impl MyTrait<T> {
    S3(x)
}

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 the impl type.

In the above example get has a type parameter T which occurs in the impl in return position. Hence a type parameter corresponding to T is added to the impl type. When get is called an a specific type is known for T then that type will be instantiated inside the returned impl 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 type Foo and Bar. That is a perfectly fine implementation, but I believe this approach has two advantages:

  1. We don't loose the information that the type is an impl type.
  2. We don't infer multiple types for the same term. Right now that occurs only due to inaccuracies, and I think it's nicer to keep it that way. This will make it easier to figure out where we have type blowup.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jul 24, 2025
@paldepind paldepind marked this pull request as ready for review July 24, 2025 14:05
@Copilot Copilot AI review requested due to automatic review settings July 24, 2025 14:05
@paldepind paldepind requested a review from a team as a code owner July 24, 2025 14:05
Copy link
Contributor

@Copilot Copilot AI left a 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 creating ImplTraitTypeParameter 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

@paldepind paldepind added the no-change-note-required This PR does not need a change note label Jul 24, 2025
Copy link
Contributor

@geoffw0 geoffw0 left a 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
Copy link
Contributor

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???

Copy link
Contributor Author

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?

Copy link
Contributor

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() }
Copy link
Contributor

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???

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants