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

[pnkfelix]

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

[pnkfelix]

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.

[pnkfelix]

(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.)

[pnkfelix]

(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.)

[ebfull]

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

[felipesere]

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.

[pnkfelix]

@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

[pnkfelix]

@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

[drbawb]

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.