From 599d456a7532aa5de020b42fd1d9d936fa647573 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 8 Apr 2024 21:09:13 -0400 Subject: [PATCH] Make the computation of coroutine_captures_by_ref_ty more sophisticated --- compiler/rustc_hir_typeck/src/upvar.rs | 114 ++++++++++--- .../async-borrowck-escaping-closure-error.rs | 1 - ...ync-borrowck-escaping-closure-error.stderr | 11 +- .../async-closures/moro-example.rs | 43 +++++ .../async-closures/no-borrow-from-env.rs | 44 +++++ ...thout-precise-captures-we-are-powerless.rs | 47 ++++++ ...t-precise-captures-we-are-powerless.stderr | 152 ++++++++++++++++++ 7 files changed, 378 insertions(+), 34 deletions(-) create mode 100644 tests/ui/async-await/async-closures/moro-example.rs create mode 100644 tests/ui/async-await/async-closures/no-borrow-from-env.rs create mode 100644 tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.rs create mode 100644 tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr diff --git a/compiler/rustc_hir_typeck/src/upvar.rs b/compiler/rustc_hir_typeck/src/upvar.rs index c987bfb9a0e88..ef569b4bef3b3 100644 --- a/compiler/rustc_hir_typeck/src/upvar.rs +++ b/compiler/rustc_hir_typeck/src/upvar.rs @@ -367,37 +367,48 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ty::INNERMOST, ty::BoundRegion { var: ty::BoundVar::ZERO, kind: ty::BoundRegionKind::BrEnv }, ); + + let num_args = args + .as_coroutine_closure() + .coroutine_closure_sig() + .skip_binder() + .tupled_inputs_ty + .tuple_fields() + .len(); + let typeck_results = self.typeck_results.borrow(); + let tupled_upvars_ty_for_borrow = Ty::new_tup_from_iter( self.tcx, - self.typeck_results - .borrow() - .closure_min_captures_flattened( - self.tcx.coroutine_for_closure(closure_def_id).expect_local(), - ) - // Skip the captures that are just moving the closure's args - // into the coroutine. These are always by move, and we append - // those later in the `CoroutineClosureSignature` helper functions. - .skip( - args.as_coroutine_closure() - .coroutine_closure_sig() - .skip_binder() - .tupled_inputs_ty - .tuple_fields() - .len(), - ) - .map(|captured_place| { - let upvar_ty = captured_place.place.ty(); - let capture = captured_place.info.capture_kind; + ty::analyze_coroutine_closure_captures( + typeck_results.closure_min_captures_flattened(closure_def_id), + typeck_results + .closure_min_captures_flattened( + self.tcx.coroutine_for_closure(closure_def_id).expect_local(), + ) + // Skip the captures that are just moving the closure's args + // into the coroutine. These are always by move, and we append + // those later in the `CoroutineClosureSignature` helper functions. + .skip(num_args), + |(_, parent_capture), (_, child_capture)| { + // This is subtle. See documentation on function. + let needs_ref = should_reborrow_from_env_of_parent_coroutine_closure( + parent_capture, + child_capture, + ); + + let upvar_ty = child_capture.place.ty(); + let capture = child_capture.info.capture_kind; // Not all upvars are captured by ref, so use // `apply_capture_kind_on_capture_ty` to ensure that we // compute the right captured type. - apply_capture_kind_on_capture_ty( + return apply_capture_kind_on_capture_ty( self.tcx, upvar_ty, capture, - Some(closure_env_region), - ) - }), + if needs_ref { Some(closure_env_region) } else { child_capture.region }, + ); + }, + ), ); let coroutine_captures_by_ref_ty = Ty::new_fn_ptr( self.tcx, @@ -1761,6 +1772,63 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } +/// Determines whether a child capture that is derived from a parent capture +/// should be borrowed with the lifetime of the parent coroutine-closure's env. +/// +/// There are two cases when this needs to happen: +/// +/// (1.) Are we borrowing data owned by the parent closure? We can determine if +/// that is the case by checking if the parent capture is by move, EXCEPT if we +/// apply a deref projection, which means we're reborrowing a reference that we +/// captured by move. +/// +/// ```rust +/// #![feature(async_closure)] +/// let x = &1i32; // Let's call this lifetime `'1`. +/// let c = async move || { +/// println!("{:?}", *x); +/// // Even though the inner coroutine borrows by ref, we're only capturing `*x`, +/// // not `x`, so the inner closure is allowed to reborrow the data for `'1`. +/// }; +/// ``` +/// +/// (2.) If a coroutine is mutably borrowing from a parent capture, then that +/// mutable borrow cannot live for longer than either the parent *or* the borrow +/// that we have on the original upvar. Therefore we always need to borrow the +/// child capture with the lifetime of the parent coroutine-closure's env. +/// +/// ```rust +/// #![feature(async_closure)] +/// let mut x = 1i32; +/// let c = async || { +/// x = 1; +/// // The parent borrows `x` for some `&'1 mut i32`. +/// // However, when we call `c()`, we implicitly autoref for the signature of +/// // `AsyncFnMut::async_call_mut`. Let's call that lifetime `'call`. Since +/// // the maximum that `&'call mut &'1 mut i32` can be reborrowed is `&'call mut i32`, +/// // the inner coroutine should capture w/ the lifetime of the coroutine-closure. +/// }; +/// ``` +/// +/// If either of these cases apply, then we should capture the borrow with the +/// lifetime of the parent coroutine-closure's env. Luckily, if this function is +/// not correct, then the program is not unsound, since we still borrowck and validate +/// the choices made from this function -- the only side-effect is that the user +/// may receive unnecessary borrowck errors. +fn should_reborrow_from_env_of_parent_coroutine_closure<'tcx>( + parent_capture: &ty::CapturedPlace<'tcx>, + child_capture: &ty::CapturedPlace<'tcx>, +) -> bool { + // (1.) + (!parent_capture.is_by_ref() + && !matches!( + child_capture.place.projections.get(parent_capture.place.projections.len()), + Some(Projection { kind: ProjectionKind::Deref, .. }) + )) + // (2.) + || matches!(child_capture.info.capture_kind, UpvarCapture::ByRef(ty::BorrowKind::MutBorrow)) +} + /// Truncate the capture so that the place being borrowed is in accordance with RFC 1240, /// which states that it's unsafe to take a reference into a struct marked `repr(packed)`. fn restrict_repr_packed_field_ref_capture<'tcx>( 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 1990d0ffe2a3f..ffb97ca04ac50 100644 --- a/tests/ui/async-await/async-borrowck-escaping-closure-error.rs +++ b/tests/ui/async-await/async-borrowck-escaping-closure-error.rs @@ -5,7 +5,6 @@ fn foo() -> Box> { let x = 0u32; Box::new((async || x)()) //~^ ERROR cannot return value referencing local variable `x` - //~| ERROR cannot return value referencing temporary value } 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 index be67c78221a7c..4b1ce300b5692 100644 --- a/tests/ui/async-await/async-borrowck-escaping-closure-error.stderr +++ b/tests/ui/async-await/async-borrowck-escaping-closure-error.stderr @@ -7,15 +7,6 @@ LL | Box::new((async || x)()) | | `x` is borrowed here | returns a value referencing data owned by the current function -error[E0515]: cannot return value referencing temporary value - --> $DIR/async-borrowck-escaping-closure-error.rs:6:5 - | -LL | Box::new((async || x)()) - | ^^^^^^^^^------------^^^ - | | | - | | temporary value created here - | returns a value referencing data owned by the current function - -error: aborting due to 2 previous errors +error: aborting due to 1 previous error For more information about this error, try `rustc --explain E0515`. diff --git a/tests/ui/async-await/async-closures/moro-example.rs b/tests/ui/async-await/async-closures/moro-example.rs new file mode 100644 index 0000000000000..5a8f42c7ca5a4 --- /dev/null +++ b/tests/ui/async-await/async-closures/moro-example.rs @@ -0,0 +1,43 @@ +//@ check-pass +//@ edition: 2021 + +#![feature(async_closure)] + +use std::future::Future; +use std::pin::Pin; +use std::{marker::PhantomData, sync::Mutex}; + +type BoxFuture<'a, T> = Pin + Send + 'a>>; + +pub struct Scope<'scope, 'env: 'scope> { + enqueued: Mutex>>, + phantom: PhantomData<&'env ()>, +} + +impl<'scope, 'env: 'scope> Scope<'scope, 'env> { + pub fn spawn(&'scope self, future: impl Future + Send + 'scope) { + self.enqueued.lock().unwrap().push(Box::pin(future)); + } +} + +fn scope_with_closure<'env, B>(_body: B) -> BoxFuture<'env, ()> +where + for<'scope> B: async FnOnce(&'scope Scope<'scope, 'env>), +{ + todo!() +} + +type ScopeRef<'scope, 'env> = &'scope Scope<'scope, 'env>; + +async fn go<'a>(value: &'a i32) { + let closure = async |scope: ScopeRef<'_, 'a>| { + let _future1 = scope.spawn(async { + // Make sure that `*value` is immutably borrowed with lifetime of + // `'a` and not with the lifetime of the containing coroutine-closure. + let _v = *value; + }); + }; + scope_with_closure(closure).await; +} + +fn main() {} diff --git a/tests/ui/async-await/async-closures/no-borrow-from-env.rs b/tests/ui/async-await/async-closures/no-borrow-from-env.rs new file mode 100644 index 0000000000000..fe84aeeb32f7f --- /dev/null +++ b/tests/ui/async-await/async-closures/no-borrow-from-env.rs @@ -0,0 +1,44 @@ +//@ edition: 2021 +//@ check-pass + +#![feature(async_closure)] + +fn outlives<'a>(_: impl Sized + 'a) {} + +async fn call_once(f: impl async FnOnce()) { + f().await; +} + +fn simple<'a>(x: &'a i32) { + let c = async || { println!("{}", *x); }; + outlives::<'a>(c()); + outlives::<'a>(call_once(c)); + + let c = async move || { println!("{}", *x); }; + outlives::<'a>(c()); + outlives::<'a>(call_once(c)); +} + +struct S<'a>(&'a i32); + +fn through_field<'a>(x: S<'a>) { + let c = async || { println!("{}", *x.0); }; + outlives::<'a>(c()); + outlives::<'a>(call_once(c)); + + let c = async move || { println!("{}", *x.0); }; + outlives::<'a>(c()); + outlives::<'a>(call_once(c)); +} + +fn through_field_and_ref<'a>(x: &S<'a>) { + let c = async || { println!("{}", *x.0); }; + outlives::<'a>(c()); + outlives::<'a>(call_once(c)); + + let c = async move || { println!("{}", *x.0); }; + outlives::<'a>(c()); + // outlives::<'a>(call_once(c)); // FIXME(async_closures): Figure out why this fails +} + +fn main() {} diff --git a/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.rs b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.rs new file mode 100644 index 0000000000000..17681161e20ed --- /dev/null +++ b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.rs @@ -0,0 +1,47 @@ +//@ edition: 2018 + +// This is `no-borrow-from-env.rs`, but under edition 2018 we still want to make +// sure that we don't ICE or anything, even if precise closure captures means +// that we can't actually borrowck successfully. + +#![feature(async_closure)] + +fn outlives<'a>(_: impl Sized + 'a) {} + +async fn call_once(f: impl async FnOnce()) { + f().await; +} + +fn simple<'a>(x: &'a i32) { + let c = async || { println!("{}", *x); }; //~ ERROR `x` does not live long enough + outlives::<'a>(c()); + outlives::<'a>(call_once(c)); + + let c = async move || { println!("{}", *x); }; + outlives::<'a>(c()); //~ ERROR `c` does not live long enough + outlives::<'a>(call_once(c)); //~ ERROR cannot move out of `c` +} + +struct S<'a>(&'a i32); + +fn through_field<'a>(x: S<'a>) { + let c = async || { println!("{}", *x.0); }; //~ ERROR `x` does not live long enough + outlives::<'a>(c()); + outlives::<'a>(call_once(c)); + + let c = async move || { println!("{}", *x.0); }; //~ ERROR cannot move out of `x` + outlives::<'a>(c()); //~ ERROR `c` does not live long enough + outlives::<'a>(call_once(c)); //~ ERROR cannot move out of `c` +} + +fn through_field_and_ref<'a>(x: &S<'a>) { + let c = async || { println!("{}", *x.0); }; //~ ERROR `x` does not live long enough + outlives::<'a>(c()); + outlives::<'a>(call_once(c)); //~ ERROR explicit lifetime required in the type of `x` + + let c = async move || { println!("{}", *x.0); }; + outlives::<'a>(c()); //~ ERROR `c` does not live long enough + // outlives::<'a>(call_once(c)); // FIXME(async_closures): Figure out why this fails +} + +fn main() {} diff --git a/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr new file mode 100644 index 0000000000000..569028934cbac --- /dev/null +++ b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr @@ -0,0 +1,152 @@ +error[E0597]: `x` does not live long enough + --> $DIR/without-precise-captures-we-are-powerless.rs:16:13 + | +LL | fn simple<'a>(x: &'a i32) { + | -- lifetime `'a` defined here +LL | let c = async || { println!("{}", *x); }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough +LL | outlives::<'a>(c()); +LL | outlives::<'a>(call_once(c)); + | ------------ argument requires that `x` is borrowed for `'a` +... +LL | } + | - `x` dropped here while still borrowed + +error[E0597]: `c` does not live long enough + --> $DIR/without-precise-captures-we-are-powerless.rs:21:20 + | +LL | fn simple<'a>(x: &'a i32) { + | -- lifetime `'a` defined here +... +LL | let c = async move || { println!("{}", *x); }; + | - binding `c` declared here +LL | outlives::<'a>(c()); + | ^-- + | | + | borrowed value does not live long enough + | argument requires that `c` is borrowed for `'a` +LL | outlives::<'a>(call_once(c)); +LL | } + | - `c` dropped here while still borrowed + +error[E0505]: cannot move out of `c` because it is borrowed + --> $DIR/without-precise-captures-we-are-powerless.rs:22:30 + | +LL | fn simple<'a>(x: &'a i32) { + | -- lifetime `'a` defined here +... +LL | let c = async move || { println!("{}", *x); }; + | - binding `c` declared here +LL | outlives::<'a>(c()); + | --- + | | + | borrow of `c` occurs here + | argument requires that `c` is borrowed for `'a` +LL | outlives::<'a>(call_once(c)); + | ^ move out of `c` occurs here + +error[E0597]: `x` does not live long enough + --> $DIR/without-precise-captures-we-are-powerless.rs:28:13 + | +LL | fn through_field<'a>(x: S<'a>) { + | -- lifetime `'a` defined here +LL | let c = async || { println!("{}", *x.0); }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough +LL | outlives::<'a>(c()); +LL | outlives::<'a>(call_once(c)); + | ------------ argument requires that `x` is borrowed for `'a` +... +LL | } + | - `x` dropped here while still borrowed + +error[E0505]: cannot move out of `x` because it is borrowed + --> $DIR/without-precise-captures-we-are-powerless.rs:32:13 + | +LL | fn through_field<'a>(x: S<'a>) { + | -- lifetime `'a` defined here +LL | let c = async || { println!("{}", *x.0); }; + | ---------------------------------- borrow of `x` occurs here +LL | outlives::<'a>(c()); +LL | outlives::<'a>(call_once(c)); + | ------------ argument requires that `x` is borrowed for `'a` +LL | +LL | let c = async move || { println!("{}", *x.0); }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move out of `x` occurs here + +error[E0597]: `c` does not live long enough + --> $DIR/without-precise-captures-we-are-powerless.rs:33:20 + | +LL | fn through_field<'a>(x: S<'a>) { + | -- lifetime `'a` defined here +... +LL | let c = async move || { println!("{}", *x.0); }; + | - binding `c` declared here +LL | outlives::<'a>(c()); + | ^-- + | | + | borrowed value does not live long enough + | argument requires that `c` is borrowed for `'a` +LL | outlives::<'a>(call_once(c)); +LL | } + | - `c` dropped here while still borrowed + +error[E0505]: cannot move out of `c` because it is borrowed + --> $DIR/without-precise-captures-we-are-powerless.rs:34:30 + | +LL | fn through_field<'a>(x: S<'a>) { + | -- lifetime `'a` defined here +... +LL | let c = async move || { println!("{}", *x.0); }; + | - binding `c` declared here +LL | outlives::<'a>(c()); + | --- + | | + | borrow of `c` occurs here + | argument requires that `c` is borrowed for `'a` +LL | outlives::<'a>(call_once(c)); + | ^ move out of `c` occurs here + +error[E0597]: `x` does not live long enough + --> $DIR/without-precise-captures-we-are-powerless.rs:38:13 + | +LL | fn through_field_and_ref<'a>(x: &S<'a>) { + | -- lifetime `'a` defined here +LL | let c = async || { println!("{}", *x.0); }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough +LL | outlives::<'a>(c()); +LL | outlives::<'a>(call_once(c)); + | ------------ argument requires that `x` is borrowed for `'a` +... +LL | } + | - `x` dropped here while still borrowed + +error[E0621]: explicit lifetime required in the type of `x` + --> $DIR/without-precise-captures-we-are-powerless.rs:40:20 + | +LL | fn through_field_and_ref<'a>(x: &S<'a>) { + | ------ help: add explicit lifetime `'a` to the type of `x`: `&'a S<'a>` +... +LL | outlives::<'a>(call_once(c)); + | ^^^^^^^^^^^^ lifetime `'a` required + +error[E0597]: `c` does not live long enough + --> $DIR/without-precise-captures-we-are-powerless.rs:43:20 + | +LL | fn through_field_and_ref<'a>(x: &S<'a>) { + | -- lifetime `'a` defined here +... +LL | let c = async move || { println!("{}", *x.0); }; + | - binding `c` declared here +LL | outlives::<'a>(c()); + | ^-- + | | + | borrowed value does not live long enough + | argument requires that `c` is borrowed for `'a` +LL | // outlives::<'a>(call_once(c)); // FIXME(async_closures): Figure out why this fails +LL | } + | - `c` dropped here while still borrowed + +error: aborting due to 10 previous errors + +Some errors have detailed explanations: E0505, E0597, E0621. +For more information about an error, try `rustc --explain E0505`.