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

impl Trait in argument position #44721

Closed
nikomatsakis opened this Issue Sep 20, 2017 · 45 comments

Comments

Projects
None yet
10 participants
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Sep 20, 2017

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

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 20, 2017

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.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Sep 20, 2017

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.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Sep 20, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Sep 20, 2017

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.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Sep 20, 2017

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!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Sep 21, 2017

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

@mdevlamynck

This comment has been minimized.

Copy link
Contributor

mdevlamynck commented Sep 22, 2017

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 ?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Sep 22, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Sep 22, 2017

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

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Sep 22, 2017

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.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Sep 25, 2017

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

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Sep 25, 2017

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!

@mdevlamynck

This comment has been minimized.

Copy link
Contributor

mdevlamynck commented Sep 25, 2017

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

@mdevlamynck

This comment has been minimized.

Copy link
Contributor

mdevlamynck commented Sep 26, 2017

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 ?

bors added a commit that referenced this issue Sep 29, 2017

Auto merge of #44866 - mdevlamynck:impl-trait, r=eddyb
First step toward implementing impl Trait in argument position

First step implementing #44721.

Add a flag to hir and ty TypeParameterDef and raise an error when using
explicit type parameters when calling a function using impl Trait in
argument position.

I don't know if there is a procedure to add an error code so I just took an available code. Is that ok ?

r? @nikomatsakis
@mdevlamynck

This comment has been minimized.

Copy link
Contributor

mdevlamynck commented Sep 30, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 3, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 4, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 4, 2017

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.

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Oct 9, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 10, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 17, 2017

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

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Oct 17, 2017

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

@mdevlamynck

This comment has been minimized.

Copy link
Contributor

mdevlamynck commented Oct 17, 2017

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

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Oct 20, 2017

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!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 20, 2017

@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 =)
@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Oct 23, 2017

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

  1. 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(),
  1. 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.
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 23, 2017

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

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Oct 28, 2017

@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"
    }
]
@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Oct 30, 2017

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?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 30, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 30, 2017

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

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Nov 2, 2017

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

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Nov 2, 2017

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

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Nov 2, 2017

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

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Nov 5, 2017

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

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Nov 5, 2017

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

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Nov 5, 2017

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

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Nov 5, 2017

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

@chrisvittal

This comment has been minimized.

Copy link
Contributor

chrisvittal commented Nov 5, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Nov 6, 2017

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.

bors added a commit that referenced this issue Nov 15, 2017

Auto merge of #45918 - chrisvittal:impl-trait-pr, r=nikomatsakis
Implement `impl Trait` in argument position (RFC1951, Universal quantification)

Implements the remainder of #44721, part of #34511.

**Note**: This PR currently allows argument position `impl Trait` in trait functions. The machinery is there to prevent this if we want to, but it currently does not.

Rename `hir::TyImplTrait` to `hir::TyImplTraitExistential` and add `hir::TyImplTraitUniversal(DefId, TyParamBounds)`. The `DefId` is needed to extract the index of the parameter in `ast_ty_to_ty`.

Introduce an `ImplTraitContext` enum to lowering to keep track of the kind and allowedness of `impl Trait` in that position. This new argument is passed through many places, all ending up in `lower_ty`.

Modify `generics_of` and `explicit_predicates_of` to collect the `impl Trait` args into anonymous synthetic generic parameters and to extend the predicates with the appropriate bounds.

Add a comparison of the 'syntheticness' of type parameters, that is, prevent the following.
```rust
trait Foo {
    fn foo(&self, &impl Debug);
}
impl Foo for Bar {
    fn foo<U: Debug>(&self, x: &U) { ... }
}
```
And vice versa.

Incedentally, supress `unused type parameter` errors if the type being compared is already a `TyError`.

**TODO**: I have tried to annotate open questions with **FIXME**s. The most notable ones that haven't been resolved are the names of the `impl Trait` types and the questions surrounding the new `compare_synthetic_generics` method.
1. For now, the names used for `impl Trait` parameters are `keywords::Invalid.name()`. I would like them to be `impl ...` if possible, but I haven't figured out a way to do that yet.
2. For `compare_synthetic_generics` I have tried to outline the open questions in the [function itself](https://github.com/chrisvittal/rust/blob/3fc9e3705f7bd01f3cb0ea470cf2892f17a92350/src/librustc_typeck/check/compare_method.rs#L714-L725)

r? @nikomatsakis

bors added a commit that referenced this issue Nov 15, 2017

Auto merge of #45918 - chrisvittal:impl-trait-pr, r=nikomatsakis
Implement `impl Trait` in argument position (RFC1951, Universal quantification)

Implements the remainder of #44721, part of #34511.

**Note**: This PR currently allows argument position `impl Trait` in trait functions. The machinery is there to prevent this if we want to, but it currently does not.

Rename `hir::TyImplTrait` to `hir::TyImplTraitExistential` and add `hir::TyImplTraitUniversal(DefId, TyParamBounds)`. The `DefId` is needed to extract the index of the parameter in `ast_ty_to_ty`.

Introduce an `ImplTraitContext` enum to lowering to keep track of the kind and allowedness of `impl Trait` in that position. This new argument is passed through many places, all ending up in `lower_ty`.

Modify `generics_of` and `explicit_predicates_of` to collect the `impl Trait` args into anonymous synthetic generic parameters and to extend the predicates with the appropriate bounds.

Add a comparison of the 'syntheticness' of type parameters, that is, prevent the following.
```rust
trait Foo {
    fn foo(&self, &impl Debug);
}
impl Foo for Bar {
    fn foo<U: Debug>(&self, x: &U) { ... }
}
```
And vice versa.

Incedentally, supress `unused type parameter` errors if the type being compared is already a `TyError`.

**TODO**: I have tried to annotate open questions with **FIXME**s. The most notable ones that haven't been resolved are the names of the `impl Trait` types and the questions surrounding the new `compare_synthetic_generics` method.
1. For now, the names used for `impl Trait` parameters are `keywords::Invalid.name()`. I would like them to be `impl ...` if possible, but I haven't figured out a way to do that yet.
2. For `compare_synthetic_generics` I have tried to outline the open questions in the [function itself](https://github.com/chrisvittal/rust/blob/3fc9e3705f7bd01f3cb0ea470cf2892f17a92350/src/librustc_typeck/check/compare_method.rs#L714-L725)

r? @nikomatsakis
@CryZe

This comment has been minimized.

Copy link
Contributor

CryZe commented Nov 29, 2017

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.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Nov 29, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Feb 25, 2018

I think this is basically done. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment