From 8771e8d8c3fb613086b3e8a5c98473e4f49dc48b Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 8 Mar 2022 13:49:34 -0800 Subject: [PATCH 1/3] Allow uninhabited types in generator interiors Drop tracking in the generator interior type checking pass would count all values in unreachable code as dropped (e.g. code after a call to a function with an uninhabited return type), which would lead to those values not being included in the witness type. This resulted in the type checker being more precise than the corresponding sanity check in the MIR transform. This patch changes the check in the MIR code to match the results of typeck by skipping uninhabited types. It also reduces the test case that was added earlier. --- compiler/rustc_mir_transform/src/generator.rs | 8 +- src/test/ui/generator/issue-93161.rs | 73 +------------------ 2 files changed, 10 insertions(+), 71 deletions(-) diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index d9a66cace52e5..77f0acb5437df 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -747,9 +747,15 @@ fn sanitize_witness<'tcx>( } let decl_ty = tcx.normalize_erasing_regions(param_env, decl.ty); + let is_uninhabited = tcx.is_ty_uninhabited_from( + tcx.parent_module(tcx.hir().local_def_id_to_hir_id(did.expect_local())).to_def_id(), + decl_ty, + param_env, + ); + // Sanity check that typeck knows about the type of locals which are // live across a suspension point - if !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) { + if !is_uninhabited && !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) { span_bug!( body.span, "Broken MIR: generator contains type {} in MIR, \ diff --git a/src/test/ui/generator/issue-93161.rs b/src/test/ui/generator/issue-93161.rs index 9988acbcb73e1..d2640d278270a 100644 --- a/src/test/ui/generator/issue-93161.rs +++ b/src/test/ui/generator/issue-93161.rs @@ -1,31 +1,9 @@ // edition:2021 -// run-pass +// build-pass +// compile-flags: -Zdrop-tracking #![feature(never_type)] -use std::future::Future; - -// See if we can run a basic `async fn` -pub async fn foo(x: &u32, y: u32) -> u32 { - let y = &y; - let z = 9; - let z = &z; - let y = async { *y + *z }.await; - let a = 10; - let a = &a; - *x + y + *a -} - -async fn add(x: u32, y: u32) -> u32 { - let a = async { x + y }; - a.await -} - -async fn build_aggregate(a: u32, b: u32, c: u32, d: u32) -> u32 { - let x = (add(a, b).await, add(c, d).await); - x.0 + x.1 -} - enum Never {} fn never() -> Never { panic!() @@ -43,51 +21,6 @@ async fn includes_never(crash: bool, x: u32) -> u32 { result } -async fn partial_init(x: u32) -> u32 { - #[allow(unreachable_code)] - let _x: (String, !) = (String::new(), return async { x + x }.await); -} - -async fn read_exact(_from: &mut &[u8], _to: &mut [u8]) -> Option<()> { - Some(()) -} - -async fn hello_world() { - let data = [0u8; 1]; - let mut reader = &data[..]; - - let mut marker = [0u8; 1]; - read_exact(&mut reader, &mut marker).await.unwrap(); -} - -fn run_fut(fut: impl Future) -> T { - use std::sync::Arc; - use std::task::{Context, Poll, Wake, Waker}; - - struct MyWaker; - impl Wake for MyWaker { - fn wake(self: Arc) { - unimplemented!() - } - } - - let waker = Waker::from(Arc::new(MyWaker)); - let mut context = Context::from_waker(&waker); - - let mut pinned = Box::pin(fut); - loop { - match pinned.as_mut().poll(&mut context) { - Poll::Pending => continue, - Poll::Ready(v) => return v, - } - } -} - fn main() { - let x = 5; - assert_eq!(run_fut(foo(&x, 7)), 31); - assert_eq!(run_fut(build_aggregate(1, 2, 3, 4)), 10); - assert_eq!(run_fut(includes_never(false, 4)), 16); - assert_eq!(run_fut(partial_init(4)), 8); - run_fut(hello_world()); + let _ = includes_never(false, 4); } From 020bc4da348336a0f65d16d4e371326ab9db5b1e Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 9 Mar 2022 11:03:49 -0800 Subject: [PATCH 2/3] Remove uninhabited type optimization --- compiler/rustc_mir_transform/src/generator.rs | 8 +-- .../check/generator_interior/drop_ranges.rs | 2 - .../drop_ranges/cfg_build.rs | 54 +++---------------- src/test/ui/async-await/async-fn-nonsend.rs | 8 --- .../ui/async-await/async-fn-nonsend.stderr | 8 +-- src/test/ui/async-await/drop-never.rs | 17 ++++++ .../{generator => async-await}/issue-93161.rs | 0 7 files changed, 29 insertions(+), 68 deletions(-) create mode 100644 src/test/ui/async-await/drop-never.rs rename src/test/ui/{generator => async-await}/issue-93161.rs (100%) diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index 77f0acb5437df..d9a66cace52e5 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -747,15 +747,9 @@ fn sanitize_witness<'tcx>( } let decl_ty = tcx.normalize_erasing_regions(param_env, decl.ty); - let is_uninhabited = tcx.is_ty_uninhabited_from( - tcx.parent_module(tcx.hir().local_def_id_to_hir_id(did.expect_local())).to_def_id(), - decl_ty, - param_env, - ); - // Sanity check that typeck knows about the type of locals which are // live across a suspension point - if !is_uninhabited && !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) { + if !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) { span_bug!( body.span, "Broken MIR: generator contains type {} in MIR, \ diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs index 972dd622d6e98..600afa3858ebe 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs @@ -43,8 +43,6 @@ pub fn compute_drop_ranges<'a, 'tcx>( let num_exprs = fcx.tcx.region_scope_tree(def_id).body_expr_count(body.id()).unwrap_or(0); let mut drop_ranges = build_control_flow_graph( fcx.tcx.hir(), - fcx.tcx, - &fcx.typeck_results.borrow(), consumed_borrowed_places, body, num_exprs, diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index cfed784ea728b..2841dcbad811a 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -9,10 +9,7 @@ use hir::{ use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; use rustc_index::vec::IndexVec; -use rustc_middle::{ - hir::map::Map, - ty::{TyCtxt, TypeckResults}, -}; +use rustc_middle::hir::map::Map; use std::mem::swap; /// Traverses the body to find the control flow graph and locations for the @@ -22,14 +19,11 @@ use std::mem::swap; /// can be done with propagate_to_fixpoint in cfg_propagate. pub(super) fn build_control_flow_graph<'tcx>( hir: Map<'tcx>, - tcx: TyCtxt<'tcx>, - typeck_results: &TypeckResults<'tcx>, consumed_borrowed_places: ConsumedAndBorrowedPlaces, body: &'tcx Body<'tcx>, num_exprs: usize, ) -> DropRangesBuilder { - let mut drop_range_visitor = - DropRangeVisitor::new(hir, tcx, typeck_results, consumed_borrowed_places, num_exprs); + let mut drop_range_visitor = DropRangeVisitor::new(hir, consumed_borrowed_places, num_exprs); intravisit::walk_body(&mut drop_range_visitor, body); drop_range_visitor.drop_ranges.process_deferred_edges(); @@ -78,39 +72,23 @@ pub(super) fn build_control_flow_graph<'tcx>( /// // all of `a` is considered initialized /// ``` -struct DropRangeVisitor<'a, 'tcx> { +struct DropRangeVisitor<'tcx> { hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, drop_ranges: DropRangesBuilder, expr_index: PostOrderId, - tcx: TyCtxt<'tcx>, - typeck_results: &'a TypeckResults<'tcx>, label_stack: Vec<(Option, PostOrderId)>, } -impl<'a, 'tcx> DropRangeVisitor<'a, 'tcx> { - fn new( - hir: Map<'tcx>, - tcx: TyCtxt<'tcx>, - typeck_results: &'a TypeckResults<'tcx>, - places: ConsumedAndBorrowedPlaces, - num_exprs: usize, - ) -> Self { +impl<'tcx> DropRangeVisitor<'tcx> { + fn new(hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, num_exprs: usize) -> Self { debug!("consumed_places: {:?}", places.consumed); let drop_ranges = DropRangesBuilder::new( places.consumed.iter().flat_map(|(_, places)| places.iter().cloned()), hir, num_exprs, ); - Self { - hir, - places, - drop_ranges, - expr_index: PostOrderId::from_u32(0), - typeck_results, - tcx, - label_stack: vec![], - } + Self { hir, places, drop_ranges, expr_index: PostOrderId::from_u32(0), label_stack: vec![] } } fn record_drop(&mut self, value: TrackedValue) { @@ -205,20 +183,6 @@ impl<'a, 'tcx> DropRangeVisitor<'a, 'tcx> { } } - /// For an expression with an uninhabited return type (e.g. a function that returns !), - /// this adds a self edge to to the CFG to model the fact that the function does not - /// return. - fn handle_uninhabited_return(&mut self, expr: &Expr<'tcx>) { - let ty = self.typeck_results.expr_ty(expr); - let ty = self.tcx.erase_regions(ty); - let m = self.tcx.parent_module(expr.hir_id).to_def_id(); - let param_env = self.tcx.param_env(m.expect_local()); - if self.tcx.is_ty_uninhabited_from(m, ty, param_env) { - // This function will not return. We model this fact as an infinite loop. - self.drop_ranges.add_control_edge(self.expr_index + 1, self.expr_index + 1); - } - } - /// Map a Destination to an equivalent expression node /// /// The destination field of a Break or Continue expression can target either an @@ -274,7 +238,7 @@ fn find_last_block_expression(block: &hir::Block<'_>) -> HirId { ) } -impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> { +impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { let mut reinit = None; match expr.kind { @@ -427,15 +391,11 @@ impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> { for arg in args { self.visit_expr(arg); } - - self.handle_uninhabited_return(expr); } ExprKind::MethodCall(_, exprs, _) => { for expr in exprs { self.visit_expr(expr); } - - self.handle_uninhabited_return(expr); } ExprKind::AddrOf(..) diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs index d7f8d7ac546c0..df8f93236419b 100644 --- a/src/test/ui/async-await/async-fn-nonsend.rs +++ b/src/test/ui/async-await/async-fn-nonsend.rs @@ -47,13 +47,6 @@ async fn non_sync_with_method_call() { } } -async fn non_sync_with_method_call_panic() { - let f: &mut std::fmt::Formatter = panic!(); - if non_sync().fmt(f).unwrap() == () { - fut().await; - } -} - async fn non_sync_with_method_call_infinite_loop() { let f: &mut std::fmt::Formatter = loop {}; if non_sync().fmt(f).unwrap() == () { @@ -69,6 +62,5 @@ pub fn pass_assert() { //~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call()); //~^ ERROR future cannot be sent between threads safely - assert_send(non_sync_with_method_call_panic()); assert_send(non_sync_with_method_call_infinite_loop()); } diff --git a/src/test/ui/async-await/async-fn-nonsend.stderr b/src/test/ui/async-await/async-fn-nonsend.stderr index 40ad46b48620d..e0a47bbdb84d5 100644 --- a/src/test/ui/async-await/async-fn-nonsend.stderr +++ b/src/test/ui/async-await/async-fn-nonsend.stderr @@ -1,5 +1,5 @@ error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:68:17 + --> $DIR/async-fn-nonsend.rs:61:17 | LL | assert_send(non_send_temporary_in_match()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_send_temporary_in_match` is not `Send` @@ -16,13 +16,13 @@ LL | Some(_) => fut().await, LL | } | - `Some(non_send())` is later dropped here note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:64:24 + --> $DIR/async-fn-nonsend.rs:57:24 | LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:70:17 + --> $DIR/async-fn-nonsend.rs:63:17 | LL | assert_send(non_sync_with_method_call()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_sync_with_method_call` is not `Send` @@ -40,7 +40,7 @@ LL | } LL | } | - `get_formatter()` is later dropped here note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:64:24 + --> $DIR/async-fn-nonsend.rs:57:24 | LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` diff --git a/src/test/ui/async-await/drop-never.rs b/src/test/ui/async-await/drop-never.rs new file mode 100644 index 0000000000000..ac82b97df9ccd --- /dev/null +++ b/src/test/ui/async-await/drop-never.rs @@ -0,0 +1,17 @@ +// build-pass +// compile-flags: -Zdrop-tracking +// edition:2021 + +enum Never {} + +fn f() -> Never { todo!() } + +fn main() { + let _ = async { + let a = String::new(); + let b = f(); + async {}.await; + drop(a); + drop(b); + }; +} diff --git a/src/test/ui/generator/issue-93161.rs b/src/test/ui/async-await/issue-93161.rs similarity index 100% rename from src/test/ui/generator/issue-93161.rs rename to src/test/ui/async-await/issue-93161.rs From 1c758745a38e8c354d5b17ff471b3b45356b2d95 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 9 Mar 2022 11:17:44 -0800 Subject: [PATCH 3/3] Restore uninhabited optimization and update MIR check --- compiler/rustc_mir_transform/src/generator.rs | 44 +++++++++------ .../check/generator_interior/drop_ranges.rs | 2 + .../drop_ranges/cfg_build.rs | 54 ++++++++++++++++--- 3 files changed, 77 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index d9a66cace52e5..0984829e05e48 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -740,24 +740,36 @@ fn sanitize_witness<'tcx>( } }; - for (local, decl) in body.local_decls.iter_enumerated() { - // Ignore locals which are internal or not saved between yields. - if !saved_locals.contains(local) || decl.internal { - continue; - } - let decl_ty = tcx.normalize_erasing_regions(param_env, decl.ty); + // First check if any of the locals are an uninhabited type. If so, we don't need to do the + // rest of this check because this code unreachable. + let any_uninhabited = body.local_decls.iter_enumerated().any(|(_, decl)| { + tcx.is_ty_uninhabited_from( + tcx.parent_module(tcx.hir().local_def_id_to_hir_id(did.expect_local())).to_def_id(), + tcx.normalize_erasing_regions(param_env, decl.ty), + param_env, + ) + }); - // Sanity check that typeck knows about the type of locals which are - // live across a suspension point - if !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) { - span_bug!( - body.span, - "Broken MIR: generator contains type {} in MIR, \ + if !any_uninhabited { + for (local, decl) in body.local_decls.iter_enumerated() { + // Ignore locals which are internal or not saved between yields. + if !saved_locals.contains(local) || decl.internal { + continue; + } + let decl_ty = tcx.normalize_erasing_regions(param_env, decl.ty); + + // Sanity check that typeck knows about the type of locals which are + // live across a suspension point + if !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) { + span_bug!( + body.span, + "Broken MIR: generator contains type {} in MIR, \ but typeck only knows about {} and {:?}", - decl_ty, - allowed, - allowed_upvars - ); + decl_ty, + allowed, + allowed_upvars + ); + } } } } diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs index 600afa3858ebe..972dd622d6e98 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs @@ -43,6 +43,8 @@ pub fn compute_drop_ranges<'a, 'tcx>( let num_exprs = fcx.tcx.region_scope_tree(def_id).body_expr_count(body.id()).unwrap_or(0); let mut drop_ranges = build_control_flow_graph( fcx.tcx.hir(), + fcx.tcx, + &fcx.typeck_results.borrow(), consumed_borrowed_places, body, num_exprs, diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index 2841dcbad811a..cfed784ea728b 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -9,7 +9,10 @@ use hir::{ use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; use rustc_index::vec::IndexVec; -use rustc_middle::hir::map::Map; +use rustc_middle::{ + hir::map::Map, + ty::{TyCtxt, TypeckResults}, +}; use std::mem::swap; /// Traverses the body to find the control flow graph and locations for the @@ -19,11 +22,14 @@ use std::mem::swap; /// can be done with propagate_to_fixpoint in cfg_propagate. pub(super) fn build_control_flow_graph<'tcx>( hir: Map<'tcx>, + tcx: TyCtxt<'tcx>, + typeck_results: &TypeckResults<'tcx>, consumed_borrowed_places: ConsumedAndBorrowedPlaces, body: &'tcx Body<'tcx>, num_exprs: usize, ) -> DropRangesBuilder { - let mut drop_range_visitor = DropRangeVisitor::new(hir, consumed_borrowed_places, num_exprs); + let mut drop_range_visitor = + DropRangeVisitor::new(hir, tcx, typeck_results, consumed_borrowed_places, num_exprs); intravisit::walk_body(&mut drop_range_visitor, body); drop_range_visitor.drop_ranges.process_deferred_edges(); @@ -72,23 +78,39 @@ pub(super) fn build_control_flow_graph<'tcx>( /// // all of `a` is considered initialized /// ``` -struct DropRangeVisitor<'tcx> { +struct DropRangeVisitor<'a, 'tcx> { hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, drop_ranges: DropRangesBuilder, expr_index: PostOrderId, + tcx: TyCtxt<'tcx>, + typeck_results: &'a TypeckResults<'tcx>, label_stack: Vec<(Option, PostOrderId)>, } -impl<'tcx> DropRangeVisitor<'tcx> { - fn new(hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, num_exprs: usize) -> Self { +impl<'a, 'tcx> DropRangeVisitor<'a, 'tcx> { + fn new( + hir: Map<'tcx>, + tcx: TyCtxt<'tcx>, + typeck_results: &'a TypeckResults<'tcx>, + places: ConsumedAndBorrowedPlaces, + num_exprs: usize, + ) -> Self { debug!("consumed_places: {:?}", places.consumed); let drop_ranges = DropRangesBuilder::new( places.consumed.iter().flat_map(|(_, places)| places.iter().cloned()), hir, num_exprs, ); - Self { hir, places, drop_ranges, expr_index: PostOrderId::from_u32(0), label_stack: vec![] } + Self { + hir, + places, + drop_ranges, + expr_index: PostOrderId::from_u32(0), + typeck_results, + tcx, + label_stack: vec![], + } } fn record_drop(&mut self, value: TrackedValue) { @@ -183,6 +205,20 @@ impl<'tcx> DropRangeVisitor<'tcx> { } } + /// For an expression with an uninhabited return type (e.g. a function that returns !), + /// this adds a self edge to to the CFG to model the fact that the function does not + /// return. + fn handle_uninhabited_return(&mut self, expr: &Expr<'tcx>) { + let ty = self.typeck_results.expr_ty(expr); + let ty = self.tcx.erase_regions(ty); + let m = self.tcx.parent_module(expr.hir_id).to_def_id(); + let param_env = self.tcx.param_env(m.expect_local()); + if self.tcx.is_ty_uninhabited_from(m, ty, param_env) { + // This function will not return. We model this fact as an infinite loop. + self.drop_ranges.add_control_edge(self.expr_index + 1, self.expr_index + 1); + } + } + /// Map a Destination to an equivalent expression node /// /// The destination field of a Break or Continue expression can target either an @@ -238,7 +274,7 @@ fn find_last_block_expression(block: &hir::Block<'_>) -> HirId { ) } -impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { +impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { let mut reinit = None; match expr.kind { @@ -391,11 +427,15 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> { for arg in args { self.visit_expr(arg); } + + self.handle_uninhabited_return(expr); } ExprKind::MethodCall(_, exprs, _) => { for expr in exprs { self.visit_expr(expr); } + + self.handle_uninhabited_return(expr); } ExprKind::AddrOf(..)