Skip to content

Regression from 1.20 to 1.21 stable -> stable with StreamingIterator code #45438

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

Closed
mikeyhew opened this issue Oct 22, 2017 · 8 comments
Closed
Assignees
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mikeyhew
Copy link
Contributor

The following code compiles with Rust version 1.20, but not with 1.21 and above:

pub trait StreamingIterator<'a>: 'a {
    type Item: 'a;

    fn next(&'a mut self) -> Option<Self::Item>;
}

pub trait StreamingIteratorExt: for<'a> StreamingIterator<'a> {}

impl<I> StreamingIteratorExt for I where
    I: for<'a>StreamingIterator<'a>
{}

Error in Rust 1.21+:

error[E0310]: the parameter type `I` may not live long enough
  --> src/lib.rs:14:9
   |
14 | impl<I> StreamingIteratorExt for I where
   |      -  ^^^^^^^^^^^^^^^^^^^^
   |      |
   |      help: consider adding an explicit lifetime bound `I: 'static`...
   |
note: ...so that the type `I` will meet its required lifetime bounds
  --> src/lib.rs:14:9
   |
14 | impl<I> StreamingIteratorExt for I where
   |         ^^^^^^^^^^^^^^^^^^^^

Note the Self: 'a bound on StreamingIterator. If I remove it, the error goes away.

@Mark-Simulacrum Mark-Simulacrum added I-nominated P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 22, 2017
@Mark-Simulacrum
Copy link
Member

@eddyb Could this be the same root cause as this regression even though it has a different version rang on it?

@eddyb
Copy link
Member

eddyb commented Oct 22, 2017

I doubt it, empty lifetime vs 'static. I don't know what happens here, this is HRTB for<'a> T: 'a which I think is T: 'static. cc @nikomatsakis

@TimNN TimNN added the C-bug Category: This is a bug. label Oct 22, 2017
@nikomatsakis nikomatsakis self-assigned this Nov 2, 2017
@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added P-high High priority and removed I-nominated P-high High priority labels Nov 2, 2017
@nikomatsakis
Copy link
Contributor

OK, so, I had not looked so closely at this example before. Now I see why the compiler is upset. There is this where-clause that I: for<'a> StreamingIterator<'a>. We currently enforce WF rules that state that, in this case, the where-clauses declared on the trait must hold. We translate this to imply that for<'a> I: 'a (since StreamingIterator<'a> has the where-clause Self: 'a). We then decide that the only way this could be true is if I: 'static. To some extent, the mystery is why this was ever accepted.

Now, as it happens, I think that this handling is not very good. The implied bounds and other work we've got going on would allow us to accept this program, I believe. But with the compiler's current handling of higher-ranked trait bounds, I think it's working as I expect.

@Mark-Simulacrum how far back can we bisect? Oh, we have a nightly bisection tool, right? I'd be interested (if the data can be readily obtained) to find out when this behavior changed.

@Mark-Simulacrum
Copy link
Member

We can bisect 90 days back by commit, and with some work by nightly (i.e., different bisectors -- mine can do only 90 days as of now, whereas nightlies should still be possible indefinitely). I don't have time in the short term to run that bisection but @est31 might be able to...

@est31
Copy link
Member

est31 commented Nov 3, 2017

Sad to hear that it was decided to delete PR artifacts after 90 days... I think it should be at least 6 months, maybe even a year until deletion.

Well we were lucky that this particular regression is inside that window. The regressing commit:

426711d 'Auto merge of #43786 - scalexm:issue-43784, r=nikomatsakis"'

@nikomatsakis
Copy link
Contributor

@est31 thanks! That was indeed a soundness fix. Seems plausible that this is expected fallout.

@nikomatsakis
Copy link
Contributor

Since this is the result of a bugfix, going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants