Temporary lifetimes sometimes yield surprising errors

@nikomatsakis

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.

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

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.

@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() {}

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

@nikomatsakis

cc @nikomatsakis?

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

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

@pnkfelix

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?

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.

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:

better educate developers about the rvalue-temporary lifetime semantics,
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

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?

@estebank

@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.

Navigation Menu

Repository navigation

Temporary lifetimes sometimes yield surprising errors #46413

20 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Temporary lifetimes sometimes yield surprising errors #46413

Description

Activity

stephaneyfx commented on Dec 15, 2017

emilyalbini commented on Jan 23, 2018

stephaneyfx commented on Jan 23, 2018

emilyalbini commented on Jan 23, 2018

emilyalbini commented on Jan 23, 2018

stephaneyfx commented on Jan 23, 2018

nikomatsakis commented on Jan 23, 2018

nikomatsakis commented on Feb 1, 2018

20 remaining items

pnkfelix commented on Nov 8, 2018

pnkfelix commented on Nov 9, 2018

estebank commented on May 25, 2019

pnkfelix commented on Sep 20, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions