From ab1a3f09fd3e1dbce850b9e1b691dc8338584e65 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 16 Sep 2019 13:06:07 -0400 Subject: [PATCH 1/7] add test for drop order of temporary in tail return expression --- ...order-for-temporary-in-tail-return-expr.rs | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 src/test/ui/async-await/drop-order/drop-order-for-temporary-in-tail-return-expr.rs diff --git a/src/test/ui/async-await/drop-order/drop-order-for-temporary-in-tail-return-expr.rs b/src/test/ui/async-await/drop-order/drop-order-for-temporary-in-tail-return-expr.rs new file mode 100644 index 0000000000000..e250506cdf0af --- /dev/null +++ b/src/test/ui/async-await/drop-order/drop-order-for-temporary-in-tail-return-expr.rs @@ -0,0 +1,94 @@ +// aux-build:arc_wake.rs +// edition:2018 +// run-pass + +#![allow(unused_variables)] + +// Test that the drop order for parameters in a fn and async fn matches up. Also test that +// parameters (used or unused) are not dropped until the async fn completes execution. +// See also #54716. + +extern crate arc_wake; + +use arc_wake::ArcWake; +use std::cell::RefCell; +use std::future::Future; +use std::sync::Arc; +use std::rc::Rc; +use std::task::Context; + +struct EmptyWaker; + +impl ArcWake for EmptyWaker { + fn wake(self: Arc) {} +} + +#[derive(Debug, Eq, PartialEq)] +enum DropOrder { + Function, + Val(&'static str), +} + +type DropOrderListPtr = Rc>>; + +struct D(&'static str, DropOrderListPtr); + +impl Drop for D { + fn drop(&mut self) { + self.1.borrow_mut().push(DropOrder::Val(self.0)); + } +} + +/// Check drop order of temporary "temp" as compared to x, y, and z. +/// +/// Expected order: +/// - z +/// - temp +/// - y +/// - x +async fn foo_async(x: D, _y: D) { + let l = x.1.clone(); + let z = D("z", l.clone()); + l.borrow_mut().push(DropOrder::Function); + helper_async(&D("temp", l)).await +} + +async fn helper_async(v: &D) { } + +fn foo_sync(x: D, _y: D) { + let l = x.1.clone(); + let z = D("z", l.clone()); + l.borrow_mut().push(DropOrder::Function); + helper_sync(&D("temp", l)) +} + +fn helper_sync(v: &D) { } + +fn assert_drop_order_after_poll>( + f: impl FnOnce(DropOrderListPtr) -> Fut, + g: impl FnOnce(DropOrderListPtr), +) { + let empty = Arc::new(EmptyWaker); + let waker = ArcWake::into_waker(empty); + let mut cx = Context::from_waker(&waker); + + let actual_order = Rc::new(RefCell::new(Vec::new())); + let mut fut = Box::pin(f(actual_order.clone())); + let r = fut.as_mut().poll(&mut cx); + + assert!(match r { + std::task::Poll::Ready(()) => true, + _ => false, + }); + + let expected_order = Rc::new(RefCell::new(Vec::new())); + g(expected_order.clone()); + + assert_eq!(*actual_order.borrow(), *expected_order.borrow()); +} + +fn main() { + // Free functions (see doc comment on function for what it tests). + assert_drop_order_after_poll(|l| foo_async(D("x", l.clone()), D("_y", l.clone())), + |l| foo_sync(D("x", l.clone()), D("_y", l.clone()))); +} From 9ae1a664f7b947dadb9e97eea7703831d9cd5d31 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 16 Sep 2019 15:25:39 -0400 Subject: [PATCH 2/7] add regression test for issue-64391 --- src/test/ui/async-await/issue-64391.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 src/test/ui/async-await/issue-64391.rs diff --git a/src/test/ui/async-await/issue-64391.rs b/src/test/ui/async-await/issue-64391.rs new file mode 100644 index 0000000000000..c6faad3aad064 --- /dev/null +++ b/src/test/ui/async-await/issue-64391.rs @@ -0,0 +1,14 @@ +// Regression test for Issue #64391. The goal here is that this +// function compiles. In the past, due to incorrect drop order for +// temporaries in the tail expression, we failed to compile this +// example. The drop order itself is directly tested in +// `drop-order/drop-order-for-temporary-in-tail-return-expr.rs`. +// +// check-pass +// edition:2018 + +async fn add(x: u32, y: u32) -> u32 { + async { x + y }.await +} + +fn main() { } From 00d159095adbb14c7dfaa6a77177be1b58600dc9 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 16 Sep 2019 16:15:20 -0400 Subject: [PATCH 3/7] adjust desugaring for async fn to correct drop order Old desugaring, given a user function body { $stmts; $expr } ``` { let $param_pattern0 = $raw_param0; ... let $param_patternN = $raw_paramN; $stmts; $expr } ``` New desugaring: ``` { let $param_pattern0 = $raw_param0; ... let $param_patternN = $raw_paramN; drop-temps { $stmts; $expr } } ``` The drop-temps is an internal bit of HIR that drops temporaries from the resulting expression, but it should be equivalent to `return { $stmts; $expr }`. --- src/librustc/hir/lowering.rs | 12 ++-------- src/librustc/hir/lowering/expr.rs | 2 +- src/librustc/hir/lowering/item.rs | 40 +++++++++++++++++++++++++++++-- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs index 58789a10609b7..492029eed0570 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -2682,12 +2682,8 @@ impl<'a> LoweringContext<'a> { bounds.iter().map(|bound| self.lower_param_bound(bound, itctx.reborrow())).collect() } - fn lower_block_with_stmts( - &mut self, - b: &Block, - targeted_by_break: bool, - mut stmts: Vec, - ) -> P { + fn lower_block(&mut self, b: &Block, targeted_by_break: bool) -> P { + let mut stmts = vec![]; let mut expr = None; for (index, stmt) in b.stmts.iter().enumerate() { @@ -2712,10 +2708,6 @@ impl<'a> LoweringContext<'a> { }) } - fn lower_block(&mut self, b: &Block, targeted_by_break: bool) -> P { - self.lower_block_with_stmts(b, targeted_by_break, vec![]) - } - fn lower_pat(&mut self, p: &Pat) -> P { let node = match p.node { PatKind::Wild => hir::PatKind::Wild, diff --git a/src/librustc/hir/lowering/expr.rs b/src/librustc/hir/lowering/expr.rs index a46cdabbb518f..87462e94f430d 100644 --- a/src/librustc/hir/lowering/expr.rs +++ b/src/librustc/hir/lowering/expr.rs @@ -1310,7 +1310,7 @@ impl LoweringContext<'_> { /// `{ let _t = $expr; _t }` but should provide better compile-time performance. /// /// The drop order can be important in e.g. `if expr { .. }`. - fn expr_drop_temps( + pub(super) fn expr_drop_temps( &mut self, span: Span, expr: P, diff --git a/src/librustc/hir/lowering/item.rs b/src/librustc/hir/lowering/item.rs index 5f82e42abb308..1e621f7fdaea6 100644 --- a/src/librustc/hir/lowering/item.rs +++ b/src/librustc/hir/lowering/item.rs @@ -1219,8 +1219,44 @@ impl LoweringContext<'_> { let async_expr = this.make_async_expr( CaptureBy::Value, closure_id, None, body.span, |this| { - let body = this.lower_block_with_stmts(body, false, statements); - this.expr_block(body, ThinVec::new()) + // Create a block from the user's function body: + let user_body = this.lower_block(body, false); + let user_body = this.expr_block(user_body, ThinVec::new()); + + // Transform into `drop-temps { }`, an expression: + let desugared_span = this.mark_span_with_reason( + DesugaringKind::Async, + user_body.span, + None, + ); + let user_body = this.expr_drop_temps( + desugared_span, + P(user_body), + ThinVec::new(), + ); + + // Create a block like + // + // ``` + // { + // let $param_pattern = $raw_param; + // ... + // drop-temps { } + // } + // ``` + // + // This construction is carefully calibrated to + // get the drop-order correct. In particular, the + // drop-temps ensures that any temporaries in the + // tail expression of `` are dropped + // *before* the parameters are dropped (see + // rust-lang/rust#64512). + let body = this.block_all( + desugared_span, + statements.into(), + Some(P(user_body)), + ); + this.expr_block(P(body), ThinVec::new()) }); (HirVec::from(parameters), this.expr(body.span, async_expr, ThinVec::new())) }) From 123f129f795a5f55a0094263f72d56249656145e Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 17 Sep 2019 04:50:40 -0400 Subject: [PATCH 4/7] apply nits from centril Co-Authored-By: Mazdak Farrokhzad --- ...rop-order-for-temporary-in-tail-return-expr.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/test/ui/async-await/drop-order/drop-order-for-temporary-in-tail-return-expr.rs b/src/test/ui/async-await/drop-order/drop-order-for-temporary-in-tail-return-expr.rs index e250506cdf0af..e40acff6dc117 100644 --- a/src/test/ui/async-await/drop-order/drop-order-for-temporary-in-tail-return-expr.rs +++ b/src/test/ui/async-await/drop-order/drop-order-for-temporary-in-tail-return-expr.rs @@ -4,9 +4,10 @@ #![allow(unused_variables)] -// Test that the drop order for parameters in a fn and async fn matches up. Also test that -// parameters (used or unused) are not dropped until the async fn completes execution. -// See also #54716. +// Test the drop order for parameters relative to local variables and +// temporaries created in the tail return expression of the function +// body. In particular, check that this drop order is the same between +// a `async fn` and an ordinary `fn`. See #64512. extern crate arc_wake; @@ -39,13 +40,13 @@ impl Drop for D { } } -/// Check drop order of temporary "temp" as compared to x, y, and z. +/// Check drop order of temporary "temp" as compared to `x`, `y`, and `z`. /// /// Expected order: -/// - z +/// - `z` /// - temp -/// - y -/// - x +/// - `y` +/// - `x` async fn foo_async(x: D, _y: D) { let l = x.1.clone(); let z = D("z", l.clone()); From 0a11e80fa83cd3ace4ca306531d84a6c8138d549 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 17 Sep 2019 04:39:21 -0400 Subject: [PATCH 5/7] introduce `lower_block_expr` convenience function, and use it --- src/librustc/hir/lowering.rs | 7 +++++++ src/librustc/hir/lowering/expr.rs | 11 +++-------- src/librustc/hir/lowering/item.rs | 8 ++------ 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs index 492029eed0570..48f7fc4446505 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -2708,6 +2708,13 @@ impl<'a> LoweringContext<'a> { }) } + /// Lowers a block directly to an expression, presuming that it + /// has no attributes and is not targeted by a `break`. + fn lower_block_expr(&mut self, b: &Block) -> hir::Expr { + let block = self.lower_block(b, false); + self.expr_block(block, ThinVec::new()) + } + fn lower_pat(&mut self, p: &Pat) -> P { let node = match p.node { PatKind::Wild => hir::PatKind::Wild, diff --git a/src/librustc/hir/lowering/expr.rs b/src/librustc/hir/lowering/expr.rs index 87462e94f430d..ef0bef9d569a4 100644 --- a/src/librustc/hir/lowering/expr.rs +++ b/src/librustc/hir/lowering/expr.rs @@ -90,10 +90,7 @@ impl LoweringContext<'_> { ), ExprKind::Async(capture_clause, closure_node_id, ref block) => { self.make_async_expr(capture_clause, closure_node_id, None, block.span, |this| { - this.with_new_scopes(|this| { - let block = this.lower_block(block, false); - this.expr_block(block, ThinVec::new()) - }) + this.with_new_scopes(|this| this.lower_block_expr(block)) }) } ExprKind::Await(ref expr) => self.lower_expr_await(e.span, expr), @@ -284,8 +281,7 @@ impl LoweringContext<'_> { let else_arm = self.arm(hir_vec![else_pat], P(else_expr)); // Handle then + scrutinee: - let then_blk = self.lower_block(then, false); - let then_expr = self.expr_block(then_blk, ThinVec::new()); + let then_expr = self.lower_block_expr(then); let (then_pat, scrutinee, desugar) = match cond.node { // ` => `: ExprKind::Let(ref pat, ref scrutinee) => { @@ -335,8 +331,7 @@ impl LoweringContext<'_> { }; // Handle then + scrutinee: - let then_blk = self.lower_block(body, false); - let then_expr = self.expr_block(then_blk, ThinVec::new()); + let then_expr = self.lower_block_expr(body); let (then_pat, scrutinee, desugar, source) = match cond.node { ExprKind::Let(ref pat, ref scrutinee) => { // to: diff --git a/src/librustc/hir/lowering/item.rs b/src/librustc/hir/lowering/item.rs index 1e621f7fdaea6..9ac3a6bdc87af 100644 --- a/src/librustc/hir/lowering/item.rs +++ b/src/librustc/hir/lowering/item.rs @@ -1071,10 +1071,7 @@ impl LoweringContext<'_> { } fn lower_fn_body_block(&mut self, decl: &FnDecl, body: &Block) -> hir::BodyId { - self.lower_fn_body(decl, |this| { - let body = this.lower_block(body, false); - this.expr_block(body, ThinVec::new()) - }) + self.lower_fn_body(decl, |this| this.lower_block_expr(body)) } pub(super) fn lower_const_body(&mut self, expr: &Expr) -> hir::BodyId { @@ -1220,8 +1217,7 @@ impl LoweringContext<'_> { CaptureBy::Value, closure_id, None, body.span, |this| { // Create a block from the user's function body: - let user_body = this.lower_block(body, false); - let user_body = this.expr_block(user_body, ThinVec::new()); + let user_body = this.lower_block_expr(body); // Transform into `drop-temps { }`, an expression: let desugared_span = this.mark_span_with_reason( From 716b2bcfb36111ea8afb49d289ddfba2b62a7b8e Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 17 Sep 2019 04:44:35 -0400 Subject: [PATCH 6/7] use drop-temps { .. } pseudo-notation DropTemps(...) looks like a function, this looks like wacko special stuff --- src/librustc/hir/lowering/expr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/hir/lowering/expr.rs b/src/librustc/hir/lowering/expr.rs index ef0bef9d569a4..990728fa0e680 100644 --- a/src/librustc/hir/lowering/expr.rs +++ b/src/librustc/hir/lowering/expr.rs @@ -351,7 +351,7 @@ impl LoweringContext<'_> { // // ``` // 'label: loop { - // match DropTemps($cond) { + // match drop-temps { $cond } { // true => $body, // _ => break, // } From 2d8b10f63c394c99f2268de3132086bc72ee5a2b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 17 Sep 2019 04:47:54 -0400 Subject: [PATCH 7/7] adjust larger comment to include the body --- src/librustc/hir/lowering/item.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/librustc/hir/lowering/item.rs b/src/librustc/hir/lowering/item.rs index 9ac3a6bdc87af..61be40a6b907f 100644 --- a/src/librustc/hir/lowering/item.rs +++ b/src/librustc/hir/lowering/item.rs @@ -1099,8 +1099,7 @@ impl LoweringContext<'_> { // from: // // async fn foo(: , : , : ) { - // async move { - // } + // // } // // into: @@ -1113,11 +1112,19 @@ impl LoweringContext<'_> { // let = __arg1; // let __arg0 = __arg0; // let = __arg0; + // drop-temps { } // see comments later in fn for details // } // } // // If `` is a simple ident, then it is lowered to a single // `let = ;` statement as an optimization. + // + // Note that the body is embedded in `drop-temps`; an + // equivalent desugaring would be `return { + // };`. The key point is that we wish to drop all the + // let-bound variables and temporaries created in the body + // (and its tail expression!) before we drop the + // parameters (c.f. rust-lang/rust#64512). for (index, parameter) in decl.inputs.iter().enumerate() { let parameter = this.lower_param(parameter); let span = parameter.pat.span; @@ -1231,7 +1238,7 @@ impl LoweringContext<'_> { ThinVec::new(), ); - // Create a block like + // As noted above, create the final block like // // ``` // { @@ -1240,13 +1247,6 @@ impl LoweringContext<'_> { // drop-temps { } // } // ``` - // - // This construction is carefully calibrated to - // get the drop-order correct. In particular, the - // drop-temps ensures that any temporaries in the - // tail expression of `` are dropped - // *before* the parameters are dropped (see - // rust-lang/rust#64512). let body = this.block_all( desugared_span, statements.into(),