Auto merge of #61872 - matthewjasper:refactor-mir-drop-gen, r=nikomatsakis

Clean up MIR drop generation

* Don't assign twice to the destination of a `while` loop containing a `break` expression
* Use `as_temp` to evaluate statement expression
* Avoid consecutive `StorageLive`s for the condition of a `while` loop
* Unify `return`, `break` and `continue` handling, and move it to `scopes.rs`
* Make some of the `scopes.rs` internals private
* Don't use `Place`s that are always `Local`s in MIR drop generation

Closes #42371
Closes #61579
Closes #61731
Closes #61834
Closes #61910
Closes #62115

Author: bors

src/librustc_mir/build/block.rs

@@ -78,7 +78,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
         let source_info = this.source_info(span);
         for stmt in stmts {
-            let Stmt { kind, opt_destruction_scope, span: stmt_span } = this.hir.mirror(stmt);
+            let Stmt { kind, opt_destruction_scope } = this.hir.mirror(stmt);
             match kind {
                 StmtKind::Expr { scope, expr } => {
                     this.block_context.push(BlockFrame::Statement { ignores_expr_result: true });
@@ -87,7 +87,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             let si = (scope, source_info);
                             this.in_scope(si, LintLevel::Inherited, |this| {
                                 let expr = this.hir.mirror(expr);
-                                this.stmt_expr(block, expr, Some(stmt_span))
+                                this.stmt_expr(block, expr, Some(scope))
                             })
                         }));
                 }

src/librustc_mir/build/expr/as_rvalue.rs

@@ -127,7 +127,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     this.schedule_drop_storage_and_value(
                         expr_span,
                         scope,
-                        &Place::from(result),
+                        result,
                         value.ty,
                     );
                 }
@@ -559,7 +559,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             this.schedule_drop_storage_and_value(
                 upvar_span,
                 temp_lifetime,
-                &Place::from(temp),
+                temp,
                 upvar_ty,
             );
         }

src/librustc_mir/build/expr/as_temp.rs

@@ -88,7 +88,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 this.schedule_drop(
                     expr_span,
                     temp_lifetime,
-                    temp_place,
+                    temp,
                     expr_ty,
                     DropKind::Storage,
                 );
@@ -101,7 +101,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             this.schedule_drop(
                 expr_span,
                 temp_lifetime,
-                temp_place,
+                temp,
                 expr_ty,
                 DropKind::Value,
             );

src/librustc_mir/build/expr/into.rs

