From 3d9d5d7c96ae3df2cfc47e933ab11ad5fa30f3bc Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 1 Apr 2024 14:47:12 -0400 Subject: [PATCH 1/2] Actually use the inferred ClosureKind from signature inference in coroutine-closures --- compiler/rustc_hir_typeck/src/closure.rs | 32 ++++++++---- compiler/rustc_hir_typeck/src/upvar.rs | 26 ++++++---- .../miri/tests/pass/async-closure-captures.rs | 34 +++++++++++++ .../tests/pass/async-closure-captures.stderr | 31 ++++++++++++ .../tests/pass/async-closure-captures.stdout | 10 ---- .../ui/async-await/async-closures/captures.rs | 40 ++++++++++++++- .../async-closures/captures.stderr | 31 ++++++++++++ .../async-closures/wrong-fn-kind.rs | 10 ++-- .../async-closures/wrong-fn-kind.stderr | 49 +++++++++---------- 9 files changed, 203 insertions(+), 60 deletions(-) create mode 100644 src/tools/miri/tests/pass/async-closure-captures.stderr delete mode 100644 src/tools/miri/tests/pass/async-closure-captures.stdout create mode 100644 tests/ui/async-await/async-closures/captures.stderr diff --git a/compiler/rustc_hir_typeck/src/closure.rs b/compiler/rustc_hir_typeck/src/closure.rs index 36af539401565..dbae8bfb54249 100644 --- a/compiler/rustc_hir_typeck/src/closure.rs +++ b/compiler/rustc_hir_typeck/src/closure.rs @@ -227,11 +227,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { kind: TypeVariableOriginKind::ClosureSynthetic, span: expr_span, }); - let closure_kind_ty = self.next_ty_var(TypeVariableOrigin { - // FIXME(eddyb) distinguish closure kind inference variables from the rest. - kind: TypeVariableOriginKind::ClosureSynthetic, - span: expr_span, - }); + + let closure_kind_ty = match expected_kind { + Some(kind) => Ty::from_closure_kind(tcx, kind), + + // Create a type variable (for now) to represent the closure kind. + // It will be unified during the upvar inference phase (`upvar.rs`) + None => self.next_ty_var(TypeVariableOrigin { + kind: TypeVariableOriginKind::ClosureSynthetic, + span: expr_span, + }), + }; + let coroutine_captures_by_ref_ty = self.next_ty_var(TypeVariableOrigin { kind: TypeVariableOriginKind::ClosureSynthetic, span: expr_span, @@ -262,10 +269,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }, ); - let coroutine_kind_ty = self.next_ty_var(TypeVariableOrigin { - kind: TypeVariableOriginKind::ClosureSynthetic, - span: expr_span, - }); + let coroutine_kind_ty = match expected_kind { + Some(kind) => Ty::from_coroutine_closure_kind(tcx, kind), + + // Create a type variable (for now) to represent the closure kind. + // It will be unified during the upvar inference phase (`upvar.rs`) + None => self.next_ty_var(TypeVariableOrigin { + kind: TypeVariableOriginKind::ClosureSynthetic, + span: expr_span, + }), + }; + let coroutine_upvars_ty = self.next_ty_var(TypeVariableOrigin { kind: TypeVariableOriginKind::ClosureSynthetic, span: expr_span, diff --git a/compiler/rustc_hir_typeck/src/upvar.rs b/compiler/rustc_hir_typeck/src/upvar.rs index 72e5b1ed95bf0..947fff679191a 100644 --- a/compiler/rustc_hir_typeck/src/upvar.rs +++ b/compiler/rustc_hir_typeck/src/upvar.rs @@ -399,16 +399,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ); // Additionally, we can now constrain the coroutine's kind type. - let ty::Coroutine(_, coroutine_args) = - *self.typeck_results.borrow().expr_ty(body.value).kind() - else { - bug!(); - }; - self.demand_eqtype( - span, - coroutine_args.as_coroutine().kind_ty(), - Ty::from_coroutine_closure_kind(self.tcx, closure_kind), - ); + // + // We only do this if `infer_kind`, because if we have constrained + // the kind from closure signature inference, the kind inferred + // for the inner coroutine may actually be more restrictive. + if infer_kind { + let ty::Coroutine(_, coroutine_args) = + *self.typeck_results.borrow().expr_ty(body.value).kind() + else { + bug!(); + }; + self.demand_eqtype( + span, + coroutine_args.as_coroutine().kind_ty(), + Ty::from_coroutine_closure_kind(self.tcx, closure_kind), + ); + } } self.log_closure_min_capture_info(closure_def_id, span); diff --git a/src/tools/miri/tests/pass/async-closure-captures.rs b/src/tools/miri/tests/pass/async-closure-captures.rs index 3e33de32efb04..cac26bfe14621 100644 --- a/src/tools/miri/tests/pass/async-closure-captures.rs +++ b/src/tools/miri/tests/pass/async-closure-captures.rs @@ -88,4 +88,38 @@ async fn async_main() { }; call_once(c).await; } + + fn force_fnonce(f: impl async FnOnce() -> T) -> impl async FnOnce() -> T { + f + } + + // Capture something with `move`, but infer to `AsyncFnOnce` + { + let x = Hello(6); + let c = force_fnonce(async move || { + println!("{x:?}"); + }); + call_once(c).await; + + let x = &Hello(7); + let c = force_fnonce(async move || { + println!("{x:?}"); + }); + call_once(c).await; + } + + // Capture something by-ref, but infer to `AsyncFnOnce` + { + let x = Hello(8); + let c = force_fnonce(async || { + println!("{x:?}"); + }); + call_once(c).await; + + let x = &Hello(9); + let c = force_fnonce(async || { + println!("{x:?}"); + }); + call_once(c).await; + } } diff --git a/src/tools/miri/tests/pass/async-closure-captures.stderr b/src/tools/miri/tests/pass/async-closure-captures.stderr new file mode 100644 index 0000000000000..f1548aadefa86 --- /dev/null +++ b/src/tools/miri/tests/pass/async-closure-captures.stderr @@ -0,0 +1,31 @@ +error[E0597]: `x` does not live long enough + --> $DIR/async-closure-captures.rs:LL:CC + | +LL | let c = force_fnonce(async move || { + | ____________________________________________- +LL | | println!("{x:?}"); + | | ^ borrowed value does not live long enough +LL | | }); + | | -- + | | || + | | |`x` dropped here while still borrowed + | |_________|borrow later used here + | value captured here by coroutine + +error[E0597]: `x` does not live long enough + --> $DIR/async-closure-captures.rs:LL:CC + | +LL | let c = force_fnonce(async move || { + | ____________________________________________- +LL | | println!("{x:?}"); + | | ^ borrowed value does not live long enough +LL | | }); + | | -- + | | || + | | |`x` dropped here while still borrowed + | |_________|borrow later used here + | value captured here by coroutine + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0597`. diff --git a/src/tools/miri/tests/pass/async-closure-captures.stdout b/src/tools/miri/tests/pass/async-closure-captures.stdout deleted file mode 100644 index a0db6d236fef1..0000000000000 --- a/src/tools/miri/tests/pass/async-closure-captures.stdout +++ /dev/null @@ -1,10 +0,0 @@ -Hello(0) -Hello(0) -Hello(1) -Hello(1) -Hello(2) -Hello(3) -Hello(3) -Hello(4) -Hello(4) -Hello(5) diff --git a/tests/ui/async-await/async-closures/captures.rs b/tests/ui/async-await/async-closures/captures.rs index e3ab8713709d4..6011292b6451e 100644 --- a/tests/ui/async-await/async-closures/captures.rs +++ b/tests/ui/async-await/async-closures/captures.rs @@ -1,7 +1,7 @@ //@ aux-build:block-on.rs //@ edition:2021 -//@ run-pass -//@ check-run-results + + // Same as miri's `tests/pass/async-closure-captures.rs`, keep in sync @@ -79,4 +79,40 @@ async fn async_main() { }; call_once(c).await; } + + fn force_fnonce(f: impl async FnOnce() -> T) -> impl async FnOnce() -> T { + f + } + + // Capture something with `move`, but infer to `AsyncFnOnce` + { + let x = Hello(6); + let c = force_fnonce(async move || { + println!("{x:?}"); + }); + call_once(c).await; + + let x = &Hello(7); + let c = force_fnonce(async move || { + println!("{x:?}"); + }); + call_once(c).await; + } + + // Capture something by-ref, but infer to `AsyncFnOnce` + { + let x = Hello(8); + let c = force_fnonce(async || { + println!("{x:?}"); + //~^ ERROR `x` does not live long enough + }); + call_once(c).await; + + let x = &Hello(9); + let c = force_fnonce(async || { + println!("{x:?}"); + //~^ ERROR `x` does not live long enough + }); + call_once(c).await; + } } diff --git a/tests/ui/async-await/async-closures/captures.stderr b/tests/ui/async-await/async-closures/captures.stderr new file mode 100644 index 0000000000000..5893854e57ab5 --- /dev/null +++ b/tests/ui/async-await/async-closures/captures.stderr @@ -0,0 +1,31 @@ +error[E0597]: `x` does not live long enough + --> $DIR/captures.rs:89:24 + | +LL | let c = force_fnonce(async move || { + | ____________________________________________- +LL | | println!("{x:?}"); + | | ^ borrowed value does not live long enough +LL | | }); + | | -- + | | || + | | |`x` dropped here while still borrowed + | |_________|borrow later used here + | value captured here by coroutine + +error[E0597]: `x` does not live long enough + --> $DIR/captures.rs:95:24 + | +LL | let c = force_fnonce(async move || { + | ____________________________________________- +LL | | println!("{x:?}"); + | | ^ borrowed value does not live long enough +LL | | }); + | | -- + | | || + | | |`x` dropped here while still borrowed + | |_________|borrow later used here + | value captured here by coroutine + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0597`. diff --git a/tests/ui/async-await/async-closures/wrong-fn-kind.rs b/tests/ui/async-await/async-closures/wrong-fn-kind.rs index 8502bb6e2d45d..3d6f856874f2c 100644 --- a/tests/ui/async-await/async-closures/wrong-fn-kind.rs +++ b/tests/ui/async-await/async-closures/wrong-fn-kind.rs @@ -2,18 +2,22 @@ #![feature(async_closure)] -fn main() { - fn needs_async_fn(_: impl async Fn()) {} +fn needs_async_fn(_: impl async Fn()) {} +fn a() { let mut x = 1; needs_async_fn(async || { - //~^ ERROR expected a closure that implements the `async Fn` trait, but this closure only implements `async FnMut` + //~^ ERROR cannot borrow `x` as mutable, as it is a captured variable in a `Fn` closure x += 1; }); +} +fn b() { let x = String::new(); needs_async_fn(move || async move { //~^ ERROR expected a closure that implements the `async Fn` trait, but this closure only implements `async FnOnce` println!("{x}"); }); } + +fn main() {} diff --git a/tests/ui/async-await/async-closures/wrong-fn-kind.stderr b/tests/ui/async-await/async-closures/wrong-fn-kind.stderr index d0f1948e48f83..e56389b320273 100644 --- a/tests/ui/async-await/async-closures/wrong-fn-kind.stderr +++ b/tests/ui/async-await/async-closures/wrong-fn-kind.stderr @@ -1,26 +1,5 @@ -error[E0525]: expected a closure that implements the `async Fn` trait, but this closure only implements `async FnMut` - --> $DIR/wrong-fn-kind.rs:9:20 - | -LL | needs_async_fn(async || { - | -------------- -^^^^^^^ - | | | - | _____|______________this closure implements `async FnMut`, not `async Fn` - | | | - | | required by a bound introduced by this call -LL | | -LL | | x += 1; - | | - closure is `async FnMut` because it mutates the variable `x` here -LL | | }); - | |_____- the requirement to implement `async Fn` derives from here - | -note: required by a bound in `needs_async_fn` - --> $DIR/wrong-fn-kind.rs:6:31 - | -LL | fn needs_async_fn(_: impl async Fn()) {} - | ^^^^^^^^^^ required by this bound in `needs_async_fn` - error[E0525]: expected a closure that implements the `async Fn` trait, but this closure only implements `async FnOnce` - --> $DIR/wrong-fn-kind.rs:15:20 + --> $DIR/wrong-fn-kind.rs:17:20 | LL | needs_async_fn(move || async move { | -------------- -^^^^^^ @@ -35,11 +14,29 @@ LL | | }); | |_____- the requirement to implement `async Fn` derives from here | note: required by a bound in `needs_async_fn` - --> $DIR/wrong-fn-kind.rs:6:31 + --> $DIR/wrong-fn-kind.rs:5:27 + | +LL | fn needs_async_fn(_: impl async Fn()) {} + | ^^^^^^^^^^ required by this bound in `needs_async_fn` + +error[E0596]: cannot borrow `x` as mutable, as it is a captured variable in a `Fn` closure + --> $DIR/wrong-fn-kind.rs:9:29 | -LL | fn needs_async_fn(_: impl async Fn()) {} - | ^^^^^^^^^^ required by this bound in `needs_async_fn` +LL | fn needs_async_fn(_: impl async Fn()) {} + | --------------- change this to accept `FnMut` instead of `Fn` +... +LL | needs_async_fn(async || { + | _____--------------_--------_^ + | | | | + | | | in this closure + | | expects `Fn` instead of `FnMut` +LL | | +LL | | x += 1; + | | - mutable borrow occurs due to use of `x` in closure +LL | | }); + | |_____^ cannot borrow as mutable error: aborting due to 2 previous errors -For more information about this error, try `rustc --explain E0525`. +Some errors have detailed explanations: E0525, E0596. +For more information about an error, try `rustc --explain E0525`. From 55e46612c1ccceb30a7a6acf11fd485f34e393e5 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 3 Apr 2024 12:16:17 -0400 Subject: [PATCH 2/2] Force `move` async-closures that are `FnOnce` to make their inner coroutines also `move` --- compiler/rustc_hir_typeck/src/upvar.rs | 24 +++++++++++++- .../src/coroutine/by_move_body.rs | 20 ++++++++---- .../tests/pass/async-closure-captures.stderr | 31 ------------------- .../tests/pass/async-closure-captures.stdout | 14 +++++++++ .../ui/async-await/async-closures/captures.rs | 6 ++-- .../async-closures/captures.run.stdout | 4 +++ .../async-closures/captures.stderr | 31 ------------------- 7 files changed, 57 insertions(+), 73 deletions(-) delete mode 100644 src/tools/miri/tests/pass/async-closure-captures.stderr create mode 100644 src/tools/miri/tests/pass/async-closure-captures.stdout delete mode 100644 tests/ui/async-await/async-closures/captures.stderr diff --git a/compiler/rustc_hir_typeck/src/upvar.rs b/compiler/rustc_hir_typeck/src/upvar.rs index 947fff679191a..c987bfb9a0e88 100644 --- a/compiler/rustc_hir_typeck/src/upvar.rs +++ b/compiler/rustc_hir_typeck/src/upvar.rs @@ -166,7 +166,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { span: Span, body_id: hir::BodyId, body: &'tcx hir::Body<'tcx>, - capture_clause: hir::CaptureBy, + mut capture_clause: hir::CaptureBy, ) { // Extract the type of the closure. let ty = self.node_ty(closure_hir_id); @@ -259,6 +259,28 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ) .consume_body(body); + // If a coroutine is comes from a coroutine-closure that is `move`, but + // the coroutine-closure was inferred to be `FnOnce` during signature + // inference, then it's still possible that we try to borrow upvars from + // the coroutine-closure because they are not used by the coroutine body + // in a way that forces a move. + // + // This would lead to an impossible to satisfy situation, since `AsyncFnOnce` + // coroutine bodies can't borrow from their parent closure. To fix this, + // we force the inner coroutine to also be `move`. This only matters for + // coroutine-closures that are `move` since otherwise they themselves will + // be borrowing from the outer environment, so there's no self-borrows occuring. + if let UpvarArgs::Coroutine(..) = args + && let hir::CoroutineKind::Desugared(_, hir::CoroutineSource::Closure) = + self.tcx.coroutine_kind(closure_def_id).expect("coroutine should have kind") + && let parent_hir_id = + self.tcx.local_def_id_to_hir_id(self.tcx.local_parent(closure_def_id)) + && let parent_ty = self.node_ty(parent_hir_id) + && let Some(ty::ClosureKind::FnOnce) = self.closure_kind(parent_ty) + { + capture_clause = self.tcx.hir_node(parent_hir_id).expect_closure().capture_clause; + } + debug!( "For closure={:?}, capture_information={:#?}", closure_def_id, delegate.capture_information diff --git a/compiler/rustc_mir_transform/src/coroutine/by_move_body.rs b/compiler/rustc_mir_transform/src/coroutine/by_move_body.rs index 0866205dfd0db..de43f9faff909 100644 --- a/compiler/rustc_mir_transform/src/coroutine/by_move_body.rs +++ b/compiler/rustc_mir_transform/src/coroutine/by_move_body.rs @@ -91,15 +91,17 @@ impl<'tcx> MirPass<'tcx> for ByMoveBody { return; } - let ty::Coroutine(_, coroutine_args) = *coroutine_ty.kind() else { bug!("{body:#?}") }; - // We don't need to generate a by-move coroutine if the kind of the coroutine is - // already `FnOnce` -- that means that any upvars that the closure consumes have - // already been taken by-value. - let coroutine_kind = coroutine_args.as_coroutine().kind_ty().to_opt_closure_kind().unwrap(); - if coroutine_kind == ty::ClosureKind::FnOnce { + // We don't need to generate a by-move coroutine if the coroutine body was + // produced by the `CoroutineKindShim`, since it's already by-move. + if matches!(body.source.instance, ty::InstanceDef::CoroutineKindShim { .. }) { return; } + let ty::Coroutine(_, args) = *coroutine_ty.kind() else { bug!("{body:#?}") }; + let args = args.as_coroutine(); + + let coroutine_kind = args.kind_ty().to_opt_closure_kind().unwrap(); + let parent_def_id = tcx.local_parent(coroutine_def_id); let ty::CoroutineClosure(_, parent_args) = *tcx.type_of(parent_def_id).instantiate_identity().kind() @@ -128,6 +130,12 @@ impl<'tcx> MirPass<'tcx> for ByMoveBody { // the outer closure body -- we need to change the coroutine to take the // upvar by value. if coroutine_capture.is_by_ref() && !parent_capture.is_by_ref() { + assert_ne!( + coroutine_kind, + ty::ClosureKind::FnOnce, + "`FnOnce` coroutine-closures return coroutines that capture from \ + their body; it will always result in a borrowck error!" + ); by_ref_fields.insert(FieldIdx::from_usize(num_args + idx)); } diff --git a/src/tools/miri/tests/pass/async-closure-captures.stderr b/src/tools/miri/tests/pass/async-closure-captures.stderr deleted file mode 100644 index f1548aadefa86..0000000000000 --- a/src/tools/miri/tests/pass/async-closure-captures.stderr +++ /dev/null @@ -1,31 +0,0 @@ -error[E0597]: `x` does not live long enough - --> $DIR/async-closure-captures.rs:LL:CC - | -LL | let c = force_fnonce(async move || { - | ____________________________________________- -LL | | println!("{x:?}"); - | | ^ borrowed value does not live long enough -LL | | }); - | | -- - | | || - | | |`x` dropped here while still borrowed - | |_________|borrow later used here - | value captured here by coroutine - -error[E0597]: `x` does not live long enough - --> $DIR/async-closure-captures.rs:LL:CC - | -LL | let c = force_fnonce(async move || { - | ____________________________________________- -LL | | println!("{x:?}"); - | | ^ borrowed value does not live long enough -LL | | }); - | | -- - | | || - | | |`x` dropped here while still borrowed - | |_________|borrow later used here - | value captured here by coroutine - -error: aborting due to 2 previous errors - -For more information about this error, try `rustc --explain E0597`. diff --git a/src/tools/miri/tests/pass/async-closure-captures.stdout b/src/tools/miri/tests/pass/async-closure-captures.stdout new file mode 100644 index 0000000000000..42a7999b2dcdd --- /dev/null +++ b/src/tools/miri/tests/pass/async-closure-captures.stdout @@ -0,0 +1,14 @@ +Hello(0) +Hello(0) +Hello(1) +Hello(1) +Hello(2) +Hello(3) +Hello(3) +Hello(4) +Hello(4) +Hello(5) +Hello(6) +Hello(7) +Hello(8) +Hello(9) diff --git a/tests/ui/async-await/async-closures/captures.rs b/tests/ui/async-await/async-closures/captures.rs index 6011292b6451e..0a9d0529bf542 100644 --- a/tests/ui/async-await/async-closures/captures.rs +++ b/tests/ui/async-await/async-closures/captures.rs @@ -1,7 +1,7 @@ //@ aux-build:block-on.rs //@ edition:2021 - - +//@ run-pass +//@ check-run-results // Same as miri's `tests/pass/async-closure-captures.rs`, keep in sync @@ -104,14 +104,12 @@ async fn async_main() { let x = Hello(8); let c = force_fnonce(async || { println!("{x:?}"); - //~^ ERROR `x` does not live long enough }); call_once(c).await; let x = &Hello(9); let c = force_fnonce(async || { println!("{x:?}"); - //~^ ERROR `x` does not live long enough }); call_once(c).await; } diff --git a/tests/ui/async-await/async-closures/captures.run.stdout b/tests/ui/async-await/async-closures/captures.run.stdout index a0db6d236fef1..42a7999b2dcdd 100644 --- a/tests/ui/async-await/async-closures/captures.run.stdout +++ b/tests/ui/async-await/async-closures/captures.run.stdout @@ -8,3 +8,7 @@ Hello(3) Hello(4) Hello(4) Hello(5) +Hello(6) +Hello(7) +Hello(8) +Hello(9) diff --git a/tests/ui/async-await/async-closures/captures.stderr b/tests/ui/async-await/async-closures/captures.stderr deleted file mode 100644 index 5893854e57ab5..0000000000000 --- a/tests/ui/async-await/async-closures/captures.stderr +++ /dev/null @@ -1,31 +0,0 @@ -error[E0597]: `x` does not live long enough - --> $DIR/captures.rs:89:24 - | -LL | let c = force_fnonce(async move || { - | ____________________________________________- -LL | | println!("{x:?}"); - | | ^ borrowed value does not live long enough -LL | | }); - | | -- - | | || - | | |`x` dropped here while still borrowed - | |_________|borrow later used here - | value captured here by coroutine - -error[E0597]: `x` does not live long enough - --> $DIR/captures.rs:95:24 - | -LL | let c = force_fnonce(async move || { - | ____________________________________________- -LL | | println!("{x:?}"); - | | ^ borrowed value does not live long enough -LL | | }); - | | -- - | | || - | | |`x` dropped here while still borrowed - | |_________|borrow later used here - | value captured here by coroutine - -error: aborting due to 2 previous errors - -For more information about this error, try `rustc --explain E0597`.