async functions fail to move values into generated futures in the latest nightly

[sfackler]

This simple async function fails to compile on the newest nightly: https://github.com/sfackler/rust-postgres/blob/2cc5bbf21b654c26bfd8b1e80b1e5c7cbd81235f/tokio-postgres/src/lib.rs#L163-L172

error[E0597]: `config` does not live long enough
   --> tokio-postgres/src/lib.rs:171:5
    |
171 |     config.connect(tls).await
    |     ^^^^^^-------------
    |     |
    |     borrowed value does not live long enough
    |     a temporary with access to the borrow is created here ...
172 | }
    | -
    | |
    | `config` dropped here while still borrowed
    | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `impl core::future::future::Future`
    |
    = 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.

I'm guessing this was caused by #64292 (cc @davidtwco)

[sfackler]

This can be hacked around via an async move {} block:

diff --git a/tokio-postgres/src/lib.rs b/tokio-postgres/src/lib.rs
index 98c245f..eaf43f1 100644
--- a/tokio-postgres/src/lib.rs
+++ b/tokio-postgres/src/lib.rs
@@ -168,7 +168,9 @@ where
     T: MakeTlsConnect<Socket>,
 {
     let config = config.parse::<Config>()?;
-    config.connect(tls).await
+    async move {
+        config.connect(tls).await
+    }.await
 }

[nikomatsakis]

I think the root cause of this is #46413 -- feels like we ought to have more test cases covering these patterns, but oh well.

I haven't tried to look closely enough to decide what possible solution is.

[nikomatsakis]

I can confirm that this is actually a duplicate of #64512, so this is a genuine bug. An alternative workaround is return config.connect(tls).await

[nikomatsakis]

Minimized test case is just this:

async fn add(x: u32, y: u32) -> u32 {
    async { x + y }.await
}

fn main() { }

[sfackler]

#64525 doesn't seem to have fixed this for my specific case:

error[E0597]: `config` does not live long enough
   --> tokio-postgres/src/lib.rs:171:5
    |
171 |     config.connect(tls).await
    |     ^^^^^^-------------
    |     |
    |     borrowed value does not live long enough
    |     a temporary with access to the borrow is created here ...
172 | }
    | -
    | |
    | `config` dropped here while still borrowed
    | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `impl core::future::future::Future`
    |
    = 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.

error: aborting due to previous error

rustc 1.39.0-nightly (7efe1c6e6 2019-09-17)

[lqd]

Reduced:

async fn connect() {
    let config = 666;
    connect2(&config, "".to_string()).await
}

async fn connect2(_config: &u32, _tls: String) {
    unimplemented!()
}

playground

[realcr]

I recently tried to compile my codebase on nightly-2019-09-13 and I seem to have similar issues. I have hundreds of errors like @sfackler mentioned. Just to be sure I'm having the same issue, here is an example:

        let fut_listener = async move {
            let mut access_control = AccessControlPk::new();
            inner_client_listener(
                connector,
                &mut access_control,
                &mut incoming_access_control,
                connections_sender,
                keepalive_transform,
                conn_timeout_ticks,
                timer_client,
                c_spawner,
                Some(event_sender)
            ).await
        }

This is the compilation error I get:

$ cargo test -p offst-relay
   Compiling offst-relay v0.1.0 (/home/real/projects/d/offst/components/relay)
error[E0597]: `access_control` does not live long enough
   --> components/relay/src/client/client_listener.rs:512:17
    |
510 | /             inner_client_listener(
511 | |                 connector,
512 | |                 &mut access_control,
    | |                 ^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough
513 | |                 &mut incoming_access_control,
...   |
519 | |                 Some(event_sender)
520 | |             ).await
    | |_____________- a temporary with access to the borrow is created here ...
521 |           }
    |           -
    |           |
    |           `access_control` dropped here while still borrowed
    |           ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `impl core::future::future::Future`
    |
    = 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.

error: aborting due to previous error

For more information about this error, try `rustc --explain E0597`.
error: Could not compile `offst-relay`.

Indeed saving the result into a temporary variable x and then returning it seem to satisfy the compiler.

[nikomatsakis]

OK, so I dug into this a bit. The errors are... correct-ish, but horrific. They are an outcome of the current temporary-lifetime rules, and this is in that sense a dup of #46413. That said, there is some imprecision here, and it may be possible to work-around the problem by increasing the precision of our analysis without actually changing the dynamic semantics of any existing code.

Let me explain what's going on to start.

The desugaring converts $foo.await into match $foo { bar => $await(bar) }. In the MIR desugaring, what this effectively does is to (a) store $foo into a temporary value (in the surrounding temporary scope) and then (b) move from that temporary into bar. The $await(bar) then does the stuff to "await" the future, but that's not important to us here.

So, in the case of our example, we have

async fn connect() {
    let config = 666;
    connect2(&config, "".to_string()).await
}

which is effectively desugared into something like

let config = 666;
match connect2(&config, "".to_string()) {
    the_future => $await(the_future ),
}

this then gets further desugared, as part of MIR lowering, into this:

// note the ordering here! This temporary is introduced into "the surrounding scope", 
// which means it goes *before* the let-bound variables. 
// This is basically the bug in our current rules.
let _3; 
let config = 666;
_3= connect2(&config, "".to_string());
let the_future = _3;
$await(the_future);
...
drop(_temp);

Now, here's the interesting thing: the borrow checker is smart enough to see that the drop(_temp) is going to be a no-op, because we've moved from _temp into the_future. So you might wonder, where does the error come from?

The answer requires a bit more digging. It turns out that when we desugar the _temp = connect2 call further, we get this:

let _temp; 
let config = 666;
let _4 = &config;
let _6 = "".to_string();
_3 = connect2(move _4, move _6);
drop(_6); // <-- interesting!
let the_future = _3;
$await(the_future);
...
drop(_3);

Here we have a drop(_6) that got added after the call. I'm not 100% sure how this falls out, but regardless it is a guaranteed no-op. This is because _6 is unconditionally moved in the connect2 call which dominates it. Nonetheless, we create the drop -- you can see the MIR graph in this gist. This in turn triggers us to create an unwind path -- maybe invoking the string destructor will panic!

Now this unwind path is problematic -- along that path we wind up doing the following:

bb10 {
  StorageDead(config)
  drop(_3) // this is the temporary we introduced!
}

Along this path, indeed, we are dropping _3 whose type indicates that it may hold a reference to config, but the memory for config was already invalidated. This is bad.

So there you have it:

• we introduce a drop that we know is a no-op
• but before we figure that out, there is an unwind path
  • if that drop did unwind, then _3 would be dropped before the config variable

I haven't 100% verified this is where the error arises, but I'm 99.9% sure.

So it seems like we could fix this if we avoided inserting the drop for move arguments -- I'll have to look into that, but it seems plausible.

[nikomatsakis]

So it seems like we could fix this if we avoided inserting the drop for move arguments -- I'll have to look into that, but it seems plausible.

Looked into this a bit. Here are a few tips of what I've found so far:

We have a notion of a "local operand". This refers to an operand that is going to get freed when the expression is done, and it includes call arguments:

rust/src/librustc_mir/build/expr/as_operand.rs

Lines 10 to 22 in 9b9d2af

	/// Returns an operand suitable for use until the end of the current
	/// scope expression.
	///
	/// The operand returned from this function will *not be valid* after
	/// an ExprKind::Scope is passed, so please do *not* return it from
	/// functions to avoid bad miscompiles.
	pub fn as_local_operand<M>(&mut self, block: BasicBlock, expr: M) -> BlockAnd<Operand<'tcx>>
	where
	M: Mirror<'tcx, Output = Expr<'tcx>>,
	{
	let local_scope = self.local_scope();
	self.as_operand(block, local_scope, expr)
	}

We create call arguments by invoking that method:

rust/src/librustc_mir/build/expr/into.rs

Lines 240 to 243 in 9b9d2af

	let args: Vec<_> = args
	.into_iter()
	.map(|arg| unpack!(block = this.as_local_operand(block, arg)))
	.collect();

If you trace that method out, it ultimately bottoms out in the expr_as_temp helper, which will invoke this.schedule_drop to schedule that we should add a Drop. Note that this is actually the second call; the first class to schedule_drop adds a StorageDead terminator.

rust/src/librustc_mir/build/expr/as_temp.rs

Lines 115 to 123 in 9b9d2af

	if let Some(temp_lifetime) = temp_lifetime {
	this.schedule_drop(
	expr_span,
	temp_lifetime,
	temp,
	expr_ty,
	DropKind::Value,
	);
	}

This is all correct so far and should not change. After all, we may need to execute a drop if (e.g.) one of the arguments being evaluated should unwind. We just don't need them once all the call arguments succeed.

What we can do however is -- once the call instruction has been emitted -- we could flag those operands as having been consumed (actually, we could probably do it right before emitting the call terminator, since it will move them before it could unwind). This would effectively cancel the calls to drop. I am imagining a call to a method cancel_drop which takes as argument the MIR temporary. It would search the enclosing scopes, find the drop of that temporary (if any), and mark it as "canceled". This would probably wind up being very simialr to what schedule_drop does to add a drop -- i.e., we have to invalidate caches. Presumably we could factor out the loop into some common code. The idea then would be that when we pop the scope we can avoid generating Drop terminators for values that have been canceled (we still want to StorageDead them).

[nikomatsakis]

I'm experimenting in a branch here. I actually took a slightly different tack than what I described above -- I'm not "removing" the drop but just recording that the operand was moved, and taking advantage of that info on the non-unwind path. We'll see how it goes, but it seems simpler.

[nikomatsakis]

OK, my patch works, though I'm not entirely sure how happy I am with it. =)

[nikomatsakis]

Checking rust-postgres with #64584. I have checked out the commit b7fe6bece5dd11ffc04d8178d5105f9e1a354ebd with the following diff applied:

diff --git a/tokio-postgres/src/lib.rs b/tokio-postgres/src/lib.rs
index 29f378a..98c245f 100644
--- a/tokio-postgres/src/lib.rs
+++ b/tokio-postgres/src/lib.rs
@@ -168,10 +168,7 @@ where
     T: MakeTlsConnect<Socket>,
 {
     let config = config.parse::<Config>()?;
-    // FIXME https://github.com/rust-lang/rust/issues/64391
-    async move {
-        config.connect(tls).await
-    }.await
+    config.connect(tls).await
 }

 /// An asynchronous notification.

I can confirm it does not build with nightly, but it does with #64584.

[nikomatsakis]

Now that #64584 has landed, I'm going to close this again, since afaik we've fixed all the known bugs here.