Implement `impl Trait` in argument position (RFC1951, Universal quantification) #45918

chrisvittal · 2017-11-10T18:58:42Z

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.

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

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.
For compare_synthetic_generics I have tried to outline the open questions in the function itself

r? @nikomatsakis

cramertj · 2017-11-10T19:15:34Z

src/librustc_typeck/diagnostics.rs

@@ -4665,6 +4625,22 @@ It is recommended that you look for a `new` function or equivalent in the
 crate's documentation.
 "##,

+E0642: r##"
+This error indicates that there is a mismatch between generic parameters and
+impl Trait parameters in a trait Declaration versus it's impl.


Nits: "Declaration" should probably be lowercase, "s/it's/its"

cramertj · 2017-11-10T19:18:05Z

src/librustc_typeck/diagnostics.rs

+```compile_fail,E0642
+#![feature(conservative_impl_trait)]
+trait Foo
+    fn foo(&self, &impl Iterator)


Unnamed parameters are deprecated. Consider changing this to fn foo(&self, iter: &impl Iterator)

I just noticed that I need to change the feature as well.

@cramertj It is sufficient to go from &impl Iterator to _: &impl Iterator, correct?

Sure, I suppose you could do that.

cramertj · 2017-11-10T19:19:14Z

src/test/compile-fail/impl-trait/impl-generic-mismatch.rs

+use std::fmt::Debug;
+
+trait Foo {
+    fn foo(&self, &impl Debug);


DItto RE unnamed arguments

chrisvittal · 2017-11-11T00:09:16Z

Try this again,

r? @nikomatsakis

chrisvittal · 2017-11-11T14:51:46Z

Oh! I just thought of one more thing to do. I need to clean up E0562 as that is the error we use for both positions now.

nikomatsakis · 2017-11-12T21:21:22Z

@chrisvittal sorry didn't get a chance to review this weekend, will shortly though I promise!

nikomatsakis

OK, I did a reasonably thorough review. The code is mostly excellent. I left some nits. However, there are a few discrepancies that I think we should correct before landing -- or at least note for later follow-up. I'm also going to push a few commits with various tests and edits.

nikomatsakis · 2017-11-13T18:08:04Z

src/librustc/hir/lowering.rs

            output: match decl.output {
-                FunctionRetTy::Ty(ref ty) => hir::Return(self.lower_ty(ty)),
+                FunctionRetTy::Ty(ref ty) => match (impl_trait_return_allow, fn_def_id) {


Nit: It's a bit hard to decipher this match. I think I'd rather see this as

if impl_trait_return_allow && fn_def_id.is_some() { let fn_def_if = fn_def_id.unwrap(); hir::Return(self.lower_ty(ty, ImplTraitContext::Existential(fn_def_id))) } else { hir::Return(self.lower_ty(ty, ImplTraitContext::Disallowed)) }

or perhaps

match fn_def_id { Some(d) if impl_trait_return_allow => hir::Return(self.lower_ty(ty, ImplTraitContext::Existential(d))), _ => hir::Return(self.lower_ty(ty, ImplTraitContext::Disallowed)) }

since that emphasizes that there are really just two outcomes here.

The second is good. I forgot you could do that.

nikomatsakis · 2017-11-13T18:09:01Z

src/librustc/hir/lowering.rs

@@ -1719,12 +1798,13 @@ impl<'a> LoweringContext<'a> {
                attrs: this.lower_attrs(&i.attrs),
                node: match i.node {
                    ForeignItemKind::Fn(ref fdec, ref generics) => {
-                        hir::ForeignItemFn(this.lower_fn_decl(fdec),
+                        let fn_def_id = this.resolver.definitions().opt_local_def_id(i.id);
+                        hir::ForeignItemFn(this.lower_fn_decl(fdec, fn_def_id, true),


I don't think we want to allow impl trait in foreign items. That would be something like this:

extern fn foo() -> impl Trait;

I agree. I overlooked this.

nikomatsakis · 2017-11-13T18:10:30Z

src/librustc/hir/lowering.rs

                    }
                    ImplItemKind::Method(ref sig, ref body) => {
                        let body_id = this.lower_body(Some(&sig.decl), |this| {
                            let body = this.lower_block(body, false);
                            this.expr_block(body, ThinVec::new())
                        });
-                        hir::ImplItemKind::Method(this.lower_method_sig(sig), body_id)
+                        let impl_trait_return_allow = !this.is_in_trait_impl;
+                        hir::ImplItemKind::Method(this.lower_method_sig(sig, fn_def_id,


Nit: each param on its own line.

nikomatsakis · 2017-11-13T18:13:04Z

src/librustc/hir/lowering.rs

@@ -1055,11 +1095,24 @@ impl<'a> LoweringContext<'a> {
        }).collect()
    }

-    fn lower_fn_decl(&mut self, decl: &FnDecl) -> P<hir::FnDecl> {
+    fn lower_fn_decl(&mut self,


Can we leave a comment about the interaction between fn_def_id and impl_trait_return_allow? Something like:

If fn_def_id is Some, then impl traits will be allowed in argument position, and they will be translated into universal arguments bound on the given fn-def-id. If, further, impl_trait_return_allow is true, then impl trait will also be allowed in return position.

It might also be nice to explain why we have it this way. For example:

We wish to support impl trait in argument position in traits and trait impls, but permit return position in inherent functions and methods. In yet other cases, such as extern functions, we wish to support neither.

nikomatsakis · 2017-11-13T18:16:11Z

src/librustc/hir/lowering.rs

@@ -103,6 +103,7 @@ pub struct LoweringContext<'a> {
    catch_scopes: Vec<NodeId>,
    loop_scopes: Vec<NodeId>,
    is_in_loop_condition: bool,
+    is_in_trait_impl: bool,


the commit message says is_in_impl_trait, but the field is is_in_trait_impl

Unfortunately, file is correct and I don't want to invalidate the history. Especially since you've also added new commits. What should I do?

nikomatsakis · 2017-11-13T18:17:36Z

src/librustc_typeck/collect.rs

+    }
+
+    let mut visitor = ImplTraitUniversalVisitor { items: Vec::new() };
+    opt_inputs.map(|inputs| for t in inputs.iter() {


Nit: if let Some(inputs) = opt_inputs { ... } would be clearer

nikomatsakis · 2017-11-13T19:28:11Z

Here are the things I think we need to fix before landing:

Most importantly, the name field being empty string seems like a problem. See the ui test in my most recent commit. That said, we could conceivably leave this for followup, and I'd be fine with that I think -- but it seems like we should at least use a hardcoded name like "impl Trait".
There are a few discrepancies in terms of where impl trait is allowed. Things don't seem quite right. These are reflected in my where-allowed.rs test, with FIXME comments.
- It should not be allowed in foreign items.
- We should either consistently allow in return position of a type (e.g., fn() -> impl Debug) or not. Right now we allow it for closure types like dyn Fn() -> impl Debug but disallow it for fn pointer types.
- The RFC is sort of ambiguous: it's clear we do no want to allow in parameter position, but return position seems harmless enough. Still, I'm fine with just making it an error for now (which is how I wrote the test) if we want to go slow. Easy to expand later.
In my ideal world, we'd have a distinct test covering each of the cases in where-allowed where (universal) impl Trait is accepted. We don't. I added some, along with some other trickier cases that leapt immediately to mind.

chrisvittal · 2017-11-14T06:14:47Z

Big reason to deny Fn() -> impl Trait, as of right now, the following is an ICE.

fn in_generics_Fn<F: Fn() -> impl Debug>(_: F) {}

nikomatsakis · 2017-11-15T18:30:47Z

@bors r+ p=1

This is an awesome change. I am gratuitously giving it p=1 because it'd be really nice to see it land before the end of the week.

bors · 2017-11-15T18:30:48Z

📌 Commit 53dceb4 has been approved by nikomatsakis

bors · 2017-11-15T18:39:29Z

⌛ Testing commit 53dceb489e31daef08cd71b0cfba212223a24f87 with merge bf53d9c666f1f99e77ba994172a24091f23f0377...

bors · 2017-11-15T20:18:10Z

💔 Test failed - status-travis

With the check for impl trait moving from type checking to HIR lowering the error needs to move too.

Replace hir::TyImplTrait with TyImplTraitUniversal and TyImplTraitExistential. Add an ImplTraitContext enum to rustc::hir::lowering to track the kind and allowedness of an impl Trait. Significantly alter lowering to thread ImplTraitContext and one other boolean parameter described below throughought much of lowering. The other parameter is for tracking if lowering a function is in a trait impl, as there is not enough information to otherwise know this information during lowering otherwise. This change also removes the checks from ast_ty_to_ty for impl trait allowedness as they are now all taking place in HIR lowering.

This is for tracking if an ImplItem is part of a trait impl. Add a with_trait_impl_ref method to ItemLowerer to appropriately save the state to allow appropriate nesting of trait and non-trait impls.

In ast_generics extraction in generics_of and explicit_predicates_of, also collect inputs if there are any. Then use a Visitor to extract the necessary information from the TyImplTraitUniversal types before extending generics and predicates with the new information.

First some background: To the compiler, the following two signatures in the trait vs the impl are the same. ```rust trait Foo { fn foo(&self, &impl Debug); } impl Foo for () { fn foo<U: Debug>(&self, x: &U) { ... } } ``` We do not want to allow this, and so we add a new error and check. The check just tests that all paramters 'syntheticness' match up. As during collection, the impl Trait parameters are transformed into anonymous synthetic generics. Furthermore, causes a check for unused type parameters to be skipped in check_bounds_are_used if there is already a TyError. Thus, an unused input will not trigger `type parameter unused` errors. Update the one test that checked for this error in the case of a TyError.

Move feature gate check to inside HIR lowering. Change error messages and update tests.

Uses Symbol::intern and hir.node_to_pretty_string to create a name for the impl Trait parameter that is just impl and then a ' + ' separated list of bounds that the user typed.

Add requested comments, restructure some small bits of code. Fix extern declarations allowing impl Trait.

We already disallowed them to be in the arg list, such as Fn(impl Debug), but now we disallow Fn() -> impl Debug. Also remove the ImplTraitContext argument from the function lower_parenthesized_parameter_data as it is now unused. Comment out part of test run-pass/impl-trait/xcrate.rs that now fails.

nikomatsakis · 2017-11-15T20:47:58Z

@bors r+

bors · 2017-11-15T20:47:58Z

📌 Commit 4ce61b7 has been approved by nikomatsakis

nikomatsakis · 2017-11-15T20:48:15Z

cc @Manishearth @oli-obk -- note that clippy is affected here (not sure if you were cc'd before).

bors · 2017-11-15T22:48:00Z

⌛ Testing commit 4ce61b7 with merge b8c70b0...

@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 · 2017-11-16T01:16:53Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing b8c70b0 to master...

spease · 2017-11-24T07:41:56Z

There's something I read that discussed this change, but I couldn't find it again.

The particular essay pointed out that once someone is exposed to generics, it becomes intuitive to try to use a generic for the return type in order to effectively do an impl Trait.

I just wanted to note that I concur with that statement. That's what I tried.

It was then argued that allowing impl Trait in argument position would allow people to get exposed to this first, and hence be conducive to them trying impl Trait in the return position. I think this too is accurate.

That being said, I would assert that specifying a type should be consistent - if impl Trait is allowed to specify a type, then generics should be too, and vice versa. This makes it simpler to understand what tools you have available to you, because then when you are familiar with all the tools for specifying a type name, you know exactly which tools you can use whenever you see a typename - otherwise you have to remember every case you can use those tools in.

This would also imply impl Trait in other areas - eg on a struct.

I don't know if this would be massively difficult to implement, I'm purely suggesting that from a user developer perspective this would help simplify learning by making the language more consistent.

sinkuu · 2018-04-10T02:27:34Z

@chrisvittal

This PR allows using impl Trait in type parameter:

fn foo<T: Bar<impl Clone>>(_x: T) {
}

(despite being prohibited in where clause:

fn foo<T>(_x: T) where T: Bar<impl Clone> {
//^ `impl Trait` not allowed outside of function and inherent method return types
}

)

Is this intentional? I had a grance through RFC, and I think it does not state impl Trait can be used in type bounds.

cramertj reviewed Nov 10, 2017

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2017

rust-highfive assigned nikomatsakis Nov 11, 2017

nikomatsakis requested changes Nov 13, 2017

View reviewed changes

nikomatsakis approved these changes Nov 15, 2017

View reviewed changes

chrisvittal and others added 13 commits November 15, 2017 15:46

Move E0562 to librustc from librustc_typeck

779fc37

With the check for impl trait moving from type checking to HIR lowering the error needs to move too.

Add bool item is_in_impl_trait to LoweringContext

e4c7e2c

This is for tracking if an ImplItem is part of a trait impl. Add a with_trait_impl_ref method to ItemLowerer to appropriately save the state to allow appropriate nesting of trait and non-trait impls.

Add collection of impl Trait argument lifetimes

f225fe4

Add universal_impl_trait feature gate

bdff946

Move feature gate check to inside HIR lowering. Change error messages and update tests.

Add/Modify tests for argument position impl Trait

06dff80

Fix style and grammar

a23bea5

Remove unamed parameters

7d25d2e

Fix unclosed delimiter in sample error

04ad8fd

Add universal_impl_trait unstable-book entry

b4c1aef

test we reject equivalent signatures with more than one argument

2520279

nikomatsakis and others added 10 commits November 15, 2017 15:46

some tests featuring multiple bounds, other errors

2786ea6

Add proper names to impl Trait parameters.

7e9948f

Uses Symbol::intern and hir.node_to_pretty_string to create a name for the impl Trait parameter that is just impl and then a ' + ' separated list of bounds that the user typed.

Incorporate review feedback

9b4372e

Add requested comments, restructure some small bits of code. Fix extern declarations allowing impl Trait.

Add/Fix stderr references for impl Trait ui tests

f710d41

Add cases to where-allowed.rs

22f0940

Renumber error to fix tidy

517db79

Remove Fn trait + impl Trait rustdoc tests

337dee4

add a new test featuring two impl traits to show what it looks like

98d5db3

Change clippy to broken after hir::Ty enum change

4ce61b7

chrisvittal force-pushed the impl-trait-pr branch from 74aeb66 to 4ce61b7 Compare November 15, 2017 20:46

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2017

bors merged commit 4ce61b7 into rust-lang:master Nov 16, 2017

chrisvittal deleted the impl-trait-pr branch November 16, 2017 01:18

This was referenced Nov 16, 2017

trait alias infrastructure #45047

Merged

impl Trait Lifetime Handling #45701

Merged

chrisvittal mentioned this pull request Nov 16, 2017

Split TyImplTrait into Universal and Existential rust-lang/rust-clippy#2228

Merged

ghost mentioned this pull request Nov 21, 2017

Remove Atomic's _owned methods crossbeam-rs/crossbeam-epoch#35

Merged

ltratt mentioned this pull request Nov 23, 2017

Change in lifetime inference? #46184

Closed

bombless mentioned this pull request Nov 27, 2022

Confusing impl Trait limitation #104526

Closed

fmease mentioned this pull request Oct 27, 2024

Syntactically rejecting impl-Trait inside non-final path segments & inside fn ptr types is futile #132212

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `impl Trait` in argument position (RFC1951, Universal quantification) #45918

Implement `impl Trait` in argument position (RFC1951, Universal quantification) #45918

chrisvittal commented Nov 10, 2017 •

edited

Loading

cramertj Nov 10, 2017

chrisvittal Nov 10, 2017

cramertj Nov 10, 2017

chrisvittal Nov 10, 2017

chrisvittal Nov 10, 2017

cramertj Nov 10, 2017

cramertj Nov 10, 2017

chrisvittal commented Nov 11, 2017

chrisvittal commented Nov 11, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis left a comment

nikomatsakis Nov 13, 2017

chrisvittal Nov 13, 2017

nikomatsakis Nov 13, 2017

chrisvittal Nov 13, 2017

nikomatsakis Nov 13, 2017

nikomatsakis Nov 13, 2017

nikomatsakis Nov 13, 2017

chrisvittal Nov 13, 2017

nikomatsakis Nov 13, 2017

nikomatsakis commented Nov 13, 2017

chrisvittal commented Nov 14, 2017 •

edited

Loading

nikomatsakis commented Nov 15, 2017

bors commented Nov 15, 2017

bors commented Nov 15, 2017

bors commented Nov 15, 2017

nikomatsakis commented Nov 15, 2017

bors commented Nov 15, 2017

nikomatsakis commented Nov 15, 2017

bors commented Nov 15, 2017

bors commented Nov 16, 2017

spease commented Nov 24, 2017

sinkuu commented Apr 10, 2018

Implement impl Trait in argument position (RFC1951, Universal quantification) #45918

Implement impl Trait in argument position (RFC1951, Universal quantification) #45918

Conversation

chrisvittal commented Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisvittal commented Nov 11, 2017

chrisvittal commented Nov 11, 2017

nikomatsakis commented Nov 12, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Nov 13, 2017

chrisvittal commented Nov 14, 2017 • edited Loading

nikomatsakis commented Nov 15, 2017

bors commented Nov 15, 2017

bors commented Nov 15, 2017

bors commented Nov 15, 2017

nikomatsakis commented Nov 15, 2017

bors commented Nov 15, 2017

nikomatsakis commented Nov 15, 2017

bors commented Nov 15, 2017

bors commented Nov 16, 2017

spease commented Nov 24, 2017

sinkuu commented Apr 10, 2018

Implement `impl Trait` in argument position (RFC1951, Universal quantification) #45918

Implement `impl Trait` in argument position (RFC1951, Universal quantification) #45918

chrisvittal commented Nov 10, 2017 •

edited

Loading

chrisvittal commented Nov 14, 2017 •

edited

Loading