From 34faed0e78ad9a693e724a94d677613e4990ebee Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Tue, 13 Dec 2022 21:13:47 +0100 Subject: [PATCH] Do not consider async `task_context` to be alive across yields When lowering async constructs to generators, the resume argument is guaranteed not to be alive across yield points. However the simple `generator_interior` analysis thinks it is. Because of that, the resume ty was part of the `GeneratorWitness` and considered to be part of the generator type, even though it is not really. This prevented async blocks from being `UnwindSafe`, and possibly `Send` in some cases. The code now special cases the fact that the `task_context` of async blocks is never alive across yield points. --- .../src/generator_interior/mod.rs | 61 ++++++++++++++++++- .../async-await-let-else.drop-tracking.stderr | 2 +- .../ui/async-await/async-is-unwindsafe.rs | 31 ++++++++++ .../ui/async-await/async-is-unwindsafe.stderr | 38 ++++++++++++ .../issue-68112.drop_tracking.stderr | 2 +- .../issue-68112.no_drop_tracking.stderr | 2 +- ...e-70935-complex-spans.drop_tracking.stderr | 2 +- ...l-drop-partial-reinit.drop_tracking.stderr | 2 +- ...rop-partial-reinit.no_drop_tracking.stderr | 2 +- 9 files changed, 135 insertions(+), 7 deletions(-) create mode 100644 src/test/ui/async-await/async-is-unwindsafe.rs create mode 100644 src/test/ui/async-await/async-is-unwindsafe.stderr diff --git a/compiler/rustc_hir_typeck/src/generator_interior/mod.rs b/compiler/rustc_hir_typeck/src/generator_interior/mod.rs index d83b9eb995d2b..b7e6a8ad9609c 100644 --- a/compiler/rustc_hir_typeck/src/generator_interior/mod.rs +++ b/compiler/rustc_hir_typeck/src/generator_interior/mod.rs @@ -33,6 +33,8 @@ struct InteriorVisitor<'a, 'tcx> { prev_unresolved_span: Option, linted_values: HirIdSet, drop_ranges: DropRanges, + task_context_hir_id: Option, + in_lowered_await: bool, } impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { @@ -53,7 +55,7 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { ty, hir_id, scope, expr, source_span, self.expr_count, ); - let live_across_yield = scope + let mut live_across_yield = scope .map(|s| { self.region_scope_tree.yield_in_scope(s).and_then(|yield_data| { // If we are recording an expression that is the last yield @@ -92,6 +94,34 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> { Some(YieldData { span: DUMMY_SP, expr_and_pat_count: 0, source: self.kind.into() }) }); + // If this is the `&mut Context<'_>` async resume argument, or we are + // just visiting a `_task_context = yield ()` expression from async + // lowering, we do *not* consider this type to be live across yields. + if Some(hir_id) == self.task_context_hir_id || self.in_lowered_await { + #[cfg(debug_assertions)] + { + // As `record` is being invoked for multiple parts / types of the + // lowered `.await`, the `ty` has to either be the `()` going *into* + // the `yield`, or a `&mut Context<'_>` coming *out* of it. + let tcx = self.fcx.tcx; + if ty == tcx.types.unit { + // all good + } else if let ty::Ref(_, adt, hir::Mutability::Mut) = ty.kind() && let ty::Adt(adt, _) = adt.kind() + { + let context_def_id = tcx.lang_items().context(); + assert_eq!( + Some(adt.did()), + context_def_id, + "expected `&mut Context<'_>, found `{:?}` instead`", + ty + ); + } else { + panic!("expected `()` or `&mut Context<'_>`, found `{:?}` instead", ty); + } + } + live_across_yield = None; + } + if let Some(yield_data) = live_across_yield { debug!( "type in expr = {:?}, scope = {:?}, type = {:?}, count = {}, yield_span = {:?}", @@ -183,6 +213,17 @@ pub fn resolve_interior<'a, 'tcx>( kind: hir::GeneratorKind, ) { let body = fcx.tcx.hir().body(body_id); + + // In case we are in an async block, this is the Param/Pat HirId of the + // `&mut Context<'_>` resume type. We can use this to explicitly prevent it + // from being considered as `live_across_yield`, which it is not, but the + // simple scope-based analysis can't tell. + let task_context_hir_id = if matches!(kind, hir::GeneratorKind::Async(_)) { + Some(body.params[0].pat.hir_id) + } else { + None + }; + let typeck_results = fcx.inh.typeck_results.borrow(); let mut visitor = InteriorVisitor { fcx, @@ -194,6 +235,8 @@ pub fn resolve_interior<'a, 'tcx>( prev_unresolved_span: None, linted_values: <_>::default(), drop_ranges: drop_ranges::compute_drop_ranges(fcx, def_id, body), + task_context_hir_id, + in_lowered_await: false, }; intravisit::walk_body(&mut visitor, body); @@ -426,6 +469,22 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { } _ => intravisit::walk_expr(self, expr), }, + ExprKind::Assign(_, rhs, _) => { + // An `.await` expression will be lowered to `_task_context = yield ()`. + // In that case, other forms of `yield` are considered errors in lowering. + if self.task_context_hir_id.is_some() + && matches!(rhs.kind, hir::ExprKind::Yield(..)) + { + assert!(!self.in_lowered_await); + self.in_lowered_await = true; + } + // We are still walking the whole expression including its types. + // First, we need to keep `expr_count` in sync as it is asserted + // at the very end, and to keep all the other computations in + // place just in case they are causing other side effects. + intravisit::walk_expr(self, expr); + self.in_lowered_await = false; + } _ => intravisit::walk_expr(self, expr), } diff --git a/src/test/ui/async-await/async-await-let-else.drop-tracking.stderr b/src/test/ui/async-await/async-await-let-else.drop-tracking.stderr index f0f5245a3b42b..9d4777d1ed4b9 100644 --- a/src/test/ui/async-await/async-await-let-else.drop-tracking.stderr +++ b/src/test/ui/async-await/async-await-let-else.drop-tracking.stderr @@ -40,7 +40,7 @@ LL | async fn bar2(_: T) -> ! { LL | | panic!() LL | | } | |_^ - = note: required because it captures the following types: `&mut Context<'_>`, `Option`, `impl Future`, `()` + = note: required because it captures the following types: `Option`, `impl Future`, `()` note: required because it's used within this `async fn` body --> $DIR/async-await-let-else.rs:21:32 | diff --git a/src/test/ui/async-await/async-is-unwindsafe.rs b/src/test/ui/async-await/async-is-unwindsafe.rs new file mode 100644 index 0000000000000..7c2c8a856353f --- /dev/null +++ b/src/test/ui/async-await/async-is-unwindsafe.rs @@ -0,0 +1,31 @@ +// edition:2018 + +fn is_unwindsafe(_: impl std::panic::UnwindSafe) {} + +fn main() { + // a normal async block that uses `&mut Context<'_>` implicitly via async lowering is fine + // as we should not consider that to be alive across an await point + is_unwindsafe(async { + async {}.await; // this needs an inner await point + }); + + is_unwindsafe(async { + //~^ ERROR the type `&mut Context<'_>` may not be safely transferred across an unwind boundary + use std::ptr::null; + use std::task::{Context, RawWaker, RawWakerVTable, Waker}; + let waker = unsafe { + Waker::from_raw(RawWaker::new( + null(), + &RawWakerVTable::new(|_| todo!(), |_| todo!(), |_| todo!(), |_| todo!()), + )) + }; + let mut cx = Context::from_waker(&waker); + let cx_ref = &mut cx; + + async {}.await; // this needs an inner await point + + // in this case, `&mut Context<'_>` is *truely* alive across an await point + drop(cx_ref); + drop(cx); + }); +} diff --git a/src/test/ui/async-await/async-is-unwindsafe.stderr b/src/test/ui/async-await/async-is-unwindsafe.stderr new file mode 100644 index 0000000000000..70e4a5c8ff880 --- /dev/null +++ b/src/test/ui/async-await/async-is-unwindsafe.stderr @@ -0,0 +1,38 @@ +error[E0277]: the type `&mut Context<'_>` may not be safely transferred across an unwind boundary + --> $DIR/async-is-unwindsafe.rs:12:19 + | +LL | is_unwindsafe(async { + | ___________________^ +LL | | +LL | | use std::ptr::null; +LL | | use std::task::{Context, RawWaker, RawWakerVTable, Waker}; +... | +LL | | drop(cx); +LL | | }); + | | ^ + | | | + | |_____`&mut Context<'_>` may not be safely transferred across an unwind boundary + | within this `[async block@$DIR/async-is-unwindsafe.rs:12:19: 30:6]` + | + = help: within `[async block@$DIR/async-is-unwindsafe.rs:12:19: 30:6]`, the trait `UnwindSafe` is not implemented for `&mut Context<'_>` + = note: `UnwindSafe` is implemented for `&std::task::Context<'_>`, but not for `&mut std::task::Context<'_>` +note: future does not implement `UnwindSafe` as this value is used across an await + --> $DIR/async-is-unwindsafe.rs:25:17 + | +LL | let cx_ref = &mut cx; + | ------ has type `&mut Context<'_>` which does not implement `UnwindSafe` +LL | +LL | async {}.await; // this needs an inner await point + | ^^^^^^ await occurs here, with `cx_ref` maybe used later +... +LL | }); + | - `cx_ref` is later dropped here +note: required by a bound in `is_unwindsafe` + --> $DIR/async-is-unwindsafe.rs:3:26 + | +LL | fn is_unwindsafe(_: impl std::panic::UnwindSafe) {} + | ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_unwindsafe` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0277`. diff --git a/src/test/ui/async-await/issue-68112.drop_tracking.stderr b/src/test/ui/async-await/issue-68112.drop_tracking.stderr index 1c90bedae790c..cae702ba1989d 100644 --- a/src/test/ui/async-await/issue-68112.drop_tracking.stderr +++ b/src/test/ui/async-await/issue-68112.drop_tracking.stderr @@ -57,7 +57,7 @@ note: required because it appears within the type `impl Future impl Future>> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: required because it captures the following types: `&mut Context<'_>`, `impl Future>>`, `()`, `Ready` + = note: required because it captures the following types: `impl Future>>`, `()`, `Ready` note: required because it's used within this `async` block --> $DIR/issue-68112.rs:60:20 | diff --git a/src/test/ui/async-await/issue-68112.no_drop_tracking.stderr b/src/test/ui/async-await/issue-68112.no_drop_tracking.stderr index e09ae7fedd805..476e8046a00f2 100644 --- a/src/test/ui/async-await/issue-68112.no_drop_tracking.stderr +++ b/src/test/ui/async-await/issue-68112.no_drop_tracking.stderr @@ -57,7 +57,7 @@ note: required because it appears within the type `impl Future impl Future>> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: required because it captures the following types: `&mut Context<'_>`, `impl Future>>`, `()`, `i32`, `Ready` + = note: required because it captures the following types: `impl Future>>`, `i32`, `Ready` note: required because it's used within this `async` block --> $DIR/issue-68112.rs:60:20 | diff --git a/src/test/ui/async-await/issue-70935-complex-spans.drop_tracking.stderr b/src/test/ui/async-await/issue-70935-complex-spans.drop_tracking.stderr index a8fd97cde8f7a..3210306bb06a6 100644 --- a/src/test/ui/async-await/issue-70935-complex-spans.drop_tracking.stderr +++ b/src/test/ui/async-await/issue-70935-complex-spans.drop_tracking.stderr @@ -18,7 +18,7 @@ LL | async fn baz(_c: impl FnMut() -> T) where T: Future { | ___________________________________________________________________^ LL | | } | |_^ - = note: required because it captures the following types: `&mut Context<'_>`, `impl Future`, `()` + = note: required because it captures the following types: `impl Future`, `()` note: required because it's used within this `async` block --> $DIR/issue-70935-complex-spans.rs:16:5 | diff --git a/src/test/ui/async-await/partial-drop-partial-reinit.drop_tracking.stderr b/src/test/ui/async-await/partial-drop-partial-reinit.drop_tracking.stderr index 25876d5084015..d46df55d84626 100644 --- a/src/test/ui/async-await/partial-drop-partial-reinit.drop_tracking.stderr +++ b/src/test/ui/async-await/partial-drop-partial-reinit.drop_tracking.stderr @@ -11,7 +11,7 @@ LL | async fn foo() { | = help: within `impl Future`, the trait `Send` is not implemented for `NotSend` = note: required because it appears within the type `(NotSend,)` - = note: required because it captures the following types: `&mut Context<'_>`, `(NotSend,)`, `()`, `impl Future` + = note: required because it captures the following types: `(NotSend,)`, `()`, `impl Future` note: required because it's used within this `async fn` body --> $DIR/partial-drop-partial-reinit.rs:31:16 | diff --git a/src/test/ui/async-await/partial-drop-partial-reinit.no_drop_tracking.stderr b/src/test/ui/async-await/partial-drop-partial-reinit.no_drop_tracking.stderr index dba2a620779f0..7fdf389cbdb05 100644 --- a/src/test/ui/async-await/partial-drop-partial-reinit.no_drop_tracking.stderr +++ b/src/test/ui/async-await/partial-drop-partial-reinit.no_drop_tracking.stderr @@ -11,7 +11,7 @@ LL | async fn foo() { | = help: within `impl Future`, the trait `Send` is not implemented for `NotSend` = note: required because it appears within the type `(NotSend,)` - = note: required because it captures the following types: `&mut Context<'_>`, `(NotSend,)`, `impl Future`, `()` + = note: required because it captures the following types: `(NotSend,)`, `impl Future` note: required because it's used within this `async fn` body --> $DIR/partial-drop-partial-reinit.rs:31:16 |