Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot await in scope that contains call to format! #64960

Closed
jonhoo opened this issue Oct 1, 2019 · 9 comments
Closed

Cannot await in scope that contains call to format! #64960

jonhoo opened this issue Oct 1, 2019 · 9 comments
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Oct 1, 2019

As part of #64477, it was identified that the following code no longer compiles (#64477 (comment)):

async fn foo(_: String) {
}

fn bar() -> impl Send {
    async move {
        foo(format!("{}:{}", 1, 2)).await;
    }
}

This is as a result of this issue, and @nikomatsakis goes into some more detail as to why the borrow checker rejects the code in #64477 (comment). Basically, the code ends up creating temporaries that refer to the arguments of format!, and those temporaries are dropped only at the end of the current scope, which is after .await. And so, those temporaries must live across a yield point, which in turn prevents the resulting generator from being Send with the error:

error[E0277]: `*mut (dyn std::ops::Fn() + 'static)` cannot be shared between threads safely
 --> src/lib.rs:4:13
  |
4 | fn bar() -> impl Send {
  |             ^^^^^^^^^ `*mut (dyn std::ops::Fn() + 'static)` cannot be shared between threads safely
  |

This pattern (I have found at least) is quite common, and so having the code rejected is unfortunate given that in these cases ew only need the returned String before the yield point.

@Nemo157 proposed a fix in #64477 (comment), which is implemented in #64856, but it changes the drop order of the argument to format!, which may have unintended consequences. In #64856 (comment), @nikomatsakis suggested that we may be able to solve this with changes to the analysis, which prompted the opening of this issue.

@csmoe csmoe added the A-async-await Area: Async & Await label Oct 1, 2019
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 1, 2019
@nikomatsakis nikomatsakis added AsyncAwait-OnDeck AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. and removed AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. labels Oct 1, 2019
@Blub
Copy link

Blub commented Oct 4, 2019

Since assigning the formatted string to a variable first makes this work, I wonder if it is the same issue as with std::slice::from_raw_parts{,mut}(), like the two cases of which one works the other fails in this playground: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=319431dd1b0f8a05c351ccad03b19d0b

EDIT:
Simplified playground link: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=207b3206c6a6c403445f8eca02749592

@nikomatsakis nikomatsakis added AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. AsyncAwait-Polish Async-await issues that are part of the "polish" area and removed AsyncAwait-OnDeck labels Oct 8, 2019
@nikomatsakis nikomatsakis self-assigned this Oct 8, 2019
@nikomatsakis
Copy link
Contributor

I'm assigning to myself only to dig into the output and add a few more facts about what's going on and what possible routes we might use to fix this -- not to actually fix it.

@nikomatsakis
Copy link
Contributor

I did a little investigation. To start, I created a standalone test that doesn't rely on unstable details from the standard library (gist). This allows me to experiment a bit -- for example, I removed the _oibit_remover field from Void, which makes the format temporaries be considered Send, which then allowed me to observe the MIR that is ultimately created. You can view the graphviz for that MIR here.

Some observations:

  • The array _17 is the relevant temporary. You can see that it is created in block BB9 and then has a StorageDead invoked at the block BB37 (assuming no unwinding occurs). In between, in block BB29, the yield occurs (look for the suspend terminator).
  • The type of _17 is [ArgumentV1<'a>; 2].
  • This type is not "needs drop" -- the ArgumentV1 struct consists of a borrowed reference (of type &Void) and a formatter (of type fn). The compiler knows that these things never need to be dropped, and hence there is no DROP terminator in the MIR. NLL already makes this observable in terms of the set of programs that are accepted.
  • Therefore, if we had a more precise accounting of what is live, this value could arguably be excluded. However, it would not trivially be excluded, in that the stack slot is technically still live and so one might imagine that there are raw pointers pointing to it. This is something we've discussed on and off in terms of allocating generator space (cc @tmandry on that point) but I hadn't considered it from this angle before.

My conclusion then is that the correct route to fixing this problem begins by trying to address #57107 (get more precision) and is then blocked probably on us resolving some of the rules around when we need to preserve stack slots. This latter case is related to #61849 but is distinct, since in this case _17 is not moved afaik, it is borrowed (and that borrow is dead).

This is sort of a "special case" of the idea of us being able to move destructors -- basically, we might conceivably shorten the lifetime of temporaries where the type does not 'need drop' (i.e., where there is no destructor).

@nikomatsakis nikomatsakis removed the AsyncAwait-Polish Async-await issues that are part of the "polish" area label Oct 9, 2019
@nikomatsakis nikomatsakis removed their assignment Oct 9, 2019
@asafigan
Copy link

To clarify the work around for anyone who comes across this issue, assigning the formatted string to a variable first allows the program to compile.

Work around for the code provided by @jonhoo:

change

foo(format!("{}:{}", 1, 2)).await;

to

let formatted_string = format!("{}:{}", 1, 2);
foo(formatted_string).await;

@pimeys
Copy link

pimeys commented Oct 21, 2019

Spent some hours trying to find out why my code is not compiling and in the end it was due to the same issue described here. What makes it even worse is the huge amount of type information spamming the whole terminal in my case.

Aaron1011 added a commit to Aaron1011/rust that referenced this issue Dec 7, 2019
…mentations

When we construct a generator type, we build a `ty::GeneratorWitness`
from a list of types that may live across a suspend point. These types are
used to determine the 'constitutent types' for a generator when
selecting an auto-trait predicate. Any types appearing in the
GeneratorWitness are required to implement the auto trait (e.g. `Send`
or `Sync`).

This analysis
is based on the HIR - as a result, it is unable to take liveness of
variables into account. This often results in unnecessary bounds being
computing (e.g requiring that a `Rc` be `Sync` even if it is dropped
before a suspend point), making generators and `async fn`s much less
ergonomic to write.

This commit uses the generator MIR to determine the actual 'constituent
types' of a generator. Specifically, a type in the generator witness is
considered to be a 'constituent type' of the witness if it appears in
the `field_tys` of the computed `GeneratorLayout`. Any type which is
stored across an suspend point must be a constituent type (since it could
be used again after a suspend), while any type that is not stored across
a suspend point cannot possible be a constituent type (since it is
impossible for it to be used again).

By re-using the existing generator layout computation logic, we get some
nice properties for free:
* Types which are dead before the suspend point are not considered
constituent types
* Types without Drop impls not considered constituent types if their
scope extends across an await point (assuming that they are never used
after an await point).

Note that this only affects `ty::GeneratorWitness`, *not*
`ty::Generator` itself. Upvars (captured types from the parent scope)
are considered to be constituent types of the base `ty::Generator`, not
the inner `ty::GeneratorWitness`. This means that upvars are always
considered constituent types - this is because by defintion, they always
live across the first implicit suspend point.

-------

Implementation:

The most significant part of this PR is the introduction of a new
'delayed generator witness mode' to `TraitEngine`. As @nikomatsakis
pointed out, attmepting to compute generator MIR during type-checking
results in the following cycle:

1. We attempt to type-check a generator's parent function
2. During type checking of the parent function, we record a
   predicate of the form `<generator>: AutoTrait`
3. We add this predicate to a `TraitEngine`, and attempt to fulfill it.
4. When we atempt to select the predicate, we attempt to compute the MIR
   for `<generator>`
5. The MIR query attempts to compute `type_of(generator_def_id)`, which
   results in us attempting to type-check the generator's parent function.

To break this cycle, we defer processing of all auto-trait predicates
involving `ty::GeneratorWitness`. These predicates are recorded in the
`TypeckTables` for the parent function. During MIR type-checking of the
parent function, we actually attempt to fulfill these predicates,
reporting any errors that occur.

The rest of the PR is mostly fallout from this change:

* `ty::GeneratorWitness` now stores the `DefId` of its generator. This
allows us to retrieve the MIR for the generator when `SelectionContext`
processes a predicate involving a `ty::GeneratorWitness`
* Since we now store `PredicateObligations` in `TypeckTables`, several
different types have now become `RustcEncodable`/`RustcDecodable`. These
are purely mechanical changes (adding new `#[derives]`), with one
exception - a new `SpecializedDecoder` impl for `List<Predicate>`.
This was essentially identical to other `SpecializedDecoder` imps, but it
would be good to have someone check it over.
* When we delay processing of a `Predicate`, we move it from one
`InferCtxt` to another. This requires us to prevent any inference
variables from leaking out from the first `InferCtxt` - if used in
another `InferCtxt`, they will either be non-existent or refer to the
the wrong variable. Fortunately, the predicate itself has no region
variables - the `ty::GeneratorWitness` has only late-bound regions,
while auto-traits have no generic parameters whatsoever.

However, we still need to deal with the `ObligationCause` stored by the
`PredicateObligation`. An `ObligationCause` (or a nested cause) may have
any number of region variables stored inside it (e.g. from stored
types). Luckily, `ObligationCause` is only used for error reporting, so
we can safely erase all regions variables from it, without affecting the
actual processing of the obligation.

To accomplish this, I took the somewhat unusual approach of implementing
`TypeFoldable` for `ObligationCause`, but did *not* change the `TypeFoldable`
implementation of `Obligation` to fold its contained
`ObligationCause. Other than this one odd case, all other callers of
`TypeFoldable` have no interest in folding an `ObligationCause`. As a
result, we explicitly fold the `ObligationCause` when computing our
deferred generator witness predicates. Since `ObligationCause` is only
used for displaying error messages, the worst that can happen is that a
slightly odd error message is displayed to a user.

With this change, several tests now have fewer errors than they did
previously, due to the improved generator analysis. Unfortunately, this
does not resolve issue rust-lang#64960. The MIR generator transformation stores
format temporaries in the generator, due to the fact that the `format!`
macro takes a reference to them. As a result, they are still considered
constituent types of the `GeneratorWitness`, and are still required to
implement `Send` and `Sync.

* I've changed the pretty-printing of `ty::GeneratorWitness` to print
out its generator DefId, as well as the word `witness`. This makes
debugging issues related to the computation of constituent types much
simpler.

As a final note, this PR contains one unrelated change - all generators
now implement `Freeze` unconditionally. I've opened a separate PR
containing this change - however, it's necessary to allow this branch to
compile without cycle errors. I've left it in this PR to make it easy to
test out this branch on actual code. Assuming that it is accepted, this
PR will be rebased against `master` when it is merged. Otherwise, I'll
need to figure out a different approach to generator
const-qualification.
@netvl
Copy link
Contributor

netvl commented Dec 30, 2019

This seems to be fixed in the latest nightly, curious what is the change that fixed it.

Edit: probably this one: #64856

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 30, 2019

Yup, #64856 fixed this issue, so it should probably now be closed.

@Centril
Copy link
Contributor

Centril commented Dec 30, 2019

Let's do that... :)

@Centril Centril closed this as completed Dec 30, 2019
@tmandry
Copy link
Member

tmandry commented Dec 31, 2019

My conclusion then is that the correct route to fixing this problem begins by trying to address #57107 (get more precision) and is then blocked probably on us resolving some of the rules around when we need to preserve stack slots. This latter case is related to #61849 but is distinct, since in this case _17 is not moved afaik, it is borrowed (and that borrow is dead).

I think you meant #57017. #59087 is also related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants