Skip to content
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

use different lifetime name for object-lifetime-default elision #63376

Merged
merged 4 commits into from Aug 20, 2019

Conversation

@nikomatsakis
Copy link
Contributor

commented Aug 8, 2019

Introduce a distinct value for LifetimeName to use when this is a object-lifetime-default elision. This allows us to avoid creating incorrect lifetime parameters for the opaque types that result. We really need to overhaul this setup at some point! It's getting increasingly byzantine. But this seems like a relatively... surgical fix.

r? @cramertj

Fixes #62517

src/test/ui/async-await/issue-62517.rs Outdated Show resolved Hide resolved
src/test/ui/async-await/issue-62517.rs Outdated Show resolved Hide resolved
src/test/ui/impl-trait/dyn-trait-elided-two-inputs.rs Outdated Show resolved Hide resolved
src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved
src/librustc/hir/mod.rs Outdated Show resolved Hide resolved
src/test/ui/impl-trait/dyn-trait-elided-two-inputs.rs Outdated Show resolved Hide resolved

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:async-await-issue-62517 branch from 6d0f938 to 8e96dd3 Aug 8, 2019

src/test/ui/async-await/issue-62517.rs Outdated Show resolved Hide resolved
src/librustc/middle/resolve_lifetime.rs Outdated Show resolved Hide resolved
src/test/ui/impl-trait/dyn-trait-elided-two-inputs.rs Outdated Show resolved Hide resolved
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Note to self, as I'm not really at keyboard now:

Check what happens with &impl Trait<Item = &u32>, and what we expect to happen. =)

Also check similar scenarios like &dyn Iterator<Item = &u32>.

Arguably, we should create a lifetime parameter for the opaque type that we create, but it should get a value of 'static -- i.e., it should follow the "object lifetime default" rules to determine its value. That would be a slightly different fix than what we have here (though probably related).

@cramertj

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Check what happens with &impl Trait<Item = &u32>, and what we expect to happen. =)

Yeah, I'd also be curious about -> impl Trait<Item = Box<dyn OtherTrait<impl ThirdTrait>>>.

This fix makes sense to me, but the collection of lifetime-shifting-in-lowering logic is growing increasingly complex and scary. I don't have a particular fix in mind, but spending some time brainstorming could be good.

r=me with those tests added and @Centril's nits addressed.

@Centril Centril referenced this pull request Aug 8, 2019
9 of 9 tasks complete
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@cramertj

This fix makes sense to me, but the collection of lifetime-shifting-in-lowering logic is growing increasingly complex and scary. I don't have a particular fix in mind, but spending some time brainstorming could be good.

Yes.

Yeah, I'd also be curious about -> impl Trait<Item = Box<dyn OtherTrait<impl ThirdTrait>>>.

I was reviewing our object lifetime default rules (specified in RFC 599 and then amended in RFC 1156). The latter RFC in particular institutes the rule that we take the default from the "innermost enclosing type", essentially; hence &'x dyn Trait defaults to &'x (dyn Trait + 'x) but &'x Box<dyn Trait> defaults to &'x Box<dyn Trait + 'static>. This was in response to massive user confusion.

It seems to me that similar logic applies to &impl Foo<Bar = dyn Trait> -- that is, we should make the default in this case be the same as it would be for impl Foo<Bar = dyn Trait>, and hence it should default to dyn Trait + 'static. This is the behavior that the current PR performs, though I think I'd like to review the tests and make sure they are adequate.

Your example:

-> impl Trait<Item = Box<dyn OtherTrait<impl ThirdTrait>>>

would therefore default to:

-> impl Trait<Item = Box<dyn OtherTrait<impl ThirdTrait> + 'static>>
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

So I added tests for &impl Foo<Item = dyn Bar> and, sure enough, encountered problems. The current code for handling object-lifetime defaults basically overlooking Item = XX bindings, so they de facto just "inherit" the bound from the surrounding context.

On nightly, this means that &dyn Foo<Item = dyn Bar> expands to &'x dyn Foo<Item = dyn Bar + 'x> (example). The behavior for &impl is rather inconsistent: on nightly it seems to (de facto) default to 'static, though I'm not entirely sure why (example). On my branch, however, it would ICE because you wind up with &'x impl Foo<Item = dyn Bar + 'x>, and we've not created a lifetime parameter for the 'x (because of the other changes this branch makes).

I've got a fix that I just pushed but it's not really the right fix. It simply uses 'static as the default object lifetime bound in Item = XX bindings all the time.

Probably the right behavior is to operate analogously an input type parameter. In other words, if you have

trait Foo<'a, T: 'a + ?Sized> { .. }

and dyn Foo<'x, dyn Bar>, you will get dyn Foo<'x, dyn Bar + 'x>. I could imagine then that

trait Foo<'a> {
  type Item: 'a + ?Sized;
}

