impl Trait in argument position

(Part of #34511)

As part of the latest impl Trait RFC, we decided to accept impl Trait in argument position:

fn foo(x: impl Iterator<Item = u32>) {
    ...
}

this is roughly equivalent to fn foo<T: Iterator<Item = u32>>(t: T), except that explicitly specifying the type is not permitted (i.e., foo::<u32> is an error).

Mentoring instructions can be found here.

Questions to resolve:

Should we permit specifying types if some parameters are implicit and some are explicit? e.g., fn foo<T>(x: impl Iterator<Item = T>>)? I think yes, foo::<u32> would be accepted (thus binding T = u32 explicitly).
- Answer: for now, just forbid explicit bindings altogether in the presence of impl Trait arguments.

except that explicitly specifying the type is not permitted (i.e., foo::<u32> is an error).

This is a showstopper, IMO.
Suppose you write a function in the sugary form first, but then need to make the parameter explicit for some reason. Or you want to use the sugar for some already existing code.
You can't do it, because with this rule impl Trait in args is not actually a sugar and using (or un-using) it breaks code.

As part of the latest impl Trait RFC, we decided to accept impl Trait in argument position

Am I going to expect a shift in what is considered "idiomatic" Rust by this? As in, will the new rule say that you should almost always use impl Trait? I don't really care if its an optional feature, but I don't think impl Trait is in any way better or nicer than actual generics.

@petrochenkov

@petrochenkov as a safe starting point, we can simply make it illegal to use explicit argument lists if impl Trait is used in argument position.

I'd prefer not to debate too much the "policy" of this question in this particular issue, since I think this should focus more on how to implement.

Mentoring instructions

The goal is to desugar impl Trait during HIR lowering -- or at least partially.

Basically, we will make it so that when the AST looks like this:

fn foo(x: impl Iterator) { ... }

The HIR looks like this:

fn foo<T: Iterator>(x: T) { ... }

We have to be careful though. For example, we don't want people to be able to explicitly supply type arguments (e.g., foo::<U>) for impl Iterator arguments -- or at least we don't know if we want to. So, for now, if there are any "synthetic type parameters" of this kind introduced, we want to issue an error.

First step:

Introduce the idea of a synthetic type parameter into the HIR. We could do this in a few ways. One would be to have a flag on the HIR Generics struct indicating if synthetic things were added at all. But it strikes me as mildly better to add the flag to the TyParam struct that represents a type parameter, somewhat similar to the existing pure_wrt_drop field (which is completely orthogonal). Probably the ideal would be to introduce a field synthetic of type SyntheticKind:

enum SyntheticKind { ImplTrait }

This way, if we add more kinds of synthetic type parameters in the future, we can track why they are synthetic for better error messages. =)

To allow us to write unit tests, we can add a testing attribute #[rustc_synthetic] that can be applied to type parameters. In HIR lowering, we can check for the presence of this attribute and set the flag to true, just as we do now for the pure_wrt_drop field (which is otherwise completely orthogonal from this work). (To add this attribute, you will need to add some code to feature_gate.rs, analogous to the code that exists for #[rustc_variance].) We could either add a special kind for this ("UnitTest") or just use ImplTrait. Whatever.

Next, we need to adjust the ty::TypeParameterDef struct and add a similar field. This struct is the compiler's semantic representation of the type parameters (whereas the HIR struct is meant to represent the syntax that the user wrote). As an example of how they are different, we only have HIR structs for the current crate, but we can load ty::TypeParameterDef from other crates that have already been compiled. Anyway, we want to add the same field, and initialize it from this code in collect.rs, which is the code that converts a hir::TypeParameterDef into a ty::TypeParameterDef.

OK, now, finally, we want to make it an error to specify explicit type parameters for some value (e.g., a function) if any of the type parameters on that function are synthetic. To do that, we want to modify the function that resolves paths during typeck, which is the instantiate_value_path function. I think we want to add a check that takes place after these existing checks. Here, the variables type_segment and fn_segment contain the generic type parameter declarations (if any) for the path being referenced (in the case of a path to the function foo, fn_segment would be populated and type_segment would be empty; in the case of a path like HashSet::<u32>::contains::<u32>, the type_segment would contain the generic parameter T from HashSet, and the fn_segment would contain the generic parameter Q from contains()).

What we want to do is to iterate through all the TypeParameterDefs from both segments and check if any of them are synthetic: if so, we can issue an error, using some code like:

let err = match synthetic_kind {
  hir::SyntheticKind::ImplTrait => {
    struct_span_err!(
      self.tcx.sess,
      span,
      EXXX, // we'll have to add a new error code...
      "cannot provide explicit type parameters when `impl Trait` is used in argument position")
  }
};
// bonus (described below) would go here
err.emit();

For bonus points, we could include a err.span_label() call on the error to point at where impl Trait is used in the function definition (or just the function definition itself), but that would a touch of plumbing.

With all this, we should be able to add a test like this (maybe named src/test/compile-fail/synthetic-param.rs):

fn foo<#[rustc_synthetic] T>(_: T) {
}

fn main() {
    let x = foo::<u32>; //~ ERROR cannot provide explicit type parameters
    foo(22); // ok
}

This seems like enough for a first PR!

I'm tagging this as WG-compiler-middle and WG-compiler-traits since I think it could go either way.

I'd love to try and take a shot at this.

If I understand your instructions correctly, the first PR you want doesn't include the parsing nor the conversion from fn foo(x: impl Iterator) { ... } to fn foo<T: Iterator>(x: T) { ... } right ? Just adding the notion of to the hir and ty and handling the error case ?

@chrisvittal

@chrisvittal I thought of something we should be sure to test. I'm not sure if we're just blanket disallowed impl Trait in traits and trait impls (might not be a bad idea, at least to start), but I think we want to ensure that impls kind of "match up" with their trait for now.

In other words, I don't want to accept things that sort of "reveal" the desugaring.

This seems OK:

trait Foo { 
    fn bar(&self, x: &impl Debug);
}

impl Foo for () { 
    fn bar(&self, x: &impl Debug) { }
}

This seems not OK:

trait Foo { 
    fn bar(&self, x: &impl Debug);
}

impl Foo for () { 
    fn bar<U>(&self, x: &U) { }
}

Nor this:

trait Foo { 
    fn bar<U: Debug>(&self, x: &U);
}

impl Foo for () { 
    fn bar(&self, x: &impl Debug) { }
}

Eventually we might want to accept these examples, but maybe not, it kind of hinges on this question we decided to defer earlier about how much to reveal the desugaring when doing things like foo::<X> (i.e., X specify the type for an impl Trait?).

@nikomatsakis

@nikomatsakis As of right now, I think that we accept 1, reject 2, and I'm not sure about 3. I will be sure to test all of them.

Okay, as it stands right now, all of the cases there seem to succeed. I think we need to check that for a given parameter, if it is 'synthetic' in the trait definition that it is 'synthetic' in the impl.

One more question, is the following okay.

trait Foo {
    fn bar(&self, &impl Debug);
}

impl Foo for () {
    fn bar(&self, &impl Debug + Display) { }
}

@nikomatsakis

@nikomatsakis For the examples above with the deshugaring, all of them currently compile.

I think that what I need to do, is in rustc_typeck::check::compare_method::compare_impl_method, add another check that checks the 'syntheticness' of the type parameters, and error if they don't match up.

I need some advice on properly getting all the spans for error reporting.

@chrisvittal

@chrisvittal Your example is definitely not ok (E0276), a better question is whether this is ok:

trait Foo {
    fn bar(&self, _: &impl (Debug + Display));
}

impl Foo for () {
    fn bar(&self, _: &impl Debug) { }
}

@kennytm

@kennytm As of right now your example compiles. Thanks for pointing the other error out.

@nikomatsakis

@nikomatsakis I just pushed a preliminary solution to detect matching up. It can be seen here. Thoughts?

Regarding things like impl Foo+Bar vs impl Foo, I would prefer to be very strict. I somewhat regret being somewhat loose about impls matching with methods in other areas, it just makes life more complicated later on.

If impl trait in argument position is just syntactic sugar for a generic, there's still no way to have the implementation of the function determine the type, instead of the caller. What are the plans on resolving that issue, cause otherwise impl trait in argument position seems kinda pointless (other than the ergonomic changes)?

In particular something like this should work somehow:

fn fill_futures(futures: &mut Vec<impl Future<Item = i32, Error = Error>>) { ... }

where the function is filling up a Vector of Futures, where the type of Future is determined by the function itself. This does not seem to be possible with impl Trait in argument position.

@CryZe

@CryZe yes, impl trait even in argument position shouldn't have anything to do with generics IMO, only with "this type is specified by the callee".

I think this is basically done. Closing.

Should we permit specifying types if some parameters are implicit and some are explicit? e.g., fn foo<T>(x: impl Iterator<Item = T>>)? I think yes, foo::<u32> would be accepted (thus binding T = u32 explicitly).

Answer: for now, just forbid explicit bindings altogether in the presence of impl Trait arguments.

Is there a tracking issue for allowing this case? It is counter-intuitive that this is forbidden.

Navigation Menu

Repository navigation

impl Trait in argument position #44721

Mentoring instructions

29 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

impl Trait in argument position #44721

Description

Activity

petrochenkov commented on Sep 20, 2017

est31 commented on Sep 20, 2017

nikomatsakis commented on Sep 20, 2017

nikomatsakis commented on Sep 20, 2017

nikomatsakis commented on Sep 20, 2017

Mentoring instructions

nikomatsakis commented on Sep 21, 2017

mdevlamynck commented on Sep 22, 2017

29 remaining items

nikomatsakis commented on Nov 2, 2017

chrisvittal commented on Nov 2, 2017

chrisvittal commented on Nov 5, 2017

chrisvittal commented on Nov 5, 2017

kennytm commented on Nov 5, 2017

chrisvittal commented on Nov 5, 2017

chrisvittal commented on Nov 5, 2017

nikomatsakis commented on Nov 6, 2017

CryZe commented on Nov 29, 2017

est31 commented on Nov 29, 2017

nikomatsakis commented on Feb 25, 2018

nbdd0121 commented on Jun 3, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions