impl Trait Lifetime Handling #45701

cramertj · 2017-11-01T21:43:23Z

This PR implements the updated strategy for handling impl Trait lifetimes, as described in RFC 1951 (cc #42183).

With this PR, the impl Trait desugaring works as follows:

fn foo<T, 'a, 'b, 'c>(...) -> impl Foo<'a, 'b> { ... }
// desugars to
exists type MyFoo<ParentT, 'parent_a, 'parent_b, 'parent_c, 'a, 'b>: Foo<'a, 'b>;
fn foo<T, 'a, 'b, 'c>(...) -> MyFoo<T, 'static, 'static, 'static, 'a, 'b> { ... }

All of the in-scope (parent) generics are listed as parent generics of the anonymous type, with parent regions being replaced by 'static. Parent regions referenced in the impl Trait return type are duplicated into the anonymous type's generics and mapped appropriately.

One case came up that wasn't specified in the RFC: it's possible to write a return type that contains multiple regions, neither of which outlives the other. In that case, it's not clear what the required lifetime of the output type should be, so we generate an error.

There's one remaining FIXME in one of the tests: -> impl Foo<'a, 'b> + 'c should be able to outlive both 'a and 'b, but not 'c. Currently, it can't outlive any of them. @nikomatsakis and I have discussed this, and there are some complex interactions here if we ever allow impl<'a, 'b> SomeTrait for AnonType<'a, 'b> { ... }, so the plan is to hold off on this until we've got a better idea of what the interactions are here.

cc #34511.
Fixes #44727.

rust-highfive · 2017-11-01T21:43:35Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

nikomatsakis · 2017-11-01T21:54:58Z

r? @nikomatsakis

nikomatsakis · 2017-11-01T21:55:16Z

Well, I guess I did write some of the core commits. Maybe I shouldn't review...

nikomatsakis · 2017-11-01T21:56:10Z

r? @eddyb

carols10cents · 2017-11-06T14:06:59Z

review ping @eddyb - pinging you in IRC too!

cramertj · 2017-11-13T21:01:35Z

Ping @eddyb

eddyb · 2017-11-14T10:16:07Z

src/librustc/hir/mod.rs

@@ -1450,8 +1456,7 @@ pub enum Ty_ {
    /// where `Bound` is a trait or a lifetime.
    TyTraitObject(HirVec<PolyTraitRef>, Lifetime),
    /// An `impl Bound1 + Bound2 + Bound3` type
-    /// where `Bound` is a trait or a lifetime.
-    TyImplTrait(TyParamBounds),
+    TyImplTrait(ExistTy, HirVec<Lifetime>),


Why are the lifetimes separate? Shouldn't they go in the TyParamBounds?

Nope. These are the lifetimes that are in the reference to the abstract type, not its definition. That is, consider the desugaring of fn foo<'a>() -> impl Trait<'a>. You get:

abstract type Foo<'b>: Trait<'b>; // ^^ this `'b` is what appears in the `ExistTy` // ^^ and this is the `TyParamBounds`, note that // the lifetime here will reference the lifetime // `'b` defined in the `abstract type` fn foo<'a>() -> Foo<'a> { .. } // ^^ this `'a` is the one that appears in this `HirVec<Lifetime>

(Clearly, we need some more comments here.)

These lifetimes are resolved outside of the generics, so they pick up whatever is in the environment-- see here. Later, those lifetimes are "applied" to the generics here.

In comparison, the lifetimes in the TyParamBounds are resolved to the Generics (here).

You can think about a TyImplTrait as an abstract type Foo<'a, 'b>: MyTrait + 'a + 'b; paired with the lifetime parameters applied to it, 'a and 'b. You can also think about this list as containing all of the type parameters, but they don't have to be separately applied since they're just picked up from the parent generics.

r=me with a comment for this added.

eddyb · 2017-11-14T10:29:18Z

src/librustc/hir/lowering.rs

+                // Exclude '_, 'static, and elided lifetimes (there should be no elided lifetimes)
+                if let hir::LifetimeName::Name(lifetime_name) = lifetime.name {
+                    if !self.currently_bound_lifetimes.contains(&lifetime_name) &&
+                       !self.already_defined_lifetimes.contains(&lifetime_name)


At this point I think that we should move named lifetime resolution (mapping a NodeId of an use to a DefId of a definition without computing the internal representation or doing elision and most of the checks) to rustc_resolve. But you probably don't want (even) more delays, so I think we can have this for now.

Definitely a separate PR. It does seem like it would be a more natural fit with how other name resolution works, though it will make stuff like what we are doing here more annoying. The "HIR-rewriting" works much more smoothly since name resolution hasn't happened yet.

How so? All of this seems harder to do with ad-hoc name resolution. But sure, we can leave it for later.

cramertj · 2017-11-15T00:52:22Z

I've added some comments and some more (passing) tests. However, I also came up with a test that currently ICEs:

fn closure_hrtb() -> impl for<'a> Fn(&'a u32)
                  -> impl Debug + 'a {
    |x| x
}

The failure is this assertion.

nikomatsakis · 2017-11-15T21:35:56Z

So, this is a bit trickier than I initially thought. I actually think we should (for now, at least) be forbidding this sort of construction. It effectively desugars into:

// abstract type A<'a>: Debug + 'a;
// abstract type B: for<'a> Fn(&'a u32) -> A<'a>;
// fn closure_hrtb() -> B { |x| x }

However, we don't currently have a very clean way of talking about a type variable instantiated "inside" binders. That is, the type variable we create to infer what A<'a> is ought to be "inside" the for<'a> binder -- but we're going to create all of them at the top level (where B exists). I think that as we transition to universes we'll be better equipped to handle constraints of this form in a natural way; I'm wary of trying to support them now if we don't have to.

That said, it does seem like this sort of thing might be desired in the wild.

cramertj · 2017-11-15T21:52:02Z

How would you suggest banning this? Issue an error? "cannot use impl Trait inside HRTB inside impl Trait"

bors · 2017-11-16T01:19:31Z

☔ The latest upstream changes (presumably #45918) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2017-11-16T18:01:25Z

src/librustc/middle/resolve_lifetime.rs

+                                 krate.trait_items.contains_key(&parent_trait_id))
+                            {
+                                span_err!(self.sess, lifetime.span, E0657,
+                                  "`impl Trait` can only capture lifetimes bound at the fn level");


seems good. One nit with the message is that we could capture e.g.

impl<'a> Foo<'a> { fn bar(&self) -> impl Trait<'a> { .. } }

"fn or impl level"?

After this change, impl Trait existentials are desugared to a new `abstract type` definition paired with a set of lifetimes to apply. In-scope generics are included as parents of the `abstract type` generics. Parent regions are replaced with static, and parent regions referenced in the `impl Trait` type are duplicated at the end of the `abstract type`'s generics.

cramertj · 2017-11-17T22:33:59Z

Failure looks unrelated:

[00:04:15] fatal: packed object 5c6856d4803cb6a830424428062f43dec4db9a21 (stored in /Users/travis/rustsrc/src/.git/modules/reference/objects/pack/pack-0963eb7b83f69d1d481da777eb771fc2c346f000.pack) is corrupt

[00:04:15] fatal: remote did not send all necessary objects

[00:04:15] fatal: clone of 'https://github.com/rust-lang-nursery/reference.git' into submodule path '/Users/travis/build/rust-lang/rust/src/doc/reference' failed

[00:04:15] Failed to clone 'src/doc/reference' a second time, aborting

[00:04:15] Command failed. Attempt 5/5:

[00:04:15] Submodule 'reference' (https://github.com/rust-lang-nursery/reference.git) unregistered for path 'src/doc/reference'

[00:04:15] Submodule 'reference' (https://github.com/rust-lang-nursery/reference.git) registered for path 'src/doc/reference'

[00:04:15] Cloning into '/Users/travis/build/rust-lang/rust/src/doc/reference'...

[00:04:16] error: failed to read object 5c6856d4803cb6a830424428062f43dec4db9a21 at offset 73516 from /Users/travis/rustsrc/src/.git/modules/reference/objects/pack/pack-0963eb7b83f69d1d481da777eb771fc2c346f000.pack

[00:04:16] fatal: packed object 5c6856d4803cb6a830424428062f43dec4db9a21 (stored in /Users/travis/rustsrc/src/.git/modules/reference/objects/pack/pack-0963eb7b83f69d1d481da777eb771fc2c346f000.pack) is corrupt

[00:04:16] fatal: remote did not send all necessary objects

[00:04:16] fatal: clone of 'https://github.com/rust-lang-nursery/reference.git' into submodule path '/Users/travis/build/rust-lang/rust/src/doc/reference' failed

[00:04:16] Failed to clone 'src/doc/reference'. Retry scheduled

[00:04:16] Cloning into '/Users/travis/build/rust-lang/rust/src/doc/reference'...

[00:04:16] error: failed to read object 5c6856d4803cb6a830424428062f43dec4db9a21 at offset 73516 from /Users/travis/rustsrc/src/.git/modules/reference/objects/pack/pack-0963eb7b83f69d1d481da777eb771fc2c346f000.pack

[00:04:16] fatal: packed object 5c6856d4803cb6a830424428062f43dec4db9a21 (stored in /Users/travis/rustsrc/src/.git/modules/reference/objects/pack/pack-0963eb7b83f69d1d481da777eb771fc2c346f000.pack) is corrupt

[00:04:16] fatal: remote did not send all necessary objects

[00:04:16] fatal: clone of 'https://github.com/rust-lang-nursery/reference.git' into submodule path '/Users/travis/build/rust-lang/rust/src/doc/reference' failed

[00:04:16] Failed to clone 'src/doc/reference' a second time, aborting

[00:04:16] The command has failed after 5 attempts.

kennytm · 2017-11-18T07:06:32Z

@bors retry #42117

bors · 2017-11-18T07:55:35Z

⌛ Testing commit bc4810d with merge 07904bb68415bbe57a39c5c5eabf1150e0159c9c...

bors · 2017-11-18T09:18:10Z

💔 Test failed - status-travis

kennytm · 2017-11-18T09:33:22Z

Legit, please break clippy (edit src/tools/toolstate.toml). cc @Manishearth @llogiq @mcarton @oli-obk

[01:15:57]    Compiling clippy_lints v0.0.171 (file:///Users/travis/build/rust-lang/rust/src/tools/clippy/clippy_lints)
[01:16:03] error[E0023]: this pattern has 1 field, but the corresponding tuple variant has 2 fields
[01:16:03]    --> src/tools/clippy/clippy_lints/src/lifetimes.rs:328:13
[01:16:03]     |
[01:16:03] 328 |             TyImplTraitExistential(ref param_bounds) |
[01:16:03]     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 2 fields, found 1
[01:16:03] 
[01:16:04] error[E0023]: this pattern has 1 field, but the corresponding tuple variant has 2 fields
[01:16:04]    --> src/tools/clippy/clippy_lints/src/lifetimes.rs:328:13
[01:16:04]     |
[01:16:04] 328 |             TyImplTraitExistential(ref param_bounds) |
[01:16:04]     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 2 fields, found 1
[01:16:04] 
[01:16:04] error: aborting due to previous error
[01:16:04] 
[01:16:04] error: Could not compile `clippy_lints`.
[01:16:04] warning: build failed, waiting for other jobs to finish...
[01:16:06] error: aborting due to previous error
[01:16:06] 
[01:16:06] error: Could not compile `clippy_lints`.

cramertj · 2017-11-20T17:52:28Z

@kennytm Done.

kennytm · 2017-11-20T17:55:01Z

@bors r=eddyb

bors · 2017-11-20T17:55:02Z

📌 Commit 450bbc5 has been approved by eddyb

bors · 2017-11-21T10:00:25Z

⌛ Testing commit 450bbc5 with merge ebda766...

@nikomatsakis

impl Trait Lifetime Handling This PR implements the updated strategy for handling `impl Trait` lifetimes, as described in [RFC 1951](https://github.com/rust-lang/rfcs/blob/master/text/1951-expand-impl-trait.md) (cc #42183). With this PR, the `impl Trait` desugaring works as follows: ```rust fn foo<T, 'a, 'b, 'c>(...) -> impl Foo<'a, 'b> { ... } // desugars to exists type MyFoo<ParentT, 'parent_a, 'parent_b, 'parent_c, 'a, 'b>: Foo<'a, 'b>; fn foo<T, 'a, 'b, 'c>(...) -> MyFoo<T, 'static, 'static, 'static, 'a, 'b> { ... } ``` All of the in-scope (parent) generics are listed as parent generics of the anonymous type, with parent regions being replaced by `'static`. Parent regions referenced in the `impl Trait` return type are duplicated into the anonymous type's generics and mapped appropriately. One case came up that wasn't specified in the RFC: it's possible to write a return type that contains multiple regions, neither of which outlives the other. In that case, it's not clear what the required lifetime of the output type should be, so we generate an error. There's one remaining FIXME in one of the tests: `-> impl Foo<'a, 'b> + 'c` should be able to outlive both `'a` and `'b`, but not `'c`. Currently, it can't outlive any of them. @nikomatsakis and I have discussed this, and there are some complex interactions here if we ever allow `impl<'a, 'b> SomeTrait for AnonType<'a, 'b> { ... }`, so the plan is to hold off on this until we've got a better idea of what the interactions are here. cc #34511. Fixes #44727.

bors · 2017-11-21T12:32:54Z

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

Additional uses of this item were added to these files in rust-lang#45701 and rust-lang#46479

rust-highfive assigned pnkfelix Nov 1, 2017

cramertj changed the title ~~Refactor and impl Trait Lifetime Handling~~ impl Trait Lifetime Handling Nov 1, 2017

rust-highfive assigned nikomatsakis and unassigned pnkfelix Nov 1, 2017

rust-highfive assigned eddyb and unassigned nikomatsakis Nov 1, 2017

cramertj force-pushed the impl-trait-this-time branch from b3d0405 to 9ea3b2f Compare November 1, 2017 23:33

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

cramertj force-pushed the impl-trait-this-time branch 4 times, most recently from 7667bbc to 0d12267 Compare November 10, 2017 10:05

eddyb reviewed Nov 14, 2017

View reviewed changes

aturon mentioned this pull request Nov 15, 2017

Tracking issue for impl Trait (RFC 1522, RFC 1951, RFC 2071) #34511

Closed

29 tasks

nikomatsakis reviewed Nov 16, 2017

View reviewed changes

cramertj force-pushed the impl-trait-this-time branch 2 times, most recently from 00e6136 to bd0f2c7 Compare November 17, 2017 17:59

cramertj force-pushed the impl-trait-this-time branch from bd0f2c7 to bc4810d Compare November 17, 2017 18:02

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 18, 2017

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 18, 2017

Clippy is broken

450bbc5

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 20, 2017

bors merged commit 450bbc5 into rust-lang:master Nov 21, 2017

This was referenced Nov 21, 2017

Generics refactoring (groundwork for const generics) #45930

Merged

Immovable types prototype where the Move trait is builtin #44917

Closed

Implement in-band lifetime bindings #46051

Merged

cramertj deleted the impl-trait-this-time branch December 9, 2017 01:44

sgrif added a commit to sgrif/rust that referenced this pull request Jan 31, 2018

Re-add some removed uses of Kind

cd8bc39

Additional uses of this item were added to these files in rust-lang#45701 and rust-lang#46479

sgrif added a commit to sgrif/rust that referenced this pull request Feb 6, 2018

Re-add some removed uses of Kind

d4727ef

Additional uses of this item were added to these files in rust-lang#45701 and rust-lang#46479

sgrif added a commit to sgrif/rust that referenced this pull request Mar 1, 2018

Re-add some removed uses of Kind

4fb201d

Additional uses of this item were added to these files in rust-lang#45701 and rust-lang#46479

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl Trait Lifetime Handling #45701

impl Trait Lifetime Handling #45701

cramertj commented Nov 1, 2017 •

edited

Loading

rust-highfive commented Nov 1, 2017

nikomatsakis commented Nov 1, 2017

nikomatsakis commented Nov 1, 2017

nikomatsakis commented Nov 1, 2017

carols10cents commented Nov 6, 2017

cramertj commented Nov 13, 2017

eddyb Nov 14, 2017

nikomatsakis Nov 14, 2017

cramertj Nov 14, 2017 •

edited

Loading

eddyb Nov 14, 2017

eddyb Nov 14, 2017

nikomatsakis Nov 14, 2017

eddyb Nov 14, 2017

cramertj commented Nov 15, 2017 •

edited

Loading

nikomatsakis commented Nov 15, 2017

cramertj commented Nov 15, 2017 •

edited

Loading

bors commented Nov 16, 2017

nikomatsakis Nov 16, 2017

cramertj Nov 16, 2017

cramertj commented Nov 17, 2017

kennytm commented Nov 18, 2017

bors commented Nov 18, 2017

bors commented Nov 18, 2017

kennytm commented Nov 18, 2017 •

edited

Loading

cramertj commented Nov 20, 2017

kennytm commented Nov 20, 2017

bors commented Nov 20, 2017

bors commented Nov 21, 2017

bors commented Nov 21, 2017

impl Trait Lifetime Handling #45701

impl Trait Lifetime Handling #45701

Conversation

cramertj commented Nov 1, 2017 • edited Loading

rust-highfive commented Nov 1, 2017

nikomatsakis commented Nov 1, 2017

nikomatsakis commented Nov 1, 2017

nikomatsakis commented Nov 1, 2017

carols10cents commented Nov 6, 2017

cramertj commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj Nov 14, 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

cramertj commented Nov 15, 2017 • edited Loading

nikomatsakis commented Nov 15, 2017

cramertj commented Nov 15, 2017 • edited Loading

bors commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented Nov 17, 2017

kennytm commented Nov 18, 2017

bors commented Nov 18, 2017

bors commented Nov 18, 2017

kennytm commented Nov 18, 2017 • edited Loading

cramertj commented Nov 20, 2017

kennytm commented Nov 20, 2017

bors commented Nov 20, 2017

bors commented Nov 21, 2017

bors commented Nov 21, 2017

cramertj commented Nov 1, 2017 •

edited

Loading

cramertj Nov 14, 2017 •

edited

Loading

cramertj commented Nov 15, 2017 •

edited

Loading

cramertj commented Nov 15, 2017 •

edited

Loading

kennytm commented Nov 18, 2017 •

edited

Loading