and dyn Foo<'a, Item = dyn Bar> would be expected to default to dyn Foo<'a, Item = dyn Bar + 'a>. But if you have

trait Foo {
   type Item: ?Sized;
}

then &dyn Foo<Item = dyn Bar> should default to dyn Bar + 'static (unlike today).

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:async-await-issue-62517 branch from 432bae1 to 83f0640 Aug 12, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Another, conservative option would be to require explicit bounds in this scenario. But that would be more of a breaking change.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

I've been investigating what it will take to do the more complex handling of object lifetime default bounds that I described above. It would definitely be complex, because at the phase where we are figuring this sort of thing out, we haven't yet resolved the Item to a specific associated type id. That is currently handled by a separate bit of code. On the other hand, I don't see any reason why it'd be impossible to do so -- we know the trait def-id, and that should give us everything we need to do the resolution. But it feels like it's going to be a non-trivial patch to me.

I suppose a conservative option would be to default to 'static unless the trait has lifetime parameters -- i.e., if there are no lifetime parameters, then we can't have the case I was thinking about above. I'm tinkering with implementation that for now. This should affect relatively little code -- you'd have to do something like dyn Foo<'a, Item = dyn Bar>.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:async-await-issue-62517 branch 2 times, most recently from d0c042d to afa4a2a Aug 12, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

I pushed a commit implementing the conservative rule I described above. It's technically a breaking change, as I noted, although I believe the current behavior is pretty clearly a bug. I wouldn't expect a large impact but it's hard to know.

The affected case is something like &dyn Foo<Item = dyn Bar> -- i.e., a &dyn type that contains a Item = binding with a dyn type that is not enclosed in some kind of pointer.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@bors try

In case we want to try and do a crater run

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

⌛️ Trying commit afa4a2a with merge 266783e...

bors added a commit that referenced this pull request Aug 13, 2019

Auto merge of #63376 - nikomatsakis:async-await-issue-62517, r=<try>
use different lifetime name for object-lifetime-default elision

Introduce a distinct value for `LifetimeName` to use when this is a object-lifetime-default elision. This allows us to avoid creating incorrect lifetime parameters for the opaque types that result. We really need to overhaul this setup at some point! It's getting increasingly byzantine. But this seems like a relatively... surgical fix.

r? @cramertj

Fixes #62517
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

☀️ Try build successful - checks-azure
Build commit: 266783e

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@craterbot run mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

👌 Experiment pr-63376 created and queued.
🤖 Automatically detected try build 266783e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Dear @rust-lang/lang -- this PR makes a breaking change (bug fix, in my opinion) to the compiler. We are doing a crater run now to see if there are any effects. I wrote up a summary of the situation in a gist, explaining both the background, intended behavior, current behavior, and the effect of this PR.

@Centril

This comment was marked as resolved.

Copy link
Member

commented Aug 13, 2019

@craterbot

This comment was marked as resolved.

Copy link
Collaborator

commented Aug 13, 2019

📝 Configuration of the pr-63376 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment was marked as resolved.

Copy link
Collaborator

commented Aug 13, 2019

🚧 Experiment pr-63376 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 17, 2019

🎉 Experiment pr-63376 is completed!
📊 3 regressed and 0 fixed (70078 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Centril
Copy link
Member

left a comment

This looks great. Thanks also for the write-up.

src/test/ui/async-await/issues/issue-62517-2.rs Outdated Show resolved Hide resolved
@Centril

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

Regression tree (from root):

  • soketto-0.2.2
    • libp2p-websocket-0.11.0
      • libp2p-0.11.0
        • blockchain-network-simple
          ^--- This is the only leaf reverse dependency in the whole tree.

Seems reasonably fixable by just sending a PR.
Imo the single root regression is acceptable enough to land this outright.

crlf0710 added a commit to crlf0710/soketto that referenced this pull request Aug 18, 2019

Fix issue with lifetime elision
Hello, according to rust-lang/rust#63376 , the compiler used to accept the current function signature of `drain_extensions()` by mistake, the compiler will soon fix this (after the above PR lands), and after that the current code won't compile.

This PR fixes the code. Please note that the latest published version soketto-0.2.2 contains this error too.

nikomatsakis added some commits Aug 8, 2019

distinguish object-lifetime-default elision from other elision
Object-lifetime-default elision is distinct from other forms of
elision; it always refers to some enclosing lifetime *present in the
surrounding type* (e.g., `&dyn Bar` expands to `&'a (dyn Bar + 'a)`.
If there is no enclosing lifetime, then it expands to `'static`.

Therefore, in an `impl Trait<Item = dyn Bar>` setting, we don't expand
to create a lifetime parameter for the `dyn Bar + 'X` bound.  It will
just be resolved to `'static`.

Annoyingly, the responsibility for this resolution is spread across
multiple bits of code right now (`middle::resolve_lifetimes`,
`lowering`). The lowering code knows that the default is for an object
lifetime, but it doesn't know what the correct result would be.
Probably this should be fixed, but what we do now is a surgical fix:
we have it generate a different result for elided lifetimes in a
object context, and then we can ignore those results when figuring out
the lifetimes that are captured in the opaque type.
use static as object-lifetime default for type XX in `Foo<Item=XX>`
Currently the default is "inherited" from context, so e.g.  `&impl
Foo<Item = dyn Bar>` would default to `&'x impl Foo<Item = dyn Bar +
'x>`, but this triggers an ICE and is not very consistent.

This patch doesn't implement what I expect would be the correct
semantics, because those are likely too complex. Instead, it handles
what I'd expect to be the common case -- where the trait has no
lifetime parameters.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:async-await-issue-62517 branch from afa4a2a to 7ee1af5 Aug 19, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@bors r=cramertj

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

📌 Commit 7ee1af5 has been approved by cramertj

Centril added a commit to Centril/rust that referenced this pull request Aug 19, 2019

Rollup merge of rust-lang#63376 - nikomatsakis:async-await-issue-6251…
…7, r=cramertj

use different lifetime name for object-lifetime-default elision

Introduce a distinct value for `LifetimeName` to use when this is a object-lifetime-default elision. This allows us to avoid creating incorrect lifetime parameters for the opaque types that result. We really need to overhaul this setup at some point! It's getting increasingly byzantine. But this seems like a relatively... surgical fix.

r? @cramertj

Fixes rust-lang#62517

bors added a commit that referenced this pull request Aug 19, 2019

Auto merge of #63715 - Centril:rollup-dga8qtp, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #63252 (Remove recommendation about idiomatic syntax for Arc::clone)
 - #63376 (use different lifetime name for object-lifetime-default elision)
 - #63620 (Use constraint span when lowering associated types)
 - #63699 (Fix suggestion from incorrect `move async` to `async move`.)
 - #63704 ( Fixed: error: unnecessary trailing semicolon)

Failed merges:

r? @ghost

@bors bors merged commit 7ee1af5 into rust-lang:master Aug 20, 2019

4 checks passed

pr Build #20190819.40 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details

Centril added a commit to Centril/rust that referenced this pull request Aug 20, 2019

Rollup merge of rust-lang#63209 - Centril:stabilize-async-await, r=cr…
…amertj

Stabilize `async_await` in Rust 1.39.0

Here we stabilize:
- free and inherent `async fn`s,
- the `<expr>.await` expression form,
- and the `async move? { ... }` block form.

Closes rust-lang#62149.
Closes rust-lang#50547.

All the blockers are now closed.

<details>
- [x] FCP in rust-lang#62149
- [x] rust-lang#61949; PR in rust-lang#62849.
- [x] rust-lang#62517; PR in rust-lang#63376.
- [x] rust-lang#63225; PR in rust-lang#63501
- [x] rust-lang#63388; PR in rust-lang#63499
- [x] rust-lang#63500; PR in rust-lang#63501
- [x] rust-lang#62121 (comment)
    - [x] Some tests for control flow (PR rust-lang#63387):
          - `?`
          - `return` in `async` blocks
          - `break`
    - [x] rust-lang#61775 (comment), i.e. tests for rust-lang#60944 with `async fn`s instead). PR in rust-lang#63383

</details>

r? @cramertj
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.