@@ -179,20 +179,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         // conduct the test, if necessary
                         let body_block;
                         if let Some(cond_expr) = opt_cond_expr {
-                            let loop_block_end;
-                            let cond = unpack!(
-                                loop_block_end = this.as_local_operand(loop_block, cond_expr)
-                            );
-                            body_block = this.cfg.start_new_block();
-                            let term =
-                                TerminatorKind::if_(this.hir.tcx(), cond, body_block, exit_block);
-                            this.cfg.terminate(loop_block_end, source_info, term);
+                            let cond_expr = this.hir.mirror(cond_expr);
+                            let (true_block, false_block)
+                                = this.test_bool(loop_block, cond_expr, source_info);
+                            body_block = true_block;
 
                             // if the test is false, there's no `break` to assign `destination`, so
-                            // we have to do it; this overwrites any `break`-assigned value but it's
-                            // always `()` anyway
-                            this.cfg
-                                .push_assign_unit(exit_block, source_info, destination);
+                            // we have to do it
+                            this.cfg.push_assign_unit(false_block, source_info, destination);
+                            this.cfg.terminate(
+                                false_block,
+                                source_info,
+                                TerminatorKind::Goto { target: exit_block },
+                            );
                         } else {
                             body_block = this.cfg.start_new_block();
                             let diverge_cleanup = this.diverge_cleanup();

src/librustc_mir/build/expr/stmt.rs

@@ -1,21 +1,21 @@
-use crate::build::scope::BreakableScope;
+use crate::build::scope::BreakableTarget;
 use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
 use crate::hair::*;
+use rustc::middle::region;
 use rustc::mir::*;
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// Builds a block of MIR statements to evaluate the HAIR `expr`.
     /// If the original expression was an AST statement,
     /// (e.g., `some().code(&here());`) then `opt_stmt_span` is the
     /// span of that statement (including its semicolon, if any).
-    /// Diagnostics use this span (which may be larger than that of
-    /// `expr`) to identify when statement temporaries are dropped.
-    pub fn stmt_expr(&mut self,
-                     mut block: BasicBlock,
-                     expr: Expr<'tcx>,
-                     opt_stmt_span: Option<StatementSpan>)
-                     -> BlockAnd<()>
-    {
+    /// The scope is used if a statement temporary must be dropped.
+    pub fn stmt_expr(
+        &mut self,
+        mut block: BasicBlock,
+        expr: Expr<'tcx>,
+        statement_scope: Option<region::Scope>,
+    ) -> BlockAnd<()> {
         let this = self;
         let expr_span = expr.span;
         let source_info = this.source_info(expr.span);
@@ -30,7 +30,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             } => {
                 let value = this.hir.mirror(value);
                 this.in_scope((region_scope, source_info), lint_level, |this| {
-                    this.stmt_expr(block, value, opt_stmt_span)
+                    this.stmt_expr(block, value, statement_scope)
                 })
             }
             ExprKind::Assign { lhs, rhs } => {
@@ -98,70 +98,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 block.unit()
             }
             ExprKind::Continue { label } => {
-                let BreakableScope {
-                    continue_block,
-                    region_scope,
-                    ..
-                } = *this.find_breakable_scope(expr_span, label);
-                let continue_block = continue_block
-                    .expect("Attempted to continue in non-continuable breakable block");
-                this.exit_scope(
-                    expr_span,
-                    (region_scope, source_info),
-                    block,
-                    continue_block,
-                );
-                this.cfg.start_new_block().unit()
+                this.break_scope(block, None, BreakableTarget::Continue(label), source_info)
             }
             ExprKind::Break { label, value } => {
-                let (break_block, region_scope, destination) = {
-                    let BreakableScope {
-                        break_block,
-                        region_scope,
-                        ref break_destination,
-                        ..
-                    } = *this.find_breakable_scope(expr_span, label);
-                    (break_block, region_scope, break_destination.clone())
-                };
-                if let Some(value) = value {
-                    debug!("stmt_expr Break val block_context.push(SubExpr) : {:?}", expr2);
-                    this.block_context.push(BlockFrame::SubExpr);
-                    unpack!(block = this.into(&destination, block, value));
-                    this.block_context.pop();
-                } else {
-                    this.cfg.push_assign_unit(block, source_info, &destination)
-                }
-                this.exit_scope(expr_span, (region_scope, source_info), block, break_block);
-                this.cfg.start_new_block().unit()
+                this.break_scope(block, value, BreakableTarget::Break(label), source_info)
             }
             ExprKind::Return { value } => {
-                block = match value {
-                    Some(value) => {
-                        debug!("stmt_expr Return val block_context.push(SubExpr) : {:?}", expr2);
-                        this.block_context.push(BlockFrame::SubExpr);
-                        let result = unpack!(
-                            this.into(
-                                &Place::RETURN_PLACE,
-                                block,
-                                value
-                            )
-                        );
-                        this.block_context.pop();
-                        result
-                    }
-                    None => {
-                        this.cfg.push_assign_unit(
-                            block,
-                            source_info,
-                            &Place::RETURN_PLACE,
-                        );
-                        block
-                    }
-                };
-                let region_scope = this.region_scope_of_return_scope();
-                let return_block = this.return_block();
-                this.exit_scope(expr_span, (region_scope, source_info), block, return_block);
-                this.cfg.start_new_block().unit()
+                this.break_scope(block, value, BreakableTarget::Return, source_info)
             }
             ExprKind::InlineAsm {
                 asm,
@@ -199,7 +142,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 block.unit()
             }
             _ => {
-                let expr_ty = expr.ty;
+                assert!(
+                    statement_scope.is_some(),
+                    "Should not be calling `stmt_expr` on a general expression \
+                     without a statement scope",
+                );
 
                 // Issue #54382: When creating temp for the value of
                 // expression like:
@@ -208,48 +155,34 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 //
                 // it is usually better to focus on `the_value` rather
                 // than the entirety of block(s) surrounding it.
-                let mut temp_span = expr_span;
-                let mut temp_in_tail_of_block = false;
-                if let ExprKind::Block { body } = expr.kind {
-                    if let Some(tail_expr) = &body.expr {
-                        let mut expr = tail_expr;
-                        while let rustc::hir::ExprKind::Block(subblock, _label) = &expr.node {
-                            if let Some(subtail_expr) = &subblock.expr {
-                                expr = subtail_expr
-                            } else {
-                                break;
+                let adjusted_span = (|| {
+                    if let ExprKind::Block { body } = expr.kind {
+                        if let Some(tail_expr) = &body.expr {
+                            let mut expr = tail_expr;
+                            while let rustc::hir::ExprKind::Block(subblock, _label) = &expr.node {
+                                if let Some(subtail_expr) = &subblock.expr {
+                                    expr = subtail_expr
+                                } else {
+                                    break;
+                                }
                             }
-                        }
-                        temp_span = expr.span;
-                        temp_in_tail_of_block = true;
-                    }
-                }
-
-                let temp = {
-                    let mut local_decl = LocalDecl::new_temp(expr.ty.clone(), temp_span);
-                    if temp_in_tail_of_block {
-                        if this.block_context.currently_ignores_tail_results() {
-                            local_decl = local_decl.block_tail(BlockTailInfo {
+                            this.block_context.push(BlockFrame::TailExpr {
                                 tail_result_is_ignored: true
                             });
+                            return Some(expr.span);
                         }
                     }
-                    let temp = this.local_decls.push(local_decl);
-                    let place = Place::from(temp);
-                    debug!("created temp {:?} for expr {:?} in block_context: {:?}",
-                           temp, expr, this.block_context);
-                    place
-                };
-                unpack!(block = this.into(&temp, block, expr));
+                    None
+                })();
 
-                // Attribute drops of the statement's temps to the
-                // semicolon at the statement's end.
-                let drop_point = this.hir.tcx().sess.source_map().end_point(match opt_stmt_span {
-                    None => expr_span,
-                    Some(StatementSpan(span)) => span,
-                });
+                let temp = unpack!(block =
+                    this.as_temp(block, statement_scope, expr, Mutability::Not));
+
+                if let Some(span) = adjusted_span {
+                    this.local_decls[temp].source_info.span = span;
+                    this.block_context.pop();
+                }
 
-                unpack!(block = this.build_drop(block, drop_point, temp, expr_ty));
                 block.unit()
             }
         }