Explain why borrows can't be held across yield point in async blocks #80614
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| "borrows cannot be held across a yield point, because the stack space of the current \ | ||
| function is not preserved", | ||
| ); | ||
| err.help("see https://rust-lang.github.io/async-book/03_async_await/01_chapter.html#awaiting-on-a-multithreaded-executor \ |
There was a problem hiding this comment.
You can add // ignore-tidy-linelength to the top of the file.
There was a problem hiding this comment.
When I add // ignore-tidy-linelength to the top of the file and then run ./x.py test tidy I get the error ignoring line length unnecessarily.
|
@jyn514 I've fixed the error message to be part of the "function requires..." error now |
| "borrows cannot be held across a yield point, because the stack \ | ||
| space of the current function is not preserved", |
There was a problem hiding this comment.
In retrospect I'm not sure this is actually correct, since it works as long as the executor isn't multi-threaded. I'm still a little hazy on how async borrows work, maybe we should go with @tmandry's explanation instead:
note: async blocks are not executed immediately and either must take a reference or ownership of outside variables they use
There was a problem hiding this comment.
Sorry, I actually don't understand that error message from @tmandry. How else could you use a variable, if not by reference or ownership? I'm contributing to the compiler in hopes of furthering my understanding of Rust. I find that the rules for the borrow checker are apparently simple, but because borrowing is so pervasively used throughout Rust, there are non obvious consequences, especially around things like closures (I actually haven't used async that much, but looking into the code, it seems like async and closures are somewhat similar regarding borrowing).
There was a problem hiding this comment.
@tmandry's message is trying to succinctly explain that async blocks need to be turned into a struct internally that owns or directly references every binding it touches coming from outside the block. The wording as it stands isn't very "newcomer friendly", but I find it hard to improve on its use of jargon, and for people that don't understand it the link to the async book should still be helpful.
There was a problem hiding this comment.
I see the confusion. Maybe just this, plus the help text, is enough to get the point across?
note: async blocks are not executed immediately
help: to force the async block to take ownership ofroom_ref(and any other referenced variables), use themovekeyword
or something like
note: async blocks are not executed immediately and by default refer to captured variables by reference
which seems a bit long.
jyn514
added
A-async-await
| note: function requires argument type to outlive `'static` | ||
| --> $DIR/issue-78938-async-block.rs:8:33 | ||
| | | ||
| LL | let gameloop_handle = spawn(async { | ||
| | _________________________________^ | ||
| LL | | game_loop(Arc::clone(&room_ref)) | ||
| LL | | }); | ||
| | |_____^ |
There was a problem hiding this comment.
Ideally we would change or remove this note when caused by async blocks, particularly when we suggest async move...
There was a problem hiding this comment.
I've removed this in my latest commit.
| help: to force the async block to take ownership of `room_ref` (and any other referenced variables), use the `move` keyword | ||
| | | ||
| LL | let gameloop_handle = spawn(async move { | ||
| LL | game_loop(Arc::clone(&room_ref)) | ||
| LL | }); |
There was a problem hiding this comment.
I'll need to check the suggestion that generates this becuase it seems to me we don't really need to show the whole block for this and we could make it less verbose.
There was a problem hiding this comment.
Perhaps show only the line with async?
There was a problem hiding this comment.
Fixing that requires "only" to modify the following
rust/compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs
Lines 1287 to 1301 in afa7408
The args_span should only point at the async { and the suggested code sould only be "async move {", where now it is the whole thing.
If you wish to tackle this in this PR, it'd be great, but I don't consider it a must have yet.
There was a problem hiding this comment.
No, I'll leave that for later or someone else.
| "borrows cannot be held across a yield point, because the stack \ | ||
| space of the current function is not preserved", |
There was a problem hiding this comment.
@tmandry's message is trying to succinctly explain that async blocks need to be turned into a struct internally that owns or directly references every binding it touches coming from outside the block. The wording as it stands isn't very "newcomer friendly", but I find it hard to improve on its use of jargon, and for people that don't understand it the link to the async book should still be helpful.
| if let ConstraintCategory::CallArgument = category { | ||
| if let Some(generator_kind) = use_span.generator_kind() { | ||
| if let GeneratorKind::Async(_) = generator_kind { |
There was a problem hiding this comment.
We can get rid of some unnecessary nesting by writing
| if let ConstraintCategory::CallArgument = category { | |
| if let Some(generator_kind) = use_span.generator_kind() { | |
| if let GeneratorKind::Async(_) = generator_kind { | |
| if let (ConstraintCategory::CallArgument, Some(GeneratorKind::Async(_))) = (category, use_span.generator_kind()) { |
| "borrows cannot be held across a yield point, because the stack \ | ||
| space of the current function is not preserved", | ||
| ); | ||
| err.help("see https://rust-lang.github.io/async-book/03_async_await/01_chapter.html#awaiting-on-a-multithreaded-executor \ |
There was a problem hiding this comment.
I'm worried this link can go stale. It seems like using the error index would be better, we can update this link there if it ever changes.
There was a problem hiding this comment.
Is this page the error index? If so, which error are you referring to?
There was a problem hiding this comment.
That is the error index. I believe @tmandry's idea is to modify https://github.com/rust-lang/rust/blob/9e5f7d5631b8f4009ac1c693e585d4b7108d4275/compiler/rustc_error_codes/src/error_codes/E0373.md to include information from and/or a link to chapter 1 of the async book.
There was a problem hiding this comment.
I've moved the link to error index in my latest commit.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Uhh, sorry for the huge number of commits. I'm not very familiar with git, and tried rebasing on to master as per the rustc dev guide. Is there a way for me to fix this? |
sledgehammervampire
force-pushed
the
issue-78938-fix
branch
from
bee8522
to
cf3b075
Compare
|
fixed :) |
sledgehammervampire
force-pushed
the
issue-78938-fix
branch
from
cf3b075
to
757bd23
Compare
Co-authored-by: Esteban Kuber <estebank@users.noreply.github.com>
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
|
||
| ```compile_fail,E0373 | ||
| use std::sync::Arc; | ||
| use tokio::runtime::Runtime; // 0.3.1 |
There was a problem hiding this comment.
you'll need to use only std for these, external libraries won't work.
| use tokio::runtime::Runtime; // 0.3.1 | ||
|
|
||
| async fn f() { | ||
| let room_ref = Arc::new(Vec::new()); |
There was a problem hiding this comment.
nit: style-wise, these examples usually use more "bland" variable names to put the focus on the code structure. use(), x, y, foo, task or task_handle, etc.
|
|
||
| This error may also be encountered while using `async` blocks: | ||
|
|
||
| ```compile_fail,E0373,edition2018 |
There was a problem hiding this comment.
Great, this test passes now! It can still be simplified though:
use std::future::Future;
async fn f() {
let v = vec![1, 2, 3i32];
spawn(async { //~ ERROR E0373
println!("{:?}", v)
});
}
fn spawn<F: Future + Send + 'static>(future: F) {
unimplemented!()
}Otherwise this change LGTM.
There was a problem hiding this comment.
I've changed the code snippet to yours in my latest commit.
compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs
Outdated
Show resolved
Hide resolved
…s.rs Co-authored-by: Esteban Kuber <estebank@users.noreply.github.com>
|
Thanks @1000teslas! @bors r+ rollup |
|
📌 Commit 3e9c95b has been approved by |
bors
added
S-waiting-on-bors
Explain why borrows can't be held across yield point in async blocks For rust-lang#78938.
Explain why borrows can't be held across yield point in async blocks For rust-lang#78938.
Rollup of 17 pull requests Successful merges: - rust-lang#78455 (Introduce {Ref, RefMut}::try_map for optional projections in RefCell) - rust-lang#80144 (Remove giant badge in README) - rust-lang#80614 (Explain why borrows can't be held across yield point in async blocks) - rust-lang#80670 (TrustedRandomAaccess specialization composes incorrectly for nested iter::Zips) - rust-lang#80681 (Clarify what the effects of a 'logic error' are) - rust-lang#80764 (Re-stabilize Weak::as_ptr and friends for unsized T) - rust-lang#80901 (Make `x.py --color always` apply to logging too) - rust-lang#80902 (Add a regression test for rust-lang#76281) - rust-lang#80941 (Do not suggest invalid code in pattern with loop) - rust-lang#80968 (Stabilize the poll_map feature) - rust-lang#80971 (Put all feature gate tests under `feature-gates/`) - rust-lang#81021 (Remove doctree::Import) - rust-lang#81040 (doctest: Reset errors before dropping the parse session) - rust-lang#81060 (Add a regression test for rust-lang#50041) - rust-lang#81065 (codegen_cranelift: Fix redundant semicolon warn) - rust-lang#81069 (Add sample code for Rc::new_cyclic) - rust-lang#81081 (Add test for rust-lang#34792) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
For #78938.