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

Temporary lifetimes sometimes yield surprising errors #46413

Closed
stephaneyfx opened this issue Nov 30, 2017 · 22 comments
Closed

Temporary lifetimes sometimes yield surprising errors #46413

stephaneyfx opened this issue Nov 30, 2017 · 22 comments
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@stephaneyfx
Copy link
Contributor

stephaneyfx commented Nov 30, 2017

UPDATE: The main remaining issue seems to be that the NLL checker is correctly identifying cases where temporary lifetimes are acting in surprising ways -- some of these were overlooked by the borrow checker. The only fix is to adjust the temporary lifetime rules, which would be technically backwards incompatible, but maybe something we could get away with. See this comment and follow-ups for more details. -- @nikomatsakis


See the second code snippet in comment below.


Old text

In the following code, f compiles but g does not. It looks like the borrow of a gets a different lifetime between these 2 functions.
Playground

fn f() {
    let a = vec![()];
    let b = (0 .. 1).map(|_| (|| a.iter()));
    let _c: Vec<_> = b.collect();
}

fn g() {
    let a = vec![()];
    let _c: Vec<_> = (0 .. 1).map(|_| (|| a.iter())).collect();
}

fn main() {}
Compiling playground v0.0.1 (file:///playground)
error[E0597]: `**a` does not live long enough
  --> src/main.rs:9:43
   |
9  |     let _c: Vec<_> = (0 .. 1).map(|_| (|| a.iter())).collect();
   |                                        -- ^                  - borrowed value only lives until here
   |                                        |  |
   |                                        |  does not live long enough
   |                                        capture occurs here
10 | }
   | - borrowed value needs to live until here

error: aborting due to previous error

error: Could not compile `playground`.

To learn more, run the command again with --verbose.
@stephaneyfx
Copy link
Contributor Author

stephaneyfx commented Dec 15, 2017

I'm not sure if this is related. Someone on IRC got a borrow error I couldn't explain and this is a minimized reproduction of the error: playground

pub struct Statement;

pub struct Rows<'stmt>(&'stmt Statement);

impl<'stmt> Drop for Rows<'stmt> {
    fn drop(&mut self) {}
}

impl<'stmt> Iterator for Rows<'stmt> {
    type Item = String;

    fn next(&mut self) -> Option<Self::Item> {
        None
    }
}

fn get_names() -> Option<String> {
    let stmt = Statement;
    let mut rows = Rows(&stmt);
    rows.map(|row| row).next()
    // Replacing the previous line with the following works:
    // let x = rows.map(|row| row).next();
    // x
    //
    // Removing the map works too as does removing the Drop impl.
}

fn main() {}

Dubious error:

   Compiling playground v0.0.1 (file:///playground)
error[E0597]: `stmt` does not live long enough
  --> src/main.rs:26:1
   |
19 |     let mut rows = Rows(&stmt);
   |                          ---- borrow occurs here
...
26 | }
   | ^ `stmt` dropped here while still borrowed
   |
   = note: values in a scope are dropped in the opposite order they are created

@pietroalbini pietroalbini added the A-borrow-checker Area: The borrow checker label Jan 23, 2018
@pietroalbini
Copy link
Member

This should be fixed with non-lexical lifetimes (playground link).

I don't think it's worth fixing the issue separately, since (if I understood correctly) NLL are planned to be stabilized this year. Closing this.

@stephaneyfx
Copy link
Contributor Author

@pietroalbini : Thank you for looking into it. The second example still does not compile with NLL. Maybe they are unrelated and I should open another issue? See playground:

#![feature(nll)]

pub struct Statement;

pub struct Rows<'stmt>(&'stmt Statement);

impl<'stmt> Drop for Rows<'stmt> {
    fn drop(&mut self) {}
}

impl<'stmt> Iterator for Rows<'stmt> {
    type Item = String;

    fn next(&mut self) -> Option<Self::Item> {
        None
    }
}

