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

'missing lifetime specifier' regression in protobuf-1.0.1 #27248

Closed
brson opened this issue Jul 23, 2015 · 17 comments
Closed

'missing lifetime specifier' regression in protobuf-1.0.1 #27248

brson opened this issue Jul 23, 2015 · 17 comments
Assignees
Labels
I-needs-decision Issue: In need of a decision. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@brson
Copy link
Contributor

brson commented Jul 23, 2015

As reported by crater, Rust 1.3 nightlies appear to have introduced a regression in lifetime elision, probably related to default trait bounds.

cc @nikomatsakis @nrc @pnkfelix

Nominating in hopes a fix for this will appear before 1.3 beta.

@brson brson added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 23, 2015
@eefriedman
Copy link
Contributor

Reduces to:

trait T:'static {}
fn main() {
    fn _message_down_cast(_m: &T) -> &i8 {
        panic!()
    }
}

@eefriedman
Copy link
Contributor

RFC 141 says that "a lifetime position is anywhere you can write a lifetime in a type". According to that rule, fn(m: &(Sized+'static)) -> &i8 is ambiguous because there are two lifetime positions in the input, and this isn't a bug.

On the other hand, I'm pretty sure that isn't actually what we want, so we need a more precise rule which specifies exactly what lifetimes qualify as a "lifetime position".

Other random observations: nightly accepts the following, but stable doesn't:

fn main() {
    type T<'a> = &'a &'a i8;
    fn s(m: T) -> &i8 { m }
    static S: i8 = 1;
    static S2: &'static i8 = &S;
    s(&S2);
}

Neither nightly nor stable accept the following:

fn main() {
    type T<'a> = &'a &'static i8;
    fn s(m: T) -> &i8 { m }
    static S: i8 = 1;
    static S2: &'static i8 = &S;
    s(&S2);
}

@arielb1
Copy link
Contributor

arielb1 commented Jul 24, 2015

I think I accidentally introduced the regression as part of #26667. The previous version ignored object lifetime bounds - I should have caught this. I think the root cause here is that lifetime elision wasn't updated to respect object bounds.

@brson
Copy link
Contributor Author

brson commented Sep 10, 2015

@nikomatsakis Per irlo this seems to affect some crates. Is there any way we can get this fixed this week in time for beta, or if not sometime in the next cycle for a backport?

@brson brson added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Sep 10, 2015
@brson
Copy link
Contributor Author

brson commented Sep 11, 2015

cc @rust-lang/compiler

@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Sep 11, 2015
@nikomatsakis nikomatsakis self-assigned this Sep 11, 2015
@nikomatsakis
Copy link
Contributor

Sorry, I dropped the ball on compiler triage, but trying to get that wagon back on track now. Anyway, I'll look at this.

@nikomatsakis
Copy link
Contributor

Ah, yes, I remember this issue now. I think this relates to the discussion about '_, and this comment of mine in particular. Basically I agree with @eefriedman that technically there is no bug here -- rather, a bug was fixed. However, the behavior doesn't seem right nonetheless, for the reasons I argued on the thread. It seems to me that trait object lifetimes, which follow distinct defaulting rules, should not affect elision in quite the way they currently do.

@nikomatsakis
Copy link
Contributor

To be more specific, I think the rule we want is that object lifetimes only affect the elision rules if they are written explicitly, and not if defaults are used. I have to investigate how easy that will be to implement.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/lang -- see previous two comments

@nikomatsakis
Copy link
Contributor

also cc @wycats -- see previous two comments

@nikomatsakis
Copy link
Contributor

OK, as I thought, the elision determination is made based on the Ty<'tcx>, which has no information about whether the object lifetime was derived from a default or not. So this would be mildly tricky to implement. Basically, we would want to walk the AST instead of the type, I think. Of course we're already doing this walk when we convert, so it might be nice if we could gather the information that we need as we do it. I'll have to think about it. But it IS an interesting question as to precisely what rules we want here that we ought to settle first.

I think that approaching this basic on the position where the lifetime appears is probably best. However, one might also approach by making special treatment for 'static (after all, the object lifetime defaulting rules will either pick 'static or they will pick the lifetime of an enclosing & or some lifetime parameter placed on the trait, so if we behave differently around 'static that would also cover this case). For example, one might expect this to compile:

struct Foo<'s> { x: &'s i32 }
fn foo(x: &Foo<'static>) -> &Foo<'static> { x }
fn main() { }

But it does not today, since there are two lifetimes (in principle) one might choose in the return type: the anonymous one, or 'static. If we made the rules treat 'static as "non-conflicting" -- i.e., only pick 'static if there is nothing else to pick, it would address this case, but perhaps that is bit too creepy.

(Mostly just thinking through the possibilities here, not endorsing a plan of action.)

@nikomatsakis nikomatsakis added I-needs-decision Issue: In need of a decision. T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels Sep 11, 2015
@nikomatsakis
Copy link
Contributor

I'm going to re-nominate, but this time for T-lang, since I think we need a policy call.

@arielb1 arielb1 mentioned this issue Sep 15, 2015
@brson
Copy link
Contributor Author

brson commented Sep 15, 2015

This is going into stable.

@aturon
Copy link
Member

aturon commented Sep 15, 2015

Note: we need to discuss the logistics that let this slip through and try to make sure it doesn't happen again.

@arielb1
Copy link
Contributor

arielb1 commented Sep 15, 2015

If we want to change this, it probably requires an RFC. The rules as they are today are somewhat confusing, which is somewhat inevitable with the invisible trait lifetime bounds.

The new rules means that code like

fn foo(t: Box<Trait>) -> &u8

will compile with the interpretation fn(Box<Trait+'static>) -> &'static u8

Because duplicate lifetimes are ignored, this code can only cause an issue if either the user explicitly specifies an object lifetime bound (e.g. fn foo<'a>(a: Box<Trait+'a>, b: &u8) -> &u8), which I feel OK with breaking, or an object lifetime gets forced to 'static.

My preferred solution is to prevent elision to 'static, probably after a warning cycle with soft elision. Functions like fn foo(x: &'static u8) -> &u8 (fine under old and current, will break) itself feel confusing.

@nikomatsakis nikomatsakis removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 17, 2015
@nikomatsakis
Copy link
Contributor

We discussed in the language subteam meeting today and reached the conclusions that:

  1. The current behavior is correct in that it follows the RFC.
  2. Changes might be warranted but an RFC amendment is required, and further discussion would be helpful.

To that end, I'm going to close this issue and open (shortly) an RFC issue with a summary of the various cases that arise and the proposed solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-needs-decision Issue: In need of a decision. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants