Skip to content

Instantly share code, notes, and snippets.

@nikomatsakis
Last active September 18, 2019 16:51
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
Star You must be signed in to star a gist
Save nikomatsakis/fee0e47e14c09c4202316d8ea51e50a0 to your computer and use it in GitHub Desktop.

TL;DR

  • Temporaries created by <expr> in <expr>.await used to be dropped before the await occurred, but we changed to have them be dropped at the end of the innermost enclosing statement, so as to increase consistency (#63832).
    • Terminology note: A temporary is some expression that must be captured in an anonymous variable. For example, foo(&vec![...]) captures the vec![...] into an anonymous variable which can be borrowed to produce the reference that foo wants.
    • Background on temporary lifetimes: In (non-async) Rust, these temporaries are typically freed at the end of the enclosing statement, although in some cases their lifetime is shorter (e.g., in an if condition) or longer (e.g., let x = &vec![...] will free the vector at the end of the innermost enclosing block).
  • This exposed a rustc bug that needs to be fixed regardless #64512. This caused 2 of the known regressions.
  • This also caused a reasonable amount of "correct" fallout, in which examples like foo(&format!(...)).await no longer work (#64477, #64433). This fallout seems to largely be caused by intermediate, non-Send temporaries -- although there are some correctable bugs where we are overapproximating what is captured, as well. The majority of the known regressions were of this kind (5 reports, but some of them contained many lines of affected code).
  • The case for keeping the current order on master (drop at end of innermost statement):
    • Overall consistency between sync/non-sync. In particular, it means that converting code from sync to async and inserting .await calls does not affect execution order (modulo yields).
    • Also, some common patterns like foo(&vec![..]).await compile.
    • The overall amount of effected code so far seems minimal -- with possible exception of Fuschia, but this is very hard to estimate.
  • The case for restoring the older order (drop before innermost await):
    • Fewer values are live over an await, resulting in smaller generators
    • Fewer values are live over an await, resulting in fewer Send-related errors
      • temporary values like mutex-guards, format! by products, etc, are oten non-send
  • Whichever way we go, there are some options for improving the cases that don't compile today, but neither path is trivial.
    • Current order on master: we would have to adopt some (opt-in, presumably) improvement that lets us reorder drops. This would let us move drops as early as possible for types where there are no visible side effects (most), which would theoretically let us restore the older "drop before await" behavior.
    • Drop before await, restored: we could potentially do targeted fixes like RFC #66 to identify temporaries whose lifetime should be prolong to avoid error. In particular, we could probably fix the foo(&vec![..]).await example this way. But there are still details to figure out, it's not obvious that this would work.

Obviously we are "within our rights" to make this change, as async-await is still on nightly only. The main question is "should we". Initially I thought it was pretty clear -- in particular, it seems like one should be able to port sequential code to async without seeing a different execution order. However, the ergonomic toll, combined with the efficiency argument, and the prospects for making a targeted improvement, make this a more complex judgement call.

History

Initial problem

As reported in #63832, the lifetime of temporaries in an await expression were shorter than is typical for Rust. In particular, owing to the present desugaring, temporaries that were part of the <expr> in a <expr>.await expression would be freed before executing the await:

let x = foo(&bar()).await;
//   1.       ^^^^^ temporary value created
//   2. ^^^ foo invoked with reference to temporary
//   3. temporary gets freed
//   4. future is awaited 

This is in contrast to the typical Rust pattern, where the temporaries would get freed at the end of the statement (or, as I often say, "at the semicolon"):

let x = foo(&bar()) + baz();
//   1.      ^^^^^ temporary value created
//   2. ^^^ foo invoked
//   3. baz is invoked
//   4. temporary is freed

This was a side-effect of the desugaring. Effectively, foo(&bar()).await become something like:

let x = {
  let __future = foo(&bar()); // innermost enclosing statement to `bar()` call
  loop { /* while future is not ready, suspend, yield result */ }
};

Fix

In #64292, we adjusted the desugaring so that the lifetime of bar() would be the "innermost enclosing statement":

let x = match foo(&bar()) { // innermost enclosing statement is same as before
  future => loop { /* while future is not ready, suspend, yield result */ }
};

When we were deciding on this fix, we had considered that this would mean that more temporaries were "live" across an await. We anticipated this to have a negative performance impact (owing to bigger generators). That is, we thought that it might lead to people writing code like so in an effort to minimize generator size:

let x = {
    let tmp = foo(&bar();
    tmp.await
};

However, I don't think we fully considered that the longer lifetimes could also lead to compilation errors -- and they seem to, quite frequently. (Or at least I personally didn't. --Niko)

Consequences

In any case, since landing #64292, there were a number of compilation errors. I count six instances of people reported broken code, although it is a bit challenging to get a sense for the "magnitude" of the effect. It seems non-trivial, though. For example, tmandry reports in a comment that "This causes errors to crop up all over the Fuchsia code base".

Two relevant rust issues are #64391 and #64477. Both are tied to this issue. The first is a case where data "lives too long" (more on that later), because the temporary lifetime has increased -- but this turns out to actually be the result of a rustc bug #64512, which should be fixed regardless. The second is a case where the data that is now live across an await is not Send -- these cases are not related to #64512 and are caused by the new, longer lifetime of temporaries.

One challenge here is that the temporary lifetimes have long been known to have some "unfortunate" interactions around the tail expressions of blocks. Specifically, the "innermost enclosing statement" for a block's tail expression is the statement that encloses the block itself. This means that let x = <expr> and let x = { <expr> } are equivalent in terms of when temporaries created in <expr> are freed, but it often leads to errors, particularly in the tail expression of a function. See e.g. #46413, which was opened during the move to NLL -- that move uncovered a number of bugs in our existing checker.

Prospects for future change

This is a bit of a tricky issue, since it concerns the dynamic lifetime of temporary values. This means that changes could affect the semantics of code -- specifically, when Drop runs. However, there is one exception: if the code currently fails to compile, then changes that allow it to compile should not be a problem.

Terminology:

  • innermost await drop order refers to the initial state before #63832 was fixed, as temporaries were scoped to just before the innermost await or innermost statement.
  • innermost statement drop order refers to the current master state, after PR [#62492] landed, as temporaries are always dropped at the innermost statement.

Innermost statement drop order is consistent, if less efficient

Innermost statement drop order is more consistent with the language overall, as that was the prior rule. Crucially, it means that converting code from sync to async and inserting .await calls does not affect execution order (modulo yields).

However, innermost statement drop order winds up preserving temporaries across await that do not have to be preserved. For example, foo(&format!(...)).await will preserve the temporary values created by format!.

Type-based fixes don't work. This is tricky to fix: we can't have a rule like "if a temporary's type is not Send, then drop it earlier, at the innermost await". We could not hope to create such a rule in generic contexts, as we would not know whether a type is send or not until monomorphizing time. Even if we could make it work, it's a very complex and ad hoc rule.

Some "pure drop" approach could work. In most cases, the drop code is just freeing memory, and it could safely be reordered. One idea that has been proposed, and which could help here, is to somehow enable types to declare that their drop is pure and thus can be reordered. The compiler would then be able to move the drop for values up earlier, perhaps before the await occurs. This is a fine optimization, and something we should definitely consider, but specifying it will take some time: it interacts with the UCG, for example, since we'll need rules for when the compiler can safely assume all aliases to the value are dead. One benefit of this is that it would affect all code uniformly.

Innermost await drop order is surprising, but more efficient and could be improved

In contrast, if we adopted innermost await drop order, it would lead to more efficient generators overall (fewer things held over yields) and eliminate errors from temporaries that did not have to be live over an await (which seems to be common in practice).

However, innermost await drop order does cause errors in some surprising cases, such as foo(&vec![..]).await. This is similar to some "temporary lifetime too short" errors from sync code, such as the motivating example from RFC #66:

let x = os::args().slice_from(1);

The fix proposed in that RFC, though never implemented, relies on a feature of our temporary lifetime rules. In particular, the dynamic lifetime of a temporary is extended from the innermost statement to the innermost block if we can see syntactically that is assigned to a let-bound variable. This is why let x = &vec![]; compiles, for example (the temporary storing vec![] is extended to the lifetime of the block, instead of being freed after the let finishes execution). RFC #66 proposed to examine the signature of methods like slice_from to deduce that (in this case) the temporary storing os::args() would have to be extended to the enclosing block.

Note: the wording of the RFC is pretty vague, and one of my (Niko's) long deferred projects is to write up an executable version of this. But I imagine the rule would look something like this: if you have a function parameter of type &'a T, and 'a appears in the return type, than we use an "extended temporary" lifetime for the value supplied as that argument. This has the advantage of being derived purely by looking at the (user-supplied) signature of the fn being called and could be integrated quite readily into our compiler.

It is plausible that a similar approach could be used to accept the motivating example of foo(&vec![...]).await. In this case, the function foo has a signature like fn foo<'a>(x: &'a [T]) -> impl Future + 'a. Note that the 'a meets our criteria above, and hence the (dynamic) lifetime of the vec![..] temporary could be extended.

There would be some stuff to figure out still, though. It just used the existing desugaring:

{
    let future = ...;
    loop { .. }
}

then the naive effect of RFC #66 would be extend the lifetime of the vec![..] temporary not to the "innermost statement" but only until the end of the block -- that is, just after the await call. To do differently, we'd have to special case the handling of temporary lifetimes in await expressions in a way that doesn't have an obvious desugaring to me.

Bottom line: It seems plausible that we could extend innermost await drop order in the future to make more examples work, but it will always be inconsistent with purely sync code.

What might a fix

Affected pattern(s)

This section goes through the affected pattern(s) and tries to figure out what is going on in each case. It turns out that some of them are bugs in rustc, but not all.

Longer lifetime for temporaries results in things "living" across await that did not used to

This is a pretty straight-forward side-effect. It often results in errors indicating that some value is not Send, as reported in #64477.

Intersection with "tail expression" woes of temporaries

Lifetime errors from #64433 -- FIXME write more

Await in return position (rustc bug)

One special case worth looking at is the return position of a function. It turns out that the errors found here are a result of a bug, #64512. Consider this example, from miri, as exempted by seanmonstar, seems representative:

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

We now require a workaround:

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

However, a 'trivial rewrite' does not

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

What's going on here? It has to do with the way we do our desugaring for async fn. Presently it's something like this:

fn add(x: u32, y: u32) -> u32 {
  async move {
    let x = x; // capture and destructure paramters...
    let y = y; // ...required to get drop order correct
    async { x + y }.await // body of the async fn
  }
}

The problem here is that, in a block, temporaries created in the tail expression are dropped after the let-bound variables in the block! This was done intentionally, so as to preserve an equivalent between let x = <expr> and let x = { <expr> }. However, it is far from clear that this was the right decision. Regardless, with this desugaring, we get a drop order like:

  • drop y
  • drop x
  • drop temporaries from tail expression

This does not the lifetime of parameters in a synchronous function , which drop after the temporaries from the return tail expression (see e.g. playground).

To correct this, we should change our desugaring to:

// async fn add($parameter_patterns) { $body }
fn add(raw_parameters) {
  async move {
    let $parameter_patterns = raw-parameters;
    return $body; // body of the async fn
  }
}

Using the explicit return will make the "innermost statement" for temporaries be the return statement. An alternative would be

async move {
  let $parameter_patterns = ...;
  let return_value = $body;
  return_value
}

but that seems strictly worse in terms of the code we might generate (it complicates NRVO).

Known examples:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment