From b2bb51734cd97682cf2124e70ebbb73a3304b6b0 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 24 Jan 2024 17:18:08 +0000 Subject: [PATCH] Make sure that async closures (and fns) only capture their parent callable's parameters by move, and nothing else --- compiler/rustc_ast_lowering/src/item.rs | 9 ++-- compiler/rustc_hir_typeck/src/upvar.rs | 51 +++++++++++++++++++ .../async-borrowck-escaping-closure-error.rs | 2 +- ...ync-borrowck-escaping-closure-error.stderr | 21 ++++++++ .../issue-74072-lifetime-name-annotations.rs | 2 + ...sue-74072-lifetime-name-annotations.stderr | 49 ++++++++++++++++-- ...021-incompatible-closure-captures-96258.rs | 2 +- ...incompatible-closure-captures-96258.stderr | 27 +--------- 8 files changed, 127 insertions(+), 36 deletions(-) create mode 100644 tests/ui/async-await/async-borrowck-escaping-closure-error.stderr diff --git a/compiler/rustc_ast_lowering/src/item.rs b/compiler/rustc_ast_lowering/src/item.rs index 6b772c1295f..a122b4c5113 100644 --- a/compiler/rustc_ast_lowering/src/item.rs +++ b/compiler/rustc_ast_lowering/src/item.rs @@ -1278,10 +1278,11 @@ pub fn lower_coroutine_body_with_moved_arguments( }; let closure_id = coroutine_kind.closure_id(); let coroutine_expr = self.make_desugared_coroutine_expr( - // FIXME(async_closures): This should only move locals, - // and not upvars. Capturing closure upvars by ref doesn't - // work right now anyways, so whatever. - CaptureBy::Value { move_kw: rustc_span::DUMMY_SP }, + // The default capture mode here is by-ref. Later on during upvar analysis, + // we will force the captured arguments to by-move, but for async closures, + // we want to make sure that we avoid unnecessarily moving captures, or else + // all async closures would default to `FnOnce` as their calling mode. + CaptureBy::Ref, closure_id, return_type_hint, body_span, diff --git a/compiler/rustc_hir_typeck/src/upvar.rs b/compiler/rustc_hir_typeck/src/upvar.rs index 82c5e566f16..864acf51025 100644 --- a/compiler/rustc_hir_typeck/src/upvar.rs +++ b/compiler/rustc_hir_typeck/src/upvar.rs @@ -200,6 +200,57 @@ fn analyze_closure( capture_information: Default::default(), fake_reads: Default::default(), }; + + // As noted in `lower_coroutine_body_with_moved_arguments`, we default the capture mode + // to `ByRef` for the `async {}` block internal to async fns/closure. This means + // that we would *not* be moving all of the parameters into the async block by default. + // + // We force all of these arguments to be captured by move before we do expr use analysis. + // + // FIXME(async_closures): This could be cleaned up. It's a bit janky that we're just + // moving all of the `LocalSource::AsyncFn` locals here. + if let Some(hir::CoroutineKind::Desugared( + _, + hir::CoroutineSource::Fn | hir::CoroutineSource::Closure, + )) = self.tcx.coroutine_kind(closure_def_id) + { + let hir::ExprKind::Block(block, _) = body.value.kind else { + bug!(); + }; + for stmt in block.stmts { + let hir::StmtKind::Local(hir::Local { + init: Some(init), + source: hir::LocalSource::AsyncFn, + pat, + .. + }) = stmt.kind + else { + bug!(); + }; + let hir::PatKind::Binding(hir::BindingAnnotation(hir::ByRef::No, _), _, _, _) = + pat.kind + else { + // Complex pattern, skip the non-upvar local. + continue; + }; + let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = init.kind else { + bug!(); + }; + let hir::def::Res::Local(local_id) = path.res else { + bug!(); + }; + let place = self.place_for_root_variable(closure_def_id, local_id); + delegate.capture_information.push(( + place, + ty::CaptureInfo { + capture_kind_expr_id: Some(init.hir_id), + path_expr_id: Some(init.hir_id), + capture_kind: UpvarCapture::ByValue, + }, + )); + } + } + euv::ExprUseVisitor::new( &mut delegate, &self.infcx, diff --git a/tests/ui/async-await/async-borrowck-escaping-closure-error.rs b/tests/ui/async-await/async-borrowck-escaping-closure-error.rs index f8ff9186842..c02bac2d7dd 100644 --- a/tests/ui/async-await/async-borrowck-escaping-closure-error.rs +++ b/tests/ui/async-await/async-borrowck-escaping-closure-error.rs @@ -1,10 +1,10 @@ // edition:2018 -// check-pass #![feature(async_closure)] fn foo() -> Box> { let x = 0u32; Box::new((async || x)()) + //~^ ERROR closure may outlive the current function, but it borrows `x`, which is owned by the current function } fn main() { diff --git a/tests/ui/async-await/async-borrowck-escaping-closure-error.stderr b/tests/ui/async-await/async-borrowck-escaping-closure-error.stderr new file mode 100644 index 00000000000..87851e1ae5b --- /dev/null +++ b/tests/ui/async-await/async-borrowck-escaping-closure-error.stderr @@ -0,0 +1,21 @@ +error[E0373]: closure may outlive the current function, but it borrows `x`, which is owned by the current function + --> $DIR/async-borrowck-escaping-closure-error.rs:6:15 + | +LL | Box::new((async || x)()) + | ^^^^^^^^ - `x` is borrowed here + | | + | may outlive borrowed value `x` + | +note: closure is returned here + --> $DIR/async-borrowck-escaping-closure-error.rs:6:5 + | +LL | Box::new((async || x)()) + | ^^^^^^^^^^^^^^^^^^^^^^^^ +help: to force the closure to take ownership of `x` (and any other referenced variables), use the `move` keyword + | +LL | Box::new((async move || x)()) + | ++++ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0373`. diff --git a/tests/ui/async-await/issue-74072-lifetime-name-annotations.rs b/tests/ui/async-await/issue-74072-lifetime-name-annotations.rs index 95683241aba..2d453e7891e 100644 --- a/tests/ui/async-await/issue-74072-lifetime-name-annotations.rs +++ b/tests/ui/async-await/issue-74072-lifetime-name-annotations.rs @@ -12,6 +12,7 @@ pub async fn async_fn(x: &mut i32) -> &i32 { pub fn async_closure(x: &mut i32) -> impl Future { (async move || { + //~^ captured variable cannot escape `FnMut` closure body let y = &*x; *x += 1; //~ ERROR cannot assign to `*x` because it is borrowed y @@ -20,6 +21,7 @@ pub fn async_closure(x: &mut i32) -> impl Future { pub fn async_closure_explicit_return_type(x: &mut i32) -> impl Future { (async move || -> &i32 { + //~^ captured variable cannot escape `FnMut` closure body let y = &*x; *x += 1; //~ ERROR cannot assign to `*x` because it is borrowed y diff --git a/tests/ui/async-await/issue-74072-lifetime-name-annotations.stderr b/tests/ui/async-await/issue-74072-lifetime-name-annotations.stderr index 628ba1a4818..9120d78164e 100644 --- a/tests/ui/async-await/issue-74072-lifetime-name-annotations.stderr +++ b/tests/ui/async-await/issue-74072-lifetime-name-annotations.stderr @@ -11,7 +11,7 @@ LL | y | - returning this value requires that `*x` is borrowed for `'1` error[E0506]: cannot assign to `*x` because it is borrowed - --> $DIR/issue-74072-lifetime-name-annotations.rs:16:9 + --> $DIR/issue-74072-lifetime-name-annotations.rs:17:9 | LL | let y = &*x; | --- `*x` is borrowed here @@ -22,11 +22,32 @@ LL | y LL | })() | - return type of async closure is &'1 i32 +error: captured variable cannot escape `FnMut` closure body + --> $DIR/issue-74072-lifetime-name-annotations.rs:14:20 + | +LL | pub fn async_closure(x: &mut i32) -> impl Future { + | - variable defined here +LL | (async move || { + | __________________-_^ + | | | + | | inferred to be a `FnMut` closure +LL | | +LL | | let y = &*x; + | | - variable captured here +LL | | *x += 1; +LL | | y +LL | | })() + | |_____^ returns an `async` block that contains a reference to a captured variable, which then escapes the closure body + | + = note: `FnMut` closures only have access to their captured variables while they are executing... + = note: ...therefore, they cannot allow references to captured variables to escape + error[E0506]: cannot assign to `*x` because it is borrowed - --> $DIR/issue-74072-lifetime-name-annotations.rs:24:9 + --> $DIR/issue-74072-lifetime-name-annotations.rs:26:9 | LL | (async move || -> &i32 { | - let's call the lifetime of this reference `'1` +LL | LL | let y = &*x; | --- `*x` is borrowed here LL | *x += 1; @@ -34,8 +55,28 @@ LL | *x += 1; LL | y | - returning this value requires that `*x` is borrowed for `'1` +error: captured variable cannot escape `FnMut` closure body + --> $DIR/issue-74072-lifetime-name-annotations.rs:23:28 + | +LL | pub fn async_closure_explicit_return_type(x: &mut i32) -> impl Future { + | - variable defined here +LL | (async move || -> &i32 { + | __________________________-_^ + | | | + | | inferred to be a `FnMut` closure +LL | | +LL | | let y = &*x; + | | - variable captured here +LL | | *x += 1; +LL | | y +LL | | })() + | |_____^ returns an `async` block that contains a reference to a captured variable, which then escapes the closure body + | + = note: `FnMut` closures only have access to their captured variables while they are executing... + = note: ...therefore, they cannot allow references to captured variables to escape + error[E0506]: cannot assign to `*x` because it is borrowed - --> $DIR/issue-74072-lifetime-name-annotations.rs:32:9 + --> $DIR/issue-74072-lifetime-name-annotations.rs:34:9 | LL | let y = &*x; | --- `*x` is borrowed here @@ -46,6 +87,6 @@ LL | y LL | } | - return type of async block is &'1 i32 -error: aborting due to 4 previous errors +error: aborting due to 6 previous errors For more information about this error, try `rustc --explain E0506`. diff --git a/tests/ui/span/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.rs b/tests/ui/span/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.rs index a9d678c1e6a..66a432be357 100644 --- a/tests/ui/span/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.rs +++ b/tests/ui/span/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.rs @@ -9,7 +9,7 @@ pub(crate) async fn new( //~^ ERROR `async fn` is not permitted in Rust 2015 interval: Duration, //~^ ERROR cannot find type `Duration` in this scope - ) -> Numberer { //~WARN: changes to closure capture in Rust 2021 + ) -> Numberer { Numberer {} } } diff --git a/tests/ui/span/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.stderr b/tests/ui/span/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.stderr index 71e9e7602e8..60433e1c284 100644 --- a/tests/ui/span/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.stderr +++ b/tests/ui/span/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.stderr @@ -18,32 +18,7 @@ help: consider importing this struct LL + use std::time::Duration; | -warning: changes to closure capture in Rust 2021 will affect drop order - --> $DIR/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.rs:12:19 - | -LL | interval: Duration, - | -------- in Rust 2018, this causes the closure to capture `interval`, but in Rust 2021, it has no effect -LL | -LL | ) -> Numberer { - | _________________-_^ - | | | - | | in Rust 2018, `interval` is dropped here along with the closure, but in Rust 2021 `interval` is not part of the closure -LL | | Numberer {} -LL | | } - | |_____^ - | - = note: for more information, see -note: the lint level is defined here - --> $DIR/drop-location-span-error-rust-2021-incompatible-closure-captures-96258.rs:1:9 - | -LL | #![warn(rust_2021_incompatible_closure_captures)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: add a dummy let to cause `interval` to be fully captured - | -LL | ) -> Numberer { let _ = &interval; - | ++++++++++++++++++ - -error: aborting due to 2 previous errors; 1 warning emitted +error: aborting due to 2 previous errors Some errors have detailed explanations: E0412, E0670. For more information about an error, try `rustc --explain E0412`.