new scoping rules for safe dtors can yield spurious semi-colon or trailing unit expr

Spawned off of #21022, #21972

There are a number of situations where a trailing expression like a for-loop ends up being treated by the region lifetime inferencer as requiring a much longer lifetime assignment than what you would intuitively expect.

A lot of the relevant cases have been addressed. In fact, its gotten to the point that I want to have a summary comment up here that specifies the status for each example. Each demo should link to a playpen (using AST-borrowck when that has a checkmark, and NLL when AST-borrowck does not have a checkmark). Some rows also include a link to the comment where it was pointed out.

A checkmark (✅) is used for cases where either 1. code is now accepted, or 2. code is (still) rejected but has a clear diagnostic explaining why.
The column marked 2018 is testing here the 2018 edition, where migration mode gets most of the benefits from NLL. They will also eventually trickle down to the 2015 edition when we switch that to use NLL as well.
The magnifying glass (🔍) is for cases where we issue a diagnostic suggesting that one add a local binding or a semi-colon (depending on whether the value is needed or not), explaining that the temporaries (under the current language semantics) are being dropped too late without it. This sort of case is, to my eye, acceptable.
The column marked NLL is for cases where you need to opt into #![feature(nll)] (or use the synonymous command line options -Z borrowck=mir -Z two-phase-borrows) to get the nicest output. In most cases the 2018 output is the same, so I didn't fill in the column in those cases.

Example on playpen	2015	2018	NLL	source
io::stdin demo 1	✅	✅
io::stdin demo 2	🤢	✅
alloc::rc demo	✅	✅
ebfull drain demo	🤢	✅		#21114 (comment)
felispere stdio demo	🤢	✅		#21114 (comment)
drbawb demo	✅	✅		#21114 (comment)
niconii demo	🤢	🔍		#21114 (comment)
kixunil demo	🤢	✅		#21114 (comment)
thiez demo 1	🤢	✅		#21114 (comment)
thiez demo 2	🤢	🔍		#21114 (comment)
shepmaster demo	🤢	✅		#21114 (comment)
jonas-schievink demo	🤢	🤢	✅	#42574
stephaneyfx demo 1	🤢	🔍		#46413 (comment)
stephaneyfx demo 2	🤢	🔍		#46413 (comment)

More details for issue follow.

A simple example of this (from examples embedded in the docs for io::stdio):

    use std::io;

    let mut stdin = io::stdin();
    for line in stdin.lock().lines() {
        println!("{}", line.unwrap());
    };

Another example, this time from an example for alloc::rc (where this time I took the time to add a comment explaining what I encountered):

fn main() {
     let gadget_owner : Rc<Owner> = Rc::new(
            Owner {
                name: "Gadget Man".to_string(),
                gadgets: RefCell::new(Vec::new())
            }
    );

    let gadget1 = Rc::new(Gadget{id: 1, owner: gadget_owner.clone()});
    let gadget2 = Rc::new(Gadget{id: 2, owner: gadget_owner.clone()});

    gadget_owner.gadgets.borrow_mut().push(gadget1.clone().downgrade());
    gadget_owner.gadgets.borrow_mut().push(gadget2.clone().downgrade());

    for gadget_opt in gadget_owner.gadgets.borrow().iter() {
        let gadget = gadget_opt.upgrade().unwrap();
        println!("Gadget {} owned by {}", gadget.id, gadget.owner.name);
    }

    // This is an unfortunate wart that is a side-effect of the implmentation
    // of new destructor semantics: if the above for-loop is the final expression
    // in the function, the borrow-checker treats the gadget_owner as needing to
    // live past the destruction scope of the function (which of course it does not).
    // To work around this, for now I am inserting a dummy value just so the above
    // for-loop is no longer the final expression in the block.
    ()
}

Luckily for #21022, the coincidence of factors here is not very frequent, which is why I'm not planning on blocking #21022 on resolving this, but instead leaving it as something to address after issues for the alpha have been addressed.

