impl Trait in argument position

Question

impl Trait in argument position

nikomatsakis opened this issue 8 years ago · 46 comments

(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.

Answer 1 · 2017-09-20T17:23:05.000Z

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.

Answer 2 · 2017-09-20T18:11:52.000Z

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.

Answer 3 · 2017-09-20T18:37:51.000Z

@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.

Answer 4 · 2017-09-20T18:38:20.000Z

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.

Answer 5 · 2017-09-20T18:47:18.000Z

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!

Answer 6 · 2017-09-21T14:30:25.000Z

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

Answer 7 · 2017-09-22T14:32:15.000Z

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 ?

Answer 8 · 2017-09-22T14:56:06.000Z

@mdevlamynck

the first PR you want doesn't include the parsing nor the conversion

Yes, that was what I suggested. However, note that the parsing is actually already done -- we accept impl Trait in that position, but we wind up reporting an error later on. Once you've done the steps I outlined, we can transform appearances of impl Trait away when they appear in argument position, so that this error never arises.

(I actually think this will wind up looking rather similar to refactors that we are doing on our support for impl Trait in return position (which is desugared differently). In particular, we can effectively replace appearances of impl Trait in argument position with some kind of special HIR node that links directly (via the DefId) to the synthetic type parameter that we introduced.)

Answer 9 · 2017-09-22T14:56:53.000Z

@mdevlamynck btw please do reach out on Gitter, either in the room for WG-compiler-middle or the room for WG-compiler-traits if you have any pressing questions. I will try to stay on top of GH notifications, but it's harder.

Answer 10 · 2017-09-22T16:39:30.000Z

I am also interested in working on this issue, and I've started trying to tackle it. @mdevlamynck, if you would like to coordinate or take a look at anything that I've already done, my work is here.

Answer 11 · 2017-09-25T18:19:37.000Z

@mdevlamynck @chrisvittal just checking in -- have either of you had much time to hack on this?

Answer 12 · 2017-09-25T19:43:18.000Z

I have. I'm having difficulties though. My work is here.

At current, I can't get the test case you've outlined to pass. I believe this is because as written, my code does not distinguish between explicit and implicit type parameters. As such, when I check the type parameters in instantiate_value_path, I see both explicit and implicit type parameters present in fn_segment labeled as synthetic causing the test to fail.

Thanks!

Answer 13 · 2017-09-25T20:57:03.000Z

Same here, I haven't had time to investigate why the test fails though.

Answer 14 · 2017-09-26T09:38:56.000Z

I have a working version, just need to add a specific error code and I'll submit a pull request. Is there a procedure or a documentation on how to add a new error code ?

Answer 15 · 2017-09-30T15:30:34.000Z

@nikomatsakis I have some time this weekend to hack on this some more. I found the code in librustc_typeck::astconv::ast_ty_to_ty() raising an error if impl Trait is used as something else than a return type.

Do you think the conversion should be handled here?

Answer 16 · 2017-10-03T15:51:24.000Z

@mdevlamynck

Do you think the conversion should be handled here?

My expectation, I think, was that we would, during HIR lowering, no longer produce a ty::ImplTrait in that position but rather some other kind of HIR node that references the type parameter.

I am not 100% sure that this is the best route. Another possibility would be to handle this during the "ty queries" construction. In more detail, HIR lowering produces a kind of AST, but then there are various queries that produce the "semantic view" of that AST. For example, there is a hir::Generics, which represents basically "what the user wrote". The way that I was starting down involved editing that hir::Generics to insert synthetic items.

But there is a query that creates a ty::Generics from that hir::Generics, and that's what the type system internally uses. So we could leave the hir::Generics unchanged but instead scrape the types of the arguments when constructing the ty::Generics.

Answer 17 · 2017-10-04T12:56:22.000Z

@mdevlamynck

Let me try to expand out my thoughts a bit. I see two possible paths for implementing this feature. Both of them involve "desugaring", it's just a question of where we do it.

Desugaring during HIR lowering. The initial route I proposed would modify the HIR. This means that if you have an ImplTrait HIR node appearing in argument position, we would do two things:

introduce a synthetic type parameter
replace the ImplTrait HIR type with some other type X that references this parameter

Now, as we discussed on Gitter, if the user wrote the equivalent code by hand, this HIR node would be a TyPath. I was a bit wary of actually using path because paths at least used to have a lot of interconnected data structures associated with them. However, looking through the code now, I think that is no longer the case.

So, this replacement type X could be a hir::TyPath. It would be something like this:

TyPath(QPath::Resolved(None, Path {
    span: /* span of the impl trait node */,
    def: Def::TyParam(S), // see notes below about S
    segments: /* empty vector */,
}))

This is basically constructing very similar HIR to what you would get if you just wrote a reference to a type parameter manually. One difference is that the list of segments would -- in the real case -- not be empty. e.g. if the user wrote fn foo<T: Iterator>(x: T), then the type of x would be something like what I wrote above, but with a segment vector like vec!["T"] (i.e., one segment with the identifier T). Reading through the code, I don't see any real harm that will come from using an empty vector here, but it could surprise people later since it can't arise naturally (i.e., there are no "empty paths"). Might be a nice signal that this is synthesized though. =)

Let me say a bit about the S I threw in there. This would be the DefId of the synthesized type parameter. In fact, I believe that the existing code already creates a def-id for every instance of ImplTrait that appears in the AST, so we can likely just re-use that as the def-id.

Desugaring during queries. Another way to go about it would be to intercept the generics_of query. In this case, somewhere around here we would extend the set of type-parameter-defs that we create so that it not only considers the explicit generics that the user provided, but also scrapes the function signature to look for ImplTrait types and creates a TypeParameterDef for each of them.

if we take this route, then we also have to alter the code that converts a HIR ImplTrait into our internal Ty data structure. As you noted, this code currently errors out if an ImplTrait appears in the wrong place. I might consider tweaking how the logic works here a bit, actually.

I think we might want to split the HIR TyImplTrait enum variant into two variants: TyImplTraitUniversal and TyImplTraitExistential. The former would be used for the case we are hacking on here -- where impl Trait is converted into a "universally quantified" type parameter:

fn foo(impl Iterator) { .. }

// becomes:

fn foo<T: Iterator>(t: T) { // "universally quantified"
}

The latter (ImplTraitExistential) would be for fn foo() -> impl Iterator { .. }.

We would then modify HIR lowering to know what the context is and convert to the appropriate case, or issue an error if ImplTrait appears somewhere else (perhaps translating to TyErr).

Presuming we did this, then the code that converts hir::TyImplTraitUniversal would look basically convert to a Ty<'tcx> that represents a reference to a type parameter. This is done via tcx.mk_param(index, S), where index would be the index of the synthetic parameter within our generics listing. I would personally carry this index through the hir::TyImplTraitUniversal variant.

Decision time. As you can see, it feels like there is a lot of overlap between these two strategies. I'm curious whether you have an opinion which one appeals to you (also, ping @eddyb and @arielb1). At the moment I lean towards the second approach -- synthesizing HIR that the user did not type feels like it's going to be a lot of pain.

Answer 18 · 2017-10-04T14:57:16.000Z

OK, let me try to retool the mentoring instructions for the second approach in a bit more detail.

The first step would be retooling the hir::TyImplTrait into hir::TyImplTraitExistential (and later TyImplTraitUniversal). The very first step then is just to rename the existing ty::ImplTrait code into ImllTraitExistential. =)

Next, we would modify HIR lowering to carry some state indicating its context. Probably my preferred way to do this would be to add a (&TypeLoweringContext) parameter to lower_ty(). This can be as simple as an enum enumerating the different places types can appear (which in turn implies how to treat impl Trait):

#[derive(Copy, Clone, Debug)]
enum TypeLoweringContext {
    FnParameter(DefId), // the DefId here is the def-id of the function to which this is a parameter
    FnReturnType(DefId), // here too
    Other
}

impl TypeLoweringContext {
    fn impl_trait_treatment(self) -> ImplTraitTreatment {
        use self::TypeLoweringContext::*;
        use self::ImplTraitTreatment::*;

        match self {
            FnParameter(_) => Universal,
            FnReturnType(_) => Existential,
            Other => Disallowed,
        }
    }
}

enum ImplTraitTreatment {
    /// Treat `impl Trait` as shorthand for a new universal generic parameter.
    /// Example: `fn foo(x: impl Debug)`, where `impl Debug` is conceptually
    /// equivalent to a fresh universal parameter like `fn foo<T: Debug>(x: T)`.
    Universal,
    
    /// Treat `impl Trait` as shorthand for a new universal existential parameter.
    /// Example: `fn foo() -> impl Debug`, where `impl Debug` is conceptually
    /// equivalent to a fresh existential parameter like `abstract type T; fn foo() -> T`.
    Existential,

    /// `impl Trait` is not accepted in this position.
    Disallowed,
}

Once we have that, we can alter the code that lowers an ImplTrait in the AST depending on the context:

TyKind::ImplTrait(ref bounds) => {
    match context.impl_trait_treatment() {
        ImplTraitTreatment::Existential => hir::TyImplTrait(self.lower_bounds(bounds)),
        ImplTraitTreatment::Universal | ImplTraitTreatment::Disallowed => {
            // For now, treat Universal as the same as disallowed since it's not done yet.
            span_err!(tcx.sess, ast_ty.span, E0562,
                              "`impl Trait` not allowed outside of function \
                               and inherent method return types");
            hir::TyErr
        }
    }
}

We can now adjust the astconv code to remove all this logic, since that is handled by the lowering code, so all we need is the "allowed" case.

The final step is to introduce hir::TyTraitUniversal. We would add the new variant into the hir::Ty_ enum and modify the HIR lowering code to produce it as appropriate. I think this variant should include the DefId of the enclosing function:

TyTraitUniversal(DefId, TyParamBounds)

Simultaneously, we would modify the generics_of predicate as I roughly described here. This means that we want to create a TypeParameterDef in this loop. The idea roughly would be to extract the argument types at the same time as we extract the HIR Generics declaration -- particularly in the case of trait-item methods and the case of fn items. This ultimately means storing a Option<&P<hir::Ty>>, which is found in the inputs field of FnDecl, which in turn can be reached either from MethodSig for method trait items or directly for fn items.

Once we have the list of inputs, we can generate the new, synthetic TypeParameterDef instances by visiting them using a HIR visitor -- we would just override visit_ty to scrape up the list of hir::TyImplTraitUniversal instances that appear. Then, for each one, we create a TypeParameterDef kind of like this:

fn visit_ty(&mut self, ty: &hir::Ty) {
    if let hir::TyTraitUniversal(..) = ty.node {
        self.implicit_defs.push(
            ty::TypeParameterDef {
                index: /* compute next index -- this should come after the generics the user wrote, presumably */,
                name: /* um what should we put here? */,
                def_id: tcx.hir.local_def_id(ty.id), // <-- use def-id we created for the `impl Trait` instance
                has_default: false,
                object_lifetime_default: rl::Set1::Empty,
                pure_wrt_drop: false,
                synthetic: Some(...), // <-- this is synthetic, naturally
        });
    }

    intravisit::walk_ty(ty);
}

where implicit_defs is some vector in the visitor that we are accumulating into.

We also have to modify predicates_of to include the implicit predicates, which would be scraped up in a similar fashion.

Finally, we have to modify the ast_ty_to_ty code to add a case for TyTraitUniversal:

TyImplTraitUniversal(fn_def_id, _) => {
    let impl_trait_def_id = self.tcx.hir.local_def_id(ast_ty.id);
    let generics = self.tcx.generics_of(fn_def_id);
    let index = /* find the index of `impl_trait_def_id` in `generics`, see LINK1 below */;
    self.tcx.mk_param(index, impl_trait_def_id)
}

LINK1

At this point, I think that basic examples should be working. For example:

fn any_zero(values: impl IntoIterator<Item=u32>) -> bool {
    for v in values { if v == 0 { return true; } }
    false
}

What will not work is one corner case having to do with lifetimes. In particular, I would expect this to ICE or otherwise misbehave:

fn any_zero<'a>(values: impl IntoIterator<Item=&'a u32>) -> bool {
    for v in values { if *v == 0 { return true; } }
    false
}

Actual final step: to fix that last case, we have to patch up early- vs late-bound lifetimes (some explanation on this concept -- and links to blog posts -- can be found here). This involves modifying the visit_early_late code in resolve_lifetimes -- or, more precisely, the insert_late_bound function. We want to extend the appears_in_where_clause case so that we consider lifetimes which appear in an ImplTraitUniversal to be part of a where-clause. Right now we just scrape the generic bounds (e.g., fn foo<T: Debug>) and the where clauses, but we need to also walk the inputs and look for ImplTraitUniversal types.

Answer 19 · 2017-10-04T19:46:05.000Z

synthesizing HIR that the user did not type feels like it's going to be a lot of pain

That's actually the point of HIR - that you can perform desugarings, including creating paths. I don't believe there is any conceptual trouble creating a segment-free path, but if e.g. privacy complains, it should be possible to find a way to shut it up.

Answer 20 · 2017-10-09T21:17:32.000Z

I may not have time to work on this in the next few days. In the mean time if anyone wants to submit a PR before I can, feel free! @chrisvittal maybe you're still interested?

Answer 21 · 2017-10-09T21:28:18.000Z

@mdevlamynck I'll take a look. I may not have time either, but if I get anywhere I'll give a pointer to my work here.

Answer 22 · 2017-10-10T20:35:01.000Z

@arielb1

That's actually the point of HIR - that you can perform desugarings, including creating paths. I don't believe there is any conceptual trouble creating a segment-free path, but if e.g. privacy complains, it should be possible to find a way to shut it up.

I've been going back and forth on this, for sure. The exact line between what should be desugaring into HIR vs into semantic types (or both) is sort of unclear to me.

I think in the end I'm up for pursuing whatever direction here and trying to judge from experience how well it goes. =)

Answer 23 · 2017-10-17T19:59:36.000Z

@chrisvittal @mdevlamynck did either of you wind up with any time to take a look? I'm trying to decide if I should be hunting for someone else to take on this task. =)

Answer 24 · 2017-10-17T20:05:04.000Z

@nikomatsakis I have not had any chance to look at it. The mentoring instructions seem straightforward. I'll try to take a look tonight. If I get anywhere, I'll let you know here.

Answer 25 · 2017-10-17T20:08:47.000Z

@nikomatsakis I may have some time this weekend. And as I said, I really don't mind if someone else is taking over :)

Answer 26 · 2017-10-20T03:38:56.000Z

I've started on this. @nikomatsakis, You mention that we can possibly add a TypeLoweringContext as an argument to lower_ty(), I'm having trouble knowing what values for a TypeLoweringContext to put into all the calls to lower_ty, as well as figuring out which other functions need a TypeLoweringContext and which ones don't.

For example lower_item_kind calls lower_ty for many different ItemKinds, what should the TypeLoweringContext argument for this be?

Thanks!

Answer 27 · 2017-10-20T23:11:26.000Z

@chrisvittal

Presuming you added the enum that I suggested, basically everything would be TypeLoweringContext::Other except for:

the recursive cases (like this one and this one), which would propagate the context
the inputs and outputs to a function -- i.e., here and here, which would use TypeLoweringContext::FnParameter(def_id) and TypeLoweringContext::FnReturnType(def_id) respectively
- only problem is that the def_id would have to be passed in
- one exception is when it is invoked as part of a fn() type, in which case you want TypeLoweringContext::Other
- so maybe pass in a opt_def_id: Option<DefId> and do something like

let context = opt_def_id.map(|def_id| TypeLoweringContext::FnParameter(def_id))
    .unwrap_or(TypeLoweringContext::Other);

Basically the rule is:

the context should be Other unless impl trait is allowed in that position =)

Answer 28 · 2017-10-23T05:29:43.000Z

@nikomatsakis My WIP branch is here.

Notes on my current implementation:

I'm implementing it almost exactly as suggested here. I've named what's we've called TypeLoweringContext, TyLoweringCtx mostly for brevity. It's not a hard change to to switch from one to the other. For the FnReturnType variant, I'm not storing a DefId I don't think it will be that hard to add it if it is necessary.

Here are the questions I have now.

How do I obtain DefId values for populating FnParameter(DefId)? My current "solution" causes an ICE when compling stage 1. below is what I'm doing now.

            inputs: decl.inputs.iter()
                .map(|arg| {
                    // FIXME causes ICE compliling stage 1, need to find a better solution
                    let def_id = self.resolver.definitions().local_def_id(arg.id);
                    self.lower_ty(&arg.ty, TyLoweringCtx::FnParameter(def_id))
                }).collect(),

If I were to create an HIR Visitor, how would I go about using it? What is the point of collecting the input argument types it we are going to also obtain the information from a visitor? I guess I'm not really understanding something here.

Answer 29 · 2017-10-23T20:44:37.000Z

@chrisvittal

How do I obtain DefId values for populating FnParameter(DefId)?

One thing that may not have been obvious: I meant for that to be the DefId of the enclosing function, not the parameter. To be honest, from skimming my write-up, I forget why I thought that would be useful, but I suspect I was right (maybe for error reporting?). But if we don't need it, things would be easier.

Presuming we do need it, though, lower_fn_decl doesn't -- I don't think -- have quite enough information to get it. I think we'll need to add a parameter. You might think this could just be something like fn_def_id: DefId, but that doesn't quite work, because at least some function declarations don't want to permit impl Trait in their arguments (in particular, the function decls for things like fn(impl Debug), which are explicit disallowed by the RFC). This is why in my previous comment I talked about passing in an Option<DefId> -- with None meaning "no impl trait here". I think we would want to pass:

Some(tcx.local_def_id(id)) here, which would be the def-id of the fn item
Some(X) here, where X is the def-id of the method (will have to be passed in)
None here
- this is for a closure arguments etc, like |x: impl Trait| or || -> impl Trait { .. }, which I don't think we want to deal with right now =)
None here
- this is for fn() types

Also, I'm beginning to question the idea of calling this the TypeLoweringContext. Maybe that's too generic, and it'd be better to just call it the "impl trait context" or something. (Before I had the idea that the "type lowering context" just told you where the type appeared, and from that you could figure out whether impl trait was allowed there -- the idea was that maybe later we'll want to use this for other things too besides impl trait. But I fear that trying to be more generic makes things more confusing in the end.)

It might also be easier to use a field than threading things around as parameters, though I think that can often be more confusing overall.

Finally, I think I made a small mistake before, in that I think we want to be sure we permit impl trait in path parameters -- e.g., something like fn foo(x: Vec<impl Debug>) ought to work (also in return position). That implies that this call to lower_type that occurs in lower_angle_bracketed_parameter_data needs to permit impl traits, whereas you currently have Other.

However, the calls in lower_parenthesized_parameter_data for lowering arguments and return types both want to be Other (you have FnReturnTy for one of them, at least). This is because the RFC disallowed Fn(impl Trait) and Fn() -> impl Trait.

Just to help, here is a series of tests and the ImplTraitTreatment I expect for each:

fn foo(x: impl Debug) => Universal
fn foo() -> impl Debug => Existential
fn foo(x: Vec<impl Debug>) => Universal
fn foo() -> Vec<impl Debug> => Existential
fn foo(x: fn(impl Debug)) => Disallowed (fn type)
fn foo() -> fn(impl Debug) => Disallowed (fn type)
fn foo(x: &dyn Iterator<Item = impl Debug>) => Universal
fn foo(x: &dyn Fn(impl Debug)) => Disallowed (fn trait sugar)
fn foo(x: impl Iterator<Item = impl Debug>) => Universal, Universal
fn foo() -> impl Iterator<Item = impl Debug> => Existential, Existential
fn foo(x: impl Fn(impl Debug)) => Universal, Disallowed (fn trait sugar)
struct Foo { x: impl Debug } => Disallowed (struct)
trait Foo { fn foo(x: impl Debug) } => Universal, but maybe we disallow for now -- gets complicated
trait Foo { fn foo() -> impl Debug; } => Disallowed (trait method return type)
everything else would be illegal for now

All the cases for fn above are the same for methods appearing in inherent impls. Trait impls are a bit different. I say we leave those aside for now and revisit them as a separate issue once we get the rest working, because they introduce some complications.

Answer 30 · 2017-10-28T15:44:57.000Z

@nikomatsakis I'm very close to being done with this. I have both the simple and the complex examples from the instructions working.

I'm currently having trouble with compile-fail/impl-trait/disallowed.rs

I'm getting too many errors, and I'm not seeing one error that I should be. The one I'm most concerned about is the last not found error, which corresponds to the where clause item here

Where does the where clause get lowered in this example? I need to find it to create the impl Trait not allowed error.

Another question, the type parameter unused errors are occuring due to this . What are some possible ways to error out earlier so I don't get those messages?

Thanks.

Here are the messages from the failing test

unexpected errors (from JSON output): [
    Error {
        line_num: 19,
        kind: Some(
            Error
        ),
        msg: "19:14: 19:15: type parameter `R` is unused [E0091]"
    },
    Error {
        line_num: 22,
        kind: Some(
            Error
        ),
        msg: "22:20: 22:21: type parameter `R` is unused [E0091]"
    },
    Error {
        line_num: 31,
        kind: Some(
            Error
        ),
        msg: "31:40: 31:59: method `lazy_to_string` has an incompatible type for trait [E0053]"
    }
]

not found errors (from test file): [
    Error {
        line_num: 14,
        kind: Some(
            Error
        ),
        msg: "`impl Trait` not allowed outside of function and inherent method return types"
    },
    Error {
        line_num: 16,
        kind: Some(
            Error
        ),
        msg: "`impl Trait` not allowed outside of function and inherent method return types"
    },
    Error {
        line_num: 52,
        kind: Some(
            Error
        ),
        msg: "`impl Trait` not allowed outside of function and inherent method return types"
    }
]

Answer 31 · 2017-10-30T02:32:18.000Z

I think that I've figured out that where clauses get lowered, in lower_where_clause. =)

Update: So it turns out that the error missing from line 52, has to do with this call to lower_path_segment. Unfortunately, if this is changed, it breaks inference in multiple places and ways. Notably, the following two cases from your list above.

> fn foo(x: Vec<impl Debug>) => Universal
> fn foo() -> Vec<impl Debug> => Existential

I'm going to try to thread some parameter into lower_qpath and see if I can properly filter the cases.

Update 2: I think the easiest way to take care of this may be to do some kind of query much like the one in the old ast_ty_to_ty code. In either lower_path_segment or lower_qpath. Thoughts?

Answer 32 · 2017-10-30T17:04:25.000Z

@chrisvittal I think that lower_qpath simply doesn't have enough context to determine the impl trait context. I would expect its callers to be telling it what to do with impl Trait usages that appear in lower_qpath.

Answer 33 · 2017-10-30T17:11:53.000Z

Example callers:

lower_pat -- no impl traits use is presently allowed
- this would correspond to something like match foo { Some::<impl Trait>(x) => { ... }
lower_expr (here and here)-- no impl trait is presently allowed. This would correspond to something like let x = foo::<impl Trait>(x);
lower_ty -- propagate itctx
lower_trait_ref and (in turn) lower_poly_trait_ref -- this should take a itctxt parameter of its own and propagate the result, I believe
- this call and this call would correspond to impl Trait<impl Debug>, and hence can pass that impl trait is disallowed (though it would (admittedly) be nice to support eventually, but I think is out of scope for the present PR)
- lowering trait object types -- this would propagate the itctxt result from the surrounding context
- lowering ty param bounds would be disallowed for now, that corresponds to fn foo<T: Bar<impl Trait>>, which I think we are not aiming to support in this PR

Answer 34 · 2017-11-02T04:15:22.000Z

I've just pushed the latest WIP here.

I've figured out that the first offending call that then propagates a Disallowed is in lower_mt.

This is the file I've been using to test:

#![allow(unused)]
#![feature(conservative_impl_trait,dyn_trait)]
use std::fmt::Debug;
fn dyn_universal(_: &dyn Iterator<Item = impl Debug>) { panic!() }

fn main() {}

Answer 35 · 2017-11-02T09:33:34.000Z

@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?).

Answer 36 · 2017-11-02T18:40:29.000Z

@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.

Answer 37 · 2017-11-05T05:33:27.000Z

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) { }
}

Answer 38 · 2017-11-05T17:45:07.000Z

@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.

Answer 39 · 2017-11-05T18:31:05.000Z

@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) { }
}

Answer 40 · 2017-11-05T18:42:08.000Z

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

Answer 41 · 2017-11-05T21:25:56.000Z

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

Answer 42 · 2017-11-06T15:42:01.000Z

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.

Answer 43 · 2017-11-29T02:28:40.000Z

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.

Answer 44 · 2017-11-29T02:37:15.000Z

@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".

Answer 45 · 2018-02-25T22:05:45.000Z

I think this is basically done. Closing.

Answer 46 · 2020-06-03T00:44:48.000Z

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.