Lifetimes bug on beta 1.31: the associated type may not live long enough #55756

mersinvald · 2018-11-07T14:43:55Z

A reduced test case (play):

#![crate_type="lib"]

pub trait Database<'a> {
    type Guard: 'a;
}

pub struct Stateful<'a, D: 'a>(&'a D);

impl<'b, D: for <'a> Database<'a>> Stateful<'b, D> {
    pub fn callee<'a>(&'a self) -> <D as Database<'a>>::Guard {
        unimplemented!()
    }
    pub fn caller<'a>(&'a self) -> <D as Database<'a>>::Guard {
        let state = self.callee();
        unimplemented!()
    }
}

Error diagnostic output in details block below

error[E0311]: the associated type `<D as Database<'_>>::Guard` may not live long enough
  --> src/lib.rs:14:13
   |
14 |         let state = self.callee();
   |             ^^^^^
   |
   = help: consider adding an explicit lifetime bound for `<D as Database<'_>>::Guard`
note: the associated type `<D as Database<'_>>::Guard` must be valid for the block suffix following statement 0 at 14:9...
  --> src/lib.rs:14:9
   |
14 | /         let state = self.callee();
15 | |         unimplemented!()
16 | |     }
   | |_____^
note: ...so that variable is valid at time of its declaration
  --> src/lib.rs:14:13
   |
14 |         let state = self.callee();
   |             ^^^^^

error[E0311]: the associated type `<D as Database<'_>>::Guard` may not live long enough
  --> src/lib.rs:14:13
   |
14 |         let state = self.callee();
   |             ^^^^^
   |
   = help: consider adding an explicit lifetime bound for `<D as Database<'_>>::Guard`
note: the associated type `<D as Database<'_>>::Guard` must be valid for the block at 13:63...
  --> src/lib.rs:13:63
   |
13 |       pub fn caller<'a>(&'a self) -> <D as Database<'a>>::Guard {
   |  _______________________________________________________________^
14 | |         let state = self.callee();
15 | |         unimplemented!()
16 | |     }
   | |_____^
note: ...so that references are valid when the destructor runs
  --> src/lib.rs:14:13
   |
14 |         let state = self.callee();
   |             ^^^^^

error[E0311]: the associated type `<D as Database<'_>>::Guard` may not live long enough
  --> src/lib.rs:14:21
   |
14 |         let state = self.callee();
   |                     ^^^^^^^^^^^^^
   |
   = help: consider adding an explicit lifetime bound for `<D as Database<'_>>::Guard`
note: the associated type `<D as Database<'_>>::Guard` must be valid for the method call at 14:21...
  --> src/lib.rs:14:21
   |
14 |         let state = self.callee();
   |                     ^^^^^^^^^^^^^
note: ...so type `<D as Database<'_>>::Guard` of expression is valid during the expression
  --> src/lib.rs:14:21
   |
14 |         let state = self.callee();
   |                     ^^^^^^^^^^^^^

error[E0311]: the associated type `<D as Database<'_>>::Guard` may not live long enough
  --> src/lib.rs:14:21
   |
14 |         let state = self.callee();
   |                     ^^^^^^^^^^^^^
   |
   = help: consider adding an explicit lifetime bound for `<D as Database<'_>>::Guard`
note: the associated type `<D as Database<'_>>::Guard` must be valid for the destruction scope surrounding statement at 14:9...
  --> src/lib.rs:14:9
   |
14 |         let state = self.callee();
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...so that references are valid when the destructor runs
  --> src/lib.rs:14:21
   |
14 |         let state = self.callee();
   |                     ^^^^^^^^^^^^^

error: aborting due to 4 previous errors

Original bug report

Versions

rustc 1.31.0-beta.4 (04da282 2018-11-01)
cargo 1.31.0-beta (efb7972a0 2018-10-24)

Expected behavior

Everything compiling correctly like it does on stable 1.30

Actual behavior

A lot of lifetime-related errors:

error[E0311]: the associated type `<D as trie::Database<'_>>::Guard` may not live long enough                                                                                                                                    
   --> /home/mike/dev/etcdev/sputnikvm/stateful/src/lib.rs:536:23                                                                                                                                                                
    |                                                                                                                                                                                                                            
536 |         let account = state.get(&address);                                                                                                                                                                                 
    |                       ^^^^^^^^^^^^^^^^^^^                                                                                                                                                                                  
    |                                                                                                                                                                                                                            
    = help: consider adding an explicit lifetime bound for `<D as trie::Database<'_>>::Guard`                                                                                                                                    
note: the associated type `<D as trie::Database<'_>>::Guard` must be valid for the method call at 536:23...                                                                                                                      
   --> /home/mike/dev/etcdev/sputnikvm/stateful/src/lib.rs:536:23                                                                                                                                                                
    |                                                                                                                                                                                                                            
536 |         let account = state.get(&address);                                                                                                                                                                                 
    |                       ^^^^^^^^^^^^^^^^^^^                                                                                                                                                                                  
note: ...so that a type/lifetime parameter is in scope here                                                                                                                                                                      
   --> /home/mike/dev/etcdev/sputnikvm/stateful/src/lib.rs:536:23                                                                                                                                                                
    |                                                                                                                                                                                                                            
536 |         let account = state.get(&address);                                                                                                                                                                                 
    |                       ^^^^^^^^^^^^^^^^^^^

It has been an issue in nightly 1.31 for a long time now, but I haven't reported the issue while it was just nightly. Now the regression is in beta channel and may land in stable, breaking existing code.

My own findings on the subject: regression is related to an old lifetime system, it's not reproducible with nll feature on, so on nightly the code can be compiled with nll, but not without it. Same holds for beta now except nll isn't quite there yet.

The subjected codebase: https://github.com/ETCDEVTeam/sputnikvm/tree/master/stateful

The text was updated successfully, but these errors were encountered:

pnkfelix · 2018-11-08T15:40:02Z

visiting for T-compiler triage. P-high. Assigning to self for initial investigation.

Mark-Simulacrum · 2018-11-09T02:17:21Z

Based on crater run, looks like this also affects:

etclient, cc @sorpaas
evm-stateful, cc @sorpaas
sputnikvm-stateful, cc @splix
ETCDEVTeam/sputnikvm-dev (cc @splix ?)
TheWaWaR/cita-create-genesis cc @TheWaWaR

pnkfelix · 2018-11-12T15:12:29Z

Bisected against when https://source.that.world/source/evm-rs.git stopped building.

Successfully built: rustc 1.30.0-nightly (4141a40 2018-09-25)
Error during compilation: rustc 1.30.0-nightly (ae7fe84 2018-09-26)

pnkfelix · 2018-11-12T15:13:52Z

Heres the bors merge record from that time frame:

% git log 4141a40..ae7fe84 --format=oneline --author=bors
ae7fe84 Auto merge of #54561 - matthiaskrgr:clippy_up, r=kennytm
c7df1f5 Auto merge of #54453 - nikomatsakis:nll-issue-53121-shred-outlives, r=pnkfelix
6846f22 Auto merge of #51946 - japaric:emit-stack-sizes, r=nikomatsakis
e783d2b Auto merge of #54199 - nikomatsakis:predicate_may_hold-failure, r=eddyb
c3a1a0d Auto merge of #53824 - ljedrz:begone_onevector, r=michaelwoerister
a2b27c1 Auto merge of #54497 - ralexstokes:stabilize_pattern_parentheses, r=nikomatsakis
beff387 Auto merge of #54548 - nrc:update, r=kennytm
0b2eae7 Auto merge of #54164 - mikhail-m1:54131, r=pnkfelix
92aff72 Auto merge of #54575 - pietroalbini:rollup, r=pietroalbini
%

pnkfelix · 2018-11-12T15:18:11Z

The most obvious culprit to my eye is PR #54453. That PR might have been intended to be restricted to NLL, I'm not sure (it touches code outside of rustc_mir, certainly). It does seem like the crate in question is not opting into NLL nor the 2018 editon.

Update: of course they aren't opting into NLL. This issue is tagged NLL-fixed-by-NLL

pnkfelix · 2018-11-12T15:58:32Z

Here is a reduced test case (play):

#![crate_type="lib"]

pub trait Database<'a> {
    type Guard: 'a;
}

pub struct Stateful<'a, D: 'a>(&'a D);

impl<'b, D: for <'a> Database<'a>> Stateful<'b, D> {
    pub fn callee<'a>(&'a self) -> <D as Database<'a>>::Guard {
        unimplemented!()
    }
    pub fn caller<'a>(&'a self) -> <D as Database<'a>>::Guard {
        let state = self.callee();
        unimplemented!()
    }
}

pnkfelix · 2018-11-12T16:08:28Z

I am not sure if I'm going to have time to look into this further before I go on leave (for ~2 weeks), and yet it seems like a potentially important thing to address. Unassigning self.

pnkfelix · 2018-11-12T16:21:07Z

I have now confirmed that PR #54453 is what injected this regression.

cc @nikomatsakis

nikomatsakis · 2018-11-13T17:45:16Z

I did a bit of investigation.

Some internal notes:

In the reduced test case, anyway, the problem seems to arise precisely because we are now a bit more flexible than we used to be. In particular, the type of state is something like <D as Database<'x>>::Guard but the region 'x is not directly constrained. The constraints arise because we have constraints like

<D as Database<'x>>::Guard: 'y

and we used to convert that into a rule that 'x: 'y, which led to us inferring that 'x must equal 'y.

There is a workaround therefore of adding an explicit type annotation (playground):

...
        let state: <D as Database<'a>>::Guard = self.callee();
...

I remember that when I was doing this I had some "contingency plans" for handling this scenario should it be a problem. I'll try to remember what they were =)

nikomatsakis · 2018-11-13T17:45:51Z

Also, it's interesting that NLL makes this test case pass. Off hand, I'm not sure entirely sure why that is.

nikomatsakis · 2018-11-13T18:36:25Z

OK, so I investigated NLL a bit. The reason is actually somewhat interesting.

NLL introduces an additional liveness check -- it says:

For any variable X live at the position P, all regions that appear in X's type must include the point P.

This rule is much "cruder" than the outlives check here. In particular, if you have a variable x: <D as Database<'a>>::Guard, then for it to pass this liveness check, 'a must include all points where x occurs.

We could adopt a similar distinction in lexical region checking with relative ease -- basically a "live at" relation that is more conservative than outlives. I'm not sure how well justified it is though.

(This is a bit weird because of eager normalization: if we were able to normalize <D as Database<'a>>::Guard to another type (e.g., u32), then we would not impose this requirement on 'a, because we would be representing its type as merely u32. This will change with lazy normalization. It is conceivable that this could cause programs to stop type-checking. I guess we'll worry about it then.)

nagisa · 2018-11-15T15:04:08Z

@nikomatsakis says they know what the problem is and hope to have a fix soon (today).

nikomatsakis · 2018-11-15T18:57:12Z

Hmm, so, I have a fix. Though looking more closely at the test, I think I see a nicer fix.

The problem is that we now observe two potentially applicable rules when trying to find bounds for <T as Database<'0>>::Guard:

<T as Database<'a>>::Guard: 'a // from the where clauses
for<'b> { <T as Database<'b>>::Guard: 'b } // from the trait definition

Because of this, we decide to take no action to influence inference, which means that '0 winds up unconstrained, leading to the ultimate error.

My current fix basically overrides this with a coarser rule (one which works fine, however, in this example).

A smarter fix would be to observe that the first rule (the one talking about 'a) is in fact "specialization" of the latter, in which case we could conclude that we should relate '0 to the bound we need.

I am trying to decide which approach is lowest risk -- the current fix, as I wrote, actually makes us compatible with how NLL is handling things. I am planning to overhaul this whole setup in the not-that-distinct future, since the current solver is essentially "unnecessarily stupid".

Some options:

Land my current fix, which is relatively simple and (I think) low risk.
- Perhaps follow-up with an attempt to create a test case and a FIXME?
Try to do the better fix

Given the deadline, I'm leaning towards the first path.

@eddyb

…es, r=eddyb remove "approx env bounds" if we already know from trait Alternative to rust-lang#55988 that fixes rust-lang#55756 -- smaller fix that I cannot see having (correctness) repercussions beyond the test at hand, and hence better for backporting. (Famous last words, I know.) r? @eddyb

nikomatsakis · 2018-11-19T09:22:15Z

Testing the more conservative fix from #56043:

nikomatsakis · 2018-11-19T09:23:42Z

@mersinvald

It has been an issue in nightly 1.31 for a long time now, but I haven't reported the issue while it was just nightly. Now the regression is in beta channel and may land in stable, breaking existing code.

btw, in the future, we would definitely appreciate bug reports against nightly! That is indeed part of the point of nightly, to help us find regressions before they make it into beta =)

nikomatsakis · 2018-11-19T09:25:49Z

I don't know how to test the final two cases that @Mark-Simulacrum cited -- I guess I need to checkout some github repositories? -- but based on the logs they seem like the same root problem and hence I expect it is fixed.

Mark-Simulacrum · 2018-11-19T13:09:41Z

Yes, you'd need to build https://github.com/ETCDEVTeam/sputnikvm-dev and https://github.com/TheWaWaR/cita-create-genesis to check them locally.

@eddyb

…es, r=eddyb remove "approx env bounds" if we already know from trait Alternative to rust-lang#55988 that fixes rust-lang#55756 -- smaller fix that I cannot see having (correctness) repercussions beyond the test at hand, and hence better for backporting. (Famous last words, I know.) r? @eddyb

TheWaWaR · 2018-11-20T03:36:47Z

@Mark-Simulacrum
https://github.com/TheWaWaR/cita-create-genesis
Confirm success build both on:

rustc 1.30.1 (1433507eb 2018-11-07)
rustc 1.32.0-nightly (5aff30734 2018-11-19)

mersinvald · 2018-11-20T12:44:41Z

@mersinvald

It has been an issue in nightly 1.31 for a long time now, but I haven't reported the issue while it was just nightly. Now the regression is in beta channel and may land in stable, breaking existing code.

btw, in the future, we would definitely appreciate bug reports against nightly! That is indeed part of the point of nightly, to help us find regressions before they make it into beta =)

Sure, I understand I should've report it once encountered. Would be less stressful for both parties. Thanks for fixing the issue!

mersinvald changed the title ~~Liferimes bug on beta 1.31: the associated type may not live long enough~~ Lifetimes bug on beta 1.31: the associated type may not live long enough Nov 7, 2018

oli-obk added A-borrow-checker Area: The borrow checker regression-from-stable-to-beta Performance or correctness regression from stable to beta. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. labels Nov 7, 2018

mersinvald mentioned this issue Nov 8, 2018

Allow CI failures on beta channel ETCDEVTeam/sputnikvm#368

Merged

pnkfelix self-assigned this Nov 8, 2018

pnkfelix added the P-high High priority label Nov 8, 2018

pnkfelix removed their assignment Nov 12, 2018

mersinvald mentioned this issue Nov 13, 2018

Lifetimes regression workaround ETCDEVTeam/sputnikvm#369

Merged

pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 15, 2018

nagisa assigned nikomatsakis Nov 15, 2018

This was referenced Nov 15, 2018

introduce "type must be valid for" into lexical region solver #55988

Closed

remove "approx env bounds" if we already know from trait #56043

Merged

bors closed this as completed in #56043 Nov 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lifetimes bug on beta 1.31: the associated type may not live long enough #55756

Lifetimes bug on beta 1.31: the associated type may not live long enough #55756

mersinvald commented Nov 7, 2018 •

edited by pnkfelix

Loading

pnkfelix commented Nov 8, 2018

Mark-Simulacrum commented Nov 9, 2018

pnkfelix commented Nov 12, 2018

pnkfelix commented Nov 12, 2018

pnkfelix commented Nov 12, 2018 •

edited

Loading

pnkfelix commented Nov 12, 2018

pnkfelix commented Nov 12, 2018

pnkfelix commented Nov 12, 2018

nikomatsakis commented Nov 13, 2018

nikomatsakis commented Nov 13, 2018

nikomatsakis commented Nov 13, 2018

nagisa commented Nov 15, 2018

nikomatsakis commented Nov 15, 2018

nikomatsakis commented Nov 19, 2018

nikomatsakis commented Nov 19, 2018

nikomatsakis commented Nov 19, 2018

Mark-Simulacrum commented Nov 19, 2018

TheWaWaR commented Nov 20, 2018 •

edited

Loading

mersinvald commented Nov 20, 2018

Lifetimes bug on beta 1.31: the associated type may not live long enough #55756

Lifetimes bug on beta 1.31: the associated type may not live long enough #55756

Comments

mersinvald commented Nov 7, 2018 • edited by pnkfelix Loading

Original bug report

Versions

Expected behavior

Actual behavior

pnkfelix commented Nov 8, 2018

Mark-Simulacrum commented Nov 9, 2018

pnkfelix commented Nov 12, 2018

pnkfelix commented Nov 12, 2018

pnkfelix commented Nov 12, 2018 • edited Loading

pnkfelix commented Nov 12, 2018

pnkfelix commented Nov 12, 2018

pnkfelix commented Nov 12, 2018

nikomatsakis commented Nov 13, 2018

nikomatsakis commented Nov 13, 2018

nikomatsakis commented Nov 13, 2018

nagisa commented Nov 15, 2018

nikomatsakis commented Nov 15, 2018

nikomatsakis commented Nov 19, 2018

nikomatsakis commented Nov 19, 2018

nikomatsakis commented Nov 19, 2018

Mark-Simulacrum commented Nov 19, 2018

TheWaWaR commented Nov 20, 2018 • edited Loading

mersinvald commented Nov 20, 2018

mersinvald commented Nov 7, 2018 •

edited by pnkfelix

Loading

pnkfelix commented Nov 12, 2018 •

edited

Loading

TheWaWaR commented Nov 20, 2018 •

edited

Loading