fn get_names() -> Option<String> {
    let stmt = Statement;
    let rows = Rows(&stmt);
    rows.map(|row| row).next()
    // Replacing the previous line with the following works:
    // let x = rows.map(|row| row).next();
    // x
    //
    // Removing the map works too as does removing the Drop impl.
}

fn main() {}

@pietroalbini
Copy link
Member

Sorry, I didn't see the other snippet :(
Reopening.

@pietroalbini pietroalbini reopened this Jan 23, 2018
@pietroalbini pietroalbini added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Jan 23, 2018
@pietroalbini
Copy link
Member

cc @nikomatsakis?

@stephaneyfx
Copy link
Contributor Author

@pietroalbini : No problem ☺️. I should probably have opened a separate issue as they may not be related after all. Thank you again!

@nikomatsakis
Copy link
Contributor

Seems like the second issue is a bug. I'll file it and dig in a bit soon.

@nikomatsakis nikomatsakis added this to the NLL: Valid code works milestone Jan 23, 2018
@nikomatsakis
Copy link
Contributor

Ok so I dug into this error a bit. The cause is the ordering of this "tail bit" in the MIR:

    | Live variables on entry to bb10: [_6 (drop)]
    bb10: {
        StorageDead(_2);
        StorageDead(_1);
        drop(_6) -> [return: bb11, unwind: bb1];
    }

Here, _1 represents the stmts variable. _6 represents the temporary from calling map, I think:

    let mut _6: std::iter::Map<Rows<'_>, [closure@/home/nmatsakis/tmp/issue-46413.rs:22:14: 22:23]>;

So, MIR borrowck is not wrong to complain -- the destructor is indeed running with access to memory whose storage is dead. The question is more why the data is getting generated in this way. At first glance, the order looks wrong there, but the lifetime of temporaries in the tail expression of a block has some subtle points, and I'm fairly sure this is intentional (though I wonder if we could alter it)

cc @pnkfelix -- do you remember?

@nikomatsakis
Copy link
Contributor

The other question is why non-MIR-based borrowck accepts this code

@nikomatsakis nikomatsakis added the NLL-complete Working towards the "valid code works" goal label Mar 14, 2018
@nikomatsakis
Copy link
Contributor

Nominated for Lang Team discussion. It's not clear what is best way forward, but any change to the underlying MIR is changing runtime semantics, which is something we should do only with great caution. That said, I think that the precise of temporaries being dropped here seems suboptimal. Maybe a lint is a good place to start -- which might also help us to "scope" how big of an impact a tweak here would have.

@nikomatsakis nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 3, 2018
@stephaneyfx
Copy link
Contributor Author

For what it's worth, the playground linked in my previous comment causes an ICE in nightly (2018-04-01).

@stephaneyfx
Copy link
Contributor Author

This may be the same issue (lifetime error in stable and ICE in nightly). Note that adding a semicolon after Foo(&i).foo() is enough to make the code compile. Playground:

// #![feature(nll)]

struct Foo<'a>(&'a i32);

impl<'a> Drop for Foo<'a> {
    fn drop(&mut self) {}
}

impl<'a> Foo<'a> {
    fn foo(&self) {}
}

fn bar() {
    let i = 3;
    Foo(&i).foo()
}

fn main() {
    bar();
}

@nikomatsakis
Copy link
Contributor

I believe the ICE may be fixed by @spastorino's PR #49808

@spastorino
Copy link
Member

Yes, this is fixed by #49808

The output for your code is ...

[santiago@archlinux tmp]$ rustc +stage1 test.rs 
error[E0597]: `i` does not live long enough
  --> test.rs:15:9
   |
15 |     Foo(&i).foo()
   |     ----^^-
   |     |   |
   |     |   borrowed value does not live long enough
   |     borrow may end up in a temporary, created here
16 | }
   | -
   | |
   | borrowed value only lives until here
   | temporary later dropped here, potentially using the reference

error: aborting due to previous error

For more information about this error, try `rustc --explain E0597`.

@KiChjang
Copy link
Member

Isn't this also a case of the drop check rule in action? c.f. #22321

@scuzzycheese
Copy link

scuzzycheese commented May 27, 2018

Is this the same bug?
I see that my lock in the first example is not freed (mutex does not go out of scope) until the end of the while loop, whereas int he second example, it's freed in the block fetching the value for the while loop:
permalink

use std::sync::Mutex;

fn main() {
   let lock = Mutex::new(Some(10));

   while let Some(_value) = { *lock.lock().unwrap() } {
       match lock.try_lock() {
           Ok(val) => {
               println!("Achieved lock: {}", val.unwrap());
           }
           Err(e) => {
               println!("Error: {}", e);
           }
       }
       break;
   }

   while let Some(_value) = {
       let val = lock.lock().unwrap();
       *val
   } {
       match lock.try_lock() {
           Ok(val) => {
               println!("Achieved lock: {}", val.unwrap());
           }
           Err(e) => {
               println!("Error: {}", e);
           }
       }
       break;
   }
}

@nikomatsakis nikomatsakis removed this from the NLL: Valid code works milestone Jul 24, 2018
@nikomatsakis nikomatsakis added the NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. label Jul 24, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Sep 13, 2018

This issue was discussed (or at least alluded to) in today's compiler team meeting. I'm going to treat addressing it as part of overall potential NLL work, so I'm 1. tagging with A-NLL, and 2. (already done!) putting on the Release milestone.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 8, 2018

Visited for T-compiler triage. Removing NLL-fixed-by-NLL issues from the Release milestone, but continue to leave them open as issues themselves. That seems like it strikes the right balance here.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 9, 2018

At this point I think the diagnostic improvements added by NLL are all that particular feature (NLL) is going to do on its own to improve things here.

There's obviously still some potential work here to be done, in any of the following areas:

  1. better educate developers about the rvalue-temporary lifetime semantics,
  2. provide diagnostic feedback on cases that might behave in ... surprising ways ... at runtime.

See also #37612 for what I believe to be an instance of this bug. The comment thread there is well-worth reading; it includes arguments for why one might change the temporary lifetime semantics, but it also includes examples of why we want the semantics we have today.

Having said that, since I don't think we're going to see further movement on this issue via NLL, I'm going to remove A-NLL from this issue.

@pnkfelix pnkfelix removed A-NLL Area: Non-lexical lifetimes (NLL) NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. labels Nov 9, 2018
@pnkfelix pnkfelix removed their assignment Nov 9, 2018
@matthewjasper matthewjasper added A-diagnostics Area: Messages for errors, warnings, and lints and removed NLL-deferred labels Dec 1, 2018
@estebank
Copy link
Contributor

The current output for the code in the first comment is:

error[E0597]: `stmt` does not live long enough
  --> src/main.rs:19:25
   |
19 |     let mut rows = Rows(&stmt);
   |                         ^^^^^ borrowed value does not live long enough
20 |     rows.map(|row| row).next()
   |     ------------------- a temporary with access to the borrow is created here ...
...
26 | }
   | -
   | |
   | `stmt` dropped here while still borrowed
   | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `std::iter::Map<Rows<'_>, [closure@src/main.rs:20:14: 20:23]>`
   |
   = note: The temporary is part of an expression at the end of a block. Consider forcing this temporary to be dropped sooner, before the block's local variables are dropped. For example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block.

It suggests exactly one of the valid fixes. @rust-lang/wg-diagnostics do we think we can add any improvements here?

@pnkfelix it feels to me that this could be closed now, right?

@pnkfelix
Copy link
Member

pnkfelix commented Sep 20, 2019

@estebank yes, at this point I think the diagnostics are as good as we might expect them to get (and they are much improved over what this bug was reporting when it was originally filed).

The main movement I could imagine at this point would be to actually change the dynamic semantics to try to make them have temporary lifetimes that are more "intuitive." But that would be a pretty serious (and probably breaking, depending on whether it changes when destructors run) change.

Since thread on this issue is not particularly enlightening with regards to such a hypothetical change (compared to issues such as #15023, #37612, or this summary comment on #21114), and therefore I do not see value in leaving this issue open.

Closing as fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

9 participants