(I'm filing this bug before landing the PR so that I can annotation each of the corresponding cases with a FIXME so that I can go back and address them after I get a chance to investigate this properly.)

@nikomatsakis

The solution here seems to be to adjust the infer lifetimes for temporaries generated from the <iter> in for <pat> in <iter> { ... }.

I have a patch to do this on my branch, and so far it seems to work, but @nikomatsakis has pointed out to me that I may be better off waiting until after #20790 lands.

(Nonetheless, there may be similar or related issues in other syntactic forms, such as if let <pat> = <expr> { ... }. At the very least, I will be able to use this bug as a reference point in FIXME notes.)

fixes for new rules for lifetimes and destructors.

(This was largely addressed by #21984. I am not sure whether there is much more we can actually do here. You can search for FIXME's in this commit bdb9f3e for most of the fallout that was caused by this in the end (its mostly with if let, along with one semicolon I had to add to a test; it was not that bad.)

I think I'm having a similar issue:

use std::collections::HashMap;
use std::sync::Mutex;

fn i_used_to_be_able_to(foo: &Mutex<HashMap<usize, usize>>) -> Vec<(usize, usize)> {
    let mut foo = foo.lock().unwrap();

    foo.drain().collect()
}

fn but_after_nightly_update_now_i_gotta(foo: &Mutex<HashMap<usize, usize>>) -> Vec<(usize, usize)> {
    let mut foo = foo.lock().unwrap();

    return foo.drain().collect();
}

fn main() {}

I get the above mentioned error when running rustc rustc 1.0.0-nightly (3ef8ff1f8 2015-02-12 00:38:24 +0000)
With the following code:

    std::old_io::stdio::stdin().lock().lines().map( |line| {
        line.unwrap().trim().to_string()
    }).collect()

When using rustc 1.0.0-nightly (00df3251f 2015-02-08 23:24:33 +0000) I don't get the error.

Split usage of stdin() into multiple lets

@felipesere

@felipesere Your example, I think, is running afoul of the failure of the region system's model to precisely track the destruction order for r-value temporaries; see #22323.

After rewriting your example to accommodate that short-coming, we have this:

fn lines() -> Vec<String> {
    let mut stdin = std::old_io::stdio::stdin();
    let mut locked = stdin.lock();
    locked.lines().map( |line| {
        line.unwrap().trim().to_string()
    }).collect()
}

which compiles for me. So I do not think this is an instance of this ticket, but rather of #22323

@ebfull

@ebfull your example is very interesting. (I am not yet sure whether the underlying cause is the same as the one that led to this ticket, but I definitely would like to fix it.)

cc #22321

I think I'm bumping up against this, I have a function that acquires a RwLock in a trailing expression. Though it's a match expression, not a for loop.

The code looks similar to this:

type Context = Arc<RwLock<HashMap<Uuid, User>>>;

fn disconnect_user(ctx: Context, uid: Uuid) -> Result<String, String> {
    // ....
   match ctx.write().unwrap().remove(&uid) {
         Some(user) => { Ok(format!("user removed")) },
         None => { Err(format!("user not removed")) },
    }
}

Which yields this error on the latest nightly (b63cee4a1 2015-02-14):

src/supervisor/handlers/connections.rs:162:8: 162:11 error: `ctx` does not live long enough
src/supervisor/handlers/connections.rs:162      match ctx.users.write().ok().unwrap().remove(&uid) {
                                                      ^~~
src/supervisor/handlers/connections.rs:153:78: 186:2 note: reference must be valid for the destruction scope surrounding block at 153:77...
src/supervisor/handlers/connections.rs:153 pub fn disconnect_user(req: &Request, env: &mut Env) -> Result<String,String>{
src/supervisor/handlers/connections.rs:154      let ctx = env.get::<Arc<Context>>().unwrap().clone();
src/supervisor/handlers/connections.rs:155      let io  = env.get::<Sender<Envelope>>().unwrap().clone();
src/supervisor/handlers/connections.rs:156      let uid = req.uid.clone();
src/supervisor/handlers/connections.rs:157 
src/supervisor/handlers/connections.rs:158      let reason = req.payload.as_ref().and_then(|pl| {
                                           ...
src/supervisor/handlers/connections.rs:154:54: 186:2 note: ...but borrowed value is only valid for the block suffix following statement 0 at 154:53
src/supervisor/handlers/connections.rs:154      let ctx = env.get::<Arc<Context>>().unwrap().clone();
src/supervisor/handlers/connections.rs:155      let io  = env.get::<Sender<Envelope>>().unwrap().clone();
src/supervisor/handlers/connections.rs:156      let uid = req.uid.clone();
src/supervisor/handlers/connections.rs:157 
src/supervisor/handlers/connections.rs:158      let reason = req.payload.as_ref().and_then(|pl| {
src/supervisor/handlers/connections.rs:159              json::decode(&pl[]).ok()
                                           ...
error: aborting due to previous error

I seem to be able to work around it in two ways. I can either bind the result of the match exprsession to a variable and return that instead, or I can bind context's lock guard to a variable and perform the match on that.

@drbawb

@drbawb yes, I suspect this is the same issue. I doubt I will be fixing this any time soon (i.e., I do not think it is a 1.0 blocker issue), so while I do hope to fix it eventually, I think you will need to live with the workarounds for now.

@pnkfelix

A full reproduction of @pnkfelix 's early example compiles today without the ():

use std::rc::Rc;
use std::rc::Weak;
use std::cell::RefCell;

struct Owner {
    name: String,
    gadgets: RefCell<Vec<Weak<Gadget>>>,
}

struct Gadget {
    id: i32,
    owner: Rc<Owner>,
}

fn main() {
    let gadget_owner : Rc<Owner> = Rc::new(
        Owner { 
            name: "Gadget Man".to_string(),
            gadgets: RefCell::new(Vec::new())
        }
        );

    let gadget1 = Rc::new(Gadget{id: 1, owner: gadget_owner.clone()});
    let gadget2 = Rc::new(Gadget{id: 2, owner: gadget_owner.clone()});

    gadget_owner.gadgets.borrow_mut().push(Rc::downgrade(&gadget1.clone()));
    gadget_owner.gadgets.borrow_mut().push(Rc::downgrade(&gadget2.clone()));

    for gadget_opt in gadget_owner.gadgets.borrow().iter() {
        let gadget = gadget_opt.upgrade().unwrap();
        println!("Gadget {} owned by {}", gadget.id, gadget.owner.name);
    }
}

That said, obviously this issue is about a bit more than that, and as he said:

(I'm filing this bug before landing the PR so that I can annotation each of the corresponding cases with a FIXME so that I can go back and address them after I get a chance to investigate this properly.)

@pnkfelix where does this stand today?

@steveklabnik

@steveklabnik it seems like the cases of interest here got fixed via independent changes elsewhere in rustc

It might be interesting to track down which PR did this, but given that we have made a number of refinements to the region system since this was filed, I'm not astounded that it was fixed .... merely mildly surprised

@pnkfelix

Thanks @pnkfelix for the reply and references!

I think the problem with this case is that the return value is just a "u8". As long as the type is "Copy", it is hard to see why the lifetime of any temporary where the final value came from needs to be extended. Along this line, I wonder if extending the lifetime of the entire tail expression may be too broad. Does it make sense to extended (the lifetime of) only the last object created by the tail expression rather than the whole expression?

I can find descriptions of how C++ deals with return objects and temporaries using copy constructors. The way I understand it is that it is basically doing a "placement new" into an ancestor stack frame/scope. Is there a similar treatise of how Rust returns objects or bubbles up the value of a {} block? I doubt whether "Clone" is involved.

I think there is still a common belief that return <expr>; at the end of a function is the same as <expr>. I hope this issue will bring forth a section in Rust documentation on why they are not the same.

There's discussion about temporary lifetimes in the Reference but having an explicit section on this would make sense.

Edit: Filed rust-lang/reference#452

@kwanyyoss

@kwanyyoss wrote:

I think the problem with this case is that the return value is just a "u8". As long as the type is "Copy", it is hard to see why the lifetime of any temporary where the final value came from needs to be extended.

Part of the goal here is to have temporary r-value rules that are uniform, in the sense that they come directly from the syntactic structure from the program, and not from e.g. the types that have been inferred nor the results of the borrow-check analysis.

Here's further elaboration of this point:

Does it make sense to extended (the lifetime of) only the last object created by the tail expression rather than the whole expression?

I don't know. My most immediate reaction to that is that it would really complicate encoding the cases where you need those intermediate values to survive long enough.

(And of course, the interpretation of your question depends very much on how one interprets the phrase "the last object created" ...)

I wrote up above:

My most immediate reaction to that is that it would really complicate encoding the cases where you need those intermediate values to survive long enough.

As an attempt to illustrate this: people already complain about the fact that they are forced to add extra let-bindings of intermediate results in cases like this:

fn does_not_work() {
    let x = &std::env::args().collect::<Vec<_>>().as_slice()[1..];
    println!("{:?}", x);
}

fn works() {
    println!("{:?}", &std::env::args().collect::<Vec<_>>().as_slice()[1..]);
}

(See #15023 )

I suspect your proposal would further exacerbate this already present problem. But I have not done any experiment to validate that suspicion.

I actually think this issue as described is effectively resolved now, under NLL at least.

There are related topics in terms of temporary lifetimes being unintuitive (#37612, #46413) and proposals to improve them (#15023).

But this specific issue was about how the dropck rules caused a number of cases to appear to regress, and as far as I can tell, NLL fixes those instances.

So I am going to tag this issue as NLL-fixed-by-NLL to reflect that.

NLL (migrate mode) is enabled in all editions as of PR #59114.

I went through all the cases listed in the table in the description; 2015 and 2018 editions indeed now match in their behaviors.

The only case in the table that was not addressed by NLL migrate mode, is:

| jonas-schievink demo | 🤢| 🤢 | ✅ | #42574

(Note that you need to comment out the #![feature(nll)] to observe the undesirable behavior.)

Since #42574 remains open as a bug, we can safely close this ticket as fixed by PR #59114.

@pnkfelix

@pnkfelix #49616 has not been resolved yet

@ZhangHanDong

@ZhangHanDong : The issue in #49616 is that you have a RefMut created as a temporary, and it is dropped after the RefCell it refers to.

The reason the destructor runs at that time is because the temporaries in the tail expression of a block are specified as being dropped after the locals of that block are dropped.

I don't know if there's a reasonable way to resolve that, apart from changing the language to allow us to run such destructors earlier (which is the effect one gets in that case by putting a semi-colon after the tail expression).

But in any case, the error message (play) does describe what is happening, and does tell you how to resolve it. This is not something that NLL was ever going to address; NLL is about changes to the static semantics of the language, but the problem you are describing in #49616 is an issue with the dynamic semantics.

Merge pull request rust-lang#21114 from lmmx/no-unused-tracing-attrib…

Navigation Menu

Repository navigation

new scoping rules for safe dtors can yield spurious semi-colon or trailing unit expr #21114

More details for issue follow.

37 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

new scoping rules for safe dtors can yield spurious semi-colon or trailing unit expr #21114

Description

More details for issue follow.

Activity

pnkfelix commented on Jan 23, 2015

pnkfelix commented on Jan 23, 2015

pnkfelix commented on Feb 11, 2015

ebfull commented on Feb 13, 2015

felipesere commented on Feb 13, 2015

pnkfelix commented on Feb 14, 2015

pnkfelix commented on Feb 14, 2015

drbawb commented on Feb 15, 2015

pnkfelix commented on Feb 16, 2015

steveklabnik commented on May 24, 2016

pnkfelix commented on May 25, 2016

37 remaining items

kwanyyoss commented on Oct 16, 2018

Havvy commented on Oct 16, 2018

pnkfelix commented on Oct 16, 2018

pnkfelix commented on Oct 16, 2018

pnkfelix commented on Nov 9, 2018

pnkfelix commented on Apr 29, 2019

ZhangHanDong commented on May 1, 2019

pnkfelix commented on Sep 20, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions