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

Non-`'static` lifetime inferred for return position `impl Trait` (fixed in beta) #65582

Closed
ecstatic-morse opened this issue Oct 19, 2019 · 21 comments

Comments

@ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Oct 19, 2019

On beta and nightly, the following code is correctly rejected since the impl Trait in return position gets 'static by default. However, on the current stable (1.38), it was wrongfully accepted. Replacing the trait object with another type seems to make the error appear on all versions.

Playground

use std::any::Any;

fn drain_dyn(v: &mut Vec<Box<dyn Any>>) -> impl Iterator<Item = Box<dyn Any>> {    
    v.drain()
}

This has already been fixed, so I'm not sure what the procedure is here. Observed in the crater run for 1.39 (#64962) and split out from #65527.

@ecstatic-morse ecstatic-morse changed the title Non-`'static` lifetime derived for return position `impl Trait` (fixed in beta) Non-`'static` lifetime assigned to return position `impl Trait` (fixed in beta) Oct 19, 2019
@ecstatic-morse ecstatic-morse changed the title Non-`'static` lifetime assigned to return position `impl Trait` (fixed in beta) Non-`'static` lifetime inferred for return position `impl Trait` (fixed in beta) Oct 19, 2019
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Oct 19, 2019

Nominating for compiler team -- I suspect we just want to close this, but wanted to raise this at least so we're aware. It's possible we can track down the fix and backport to stable, though at this point we're at 4 weeks out so it may not be worth it.

@Centril

This comment has been minimized.

Copy link
Member

@Centril Centril commented Oct 19, 2019

I'm betting this is #62849 (this should be checked and then that PR should be stable-nominated).

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

@jonas-schievink my understanding, based on the description, is that this is stable-to-stable regression that already has a fix that has ridden the trains to beta, not a stable-to-beta regression.

  • Update: that understanding was flawed, as you can see from my comments that follow.
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

(unless the tagging as a regression is due to it being a user observable change in behavior from "stable rust". I need to track down what the behavior of this has been over time...)

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

If this is "just" a change in observable behavior, where we had "always" accepted the code previously, then I don't think there is any motivation to do a backport-to-stable; there is no need to speed up how soon the change lands. (If anything, we should be trying to evaluate the fallout from the change, and deciding if we need to do a revert of the change on beta. But I think the crater report that preceded this bug being filed is telling us that such a reversion is not necessary...)

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

Okay, so it looks like if you revise the test to work in older versions of Rust (by using Box<Trait> instead of Box<dyn Train> and fixing the call to drain to pass a range), then this has been accepted by the compiler ever since impl Trait in return position was stabilized, back in Rust 1.26.0.

So then the question is whether this is an old known bug in impl Trait; if not, then the question is if wheteher this fallout from whatever change injected this (presumably PR #62849) is acceptable/desirable.

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

I think resolving the questions in the previous comment is part of T-lang, not just T-compiler. Tagging with T-lang and leaving nomination tag in place for discussion at T-lang meeting.

@pnkfelix pnkfelix added the T-lang label Oct 24, 2019
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

(also, is this related to #53613 and #51525 ?)

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

((Or maybe PR #62849 was always intended to be something that papers over #53613 and #51525, and it just didn't link to those tickets...?))

  • I guess issue #61949 (which spawned off PR #62849), did link to #53613.
  • So I guess this was anticipated fallout?
  • Still would be good to make sure that both T-compiler and T-lang are aware of it.
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

triage: P-high. Leaving nomination tag in place, for both T-compiler and T-lang.

@pnkfelix pnkfelix added the P-high label Oct 24, 2019
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 24, 2019

My two cents:

This was anticipated fallout and yes we were fixing an old bug, basically. One thing I can't tell is whether there was fallout from this change in terms of things breaking in the wild? The comment by @ecstatic-morse suggests yes, but I couldn't immediately identify which things broke.

@jonas-schievink

This comment has been minimized.

Copy link
Member

@jonas-schievink jonas-schievink commented Oct 24, 2019

According to #65527 (comment), soketto 0.2.0 broke

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 24, 2019

Hey @twittner -- it looks like soketto 0.2.0 was affected by a recent bug fix that we did as part of the async-await work. Let us know if you need any help fixing the fallout.

@Centril Centril removed the I-nominated label Oct 24, 2019
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 24, 2019

Looking more closely at this example, I am not convinced that this is related to #62849. If it were, I would expect an error message like

return type cannot contain a projection or Self that references lifetimes from a parent scope

That said, this does really look like a bug fix -- I am pretty unclear on what fixed it, but I'm glad it got fixed! (Maybe we want to mark this as E-needs-test, even?)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 24, 2019

It also used to compile with Vec:

use std::any::Any;

fn drain_dyn(v: &mut Vec<Box<dyn Any>>) -> impl Iterator<Item = Box<dyn Any>> {
    v.drain(..)
}

compiles on stable, but not nightly

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse commented Oct 24, 2019

#62849 also had a full crater run, which should have caught this (although sometimes the crate lists are updated).

@twittner

This comment has been minimized.

Copy link

@twittner twittner commented Oct 24, 2019

Thanks @nikomatsakis! This was fixed by paritytech/soketto#1 and published as version 0.2.3 of soketto on 2019-08-19.

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Oct 24, 2019

(so was this in fact "fixed" aka injected by PR #63376 ? that might make sense... )

@Centril

This comment has been minimized.

Copy link
Member

@Centril Centril commented Oct 24, 2019

Heh; I've even left a comment wrt. soketto in that PR. 😆

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 25, 2019

OK, I'm going to close this as "expected bug fix with no further action needed". Thanks all!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 25, 2019

Note that the PR includes a text that seems like the same basic scenario:

src/test/ui/object-lifetime/object-lifetime-default-dyn-binding-nonstatic1.rs

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.