Auto merge of #65249 - matthewjasper:revert-into-drop, r=nnethercote

Revert "Make `into` schedule drop for the destination"

This was a *very* large perf regression in some cases. I'll undo the revert once I have time to avoid the regression.

Author: bors

src/librustc_mir/build/block.rs

@@ -1,22 +1,18 @@
 use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
 use crate::build::ForGuard::OutsideGuard;
 use crate::build::matches::ArmHasGuard;
-use crate::build::scope::DropKind;
 use crate::hair::*;
-use rustc::middle::region;
 use rustc::mir::*;
 use rustc::hir;
 use syntax_pos::Span;
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
-    pub fn ast_block(
-        &mut self,
-        destination: &Place<'tcx>,
-        scope: Option<region::Scope>,
-        block: BasicBlock,
-        ast_block: &'tcx hir::Block,
-        source_info: SourceInfo,
-    ) -> BlockAnd<()> {
+    pub fn ast_block(&mut self,
+                     destination: &Place<'tcx>,
+                     block: BasicBlock,
+                     ast_block: &'tcx hir::Block,
+                     source_info: SourceInfo)
+                     -> BlockAnd<()> {
         let Block {
             region_scope,
             opt_destruction_scope,
@@ -25,61 +21,37 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             expr,
             targeted_by_break,
             safety_mode
-        } = self.hir.mirror(ast_block);
+        } =
+            self.hir.mirror(ast_block);
         self.in_opt_scope(opt_destruction_scope.map(|de|(de, source_info)), move |this| {
             this.in_scope((region_scope, source_info), LintLevel::Inherited, move |this| {
                 if targeted_by_break {
                     // This is a `break`-able block
                     let exit_block = this.cfg.start_new_block();
-                    if let Some(scope) = scope {
-                        // Breakable blocks assign to their destination on each
-                        // `break`, as well as when they exit normally. So we
-                        // can't schedule the drop in the last expression like
-                        // normal blocks do.
-                        let local = destination.as_local()
-                            .expect("cannot schedule drop of non-Local place");
-                        this.schedule_drop(span, scope, local, DropKind::Value);
-                    }
                     let block_exit = this.in_breakable_scope(
                         None, exit_block, destination.clone(), |this| {
-                            this.ast_block_stmts(
-                                destination,
-                                None,
-                                block,
-                                span,
-                                stmts,
-                                expr,
-                                safety_mode,
-                            )
+                            this.ast_block_stmts(destination, block, span, stmts, expr,
+                                                 safety_mode)
                         });
                     this.cfg.terminate(unpack!(block_exit), source_info,
                                        TerminatorKind::Goto { target: exit_block });
                     exit_block.unit()
                 } else {
-                    this.ast_block_stmts(
-                        destination,
-                        scope,
-                        block,
-                        span,
-                        stmts,
-                        expr,
-                        safety_mode,
-                    )
+                    this.ast_block_stmts(destination, block, span, stmts, expr,
+                                         safety_mode)
                 }
             })
         })
     }
 
-    fn ast_block_stmts(
-        &mut self,
-        destination: &Place<'tcx>,
-        scope: Option<region::Scope>,
-        mut block: BasicBlock,
-        span: Span,
-        stmts: Vec<StmtRef<'tcx>>,
-        expr: Option<ExprRef<'tcx>>,
-        safety_mode: BlockSafety,
-    ) -> BlockAnd<()> {
+    fn ast_block_stmts(&mut self,
+                       destination: &Place<'tcx>,
+                       mut block: BasicBlock,
+                       span: Span,
+                       stmts: Vec<StmtRef<'tcx>>,
+                       expr: Option<ExprRef<'tcx>>,
+                       safety_mode: BlockSafety)
+                       -> BlockAnd<()> {
         let this = self;
 
         // This convoluted structure is to avoid using recursion as we walk down a list
@@ -205,7 +177,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 this.block_context.currently_ignores_tail_results();
             this.block_context.push(BlockFrame::TailExpr { tail_result_is_ignored });
 
-            unpack!(block = this.into(destination, scope, block, expr));
+            unpack!(block = this.into(destination, block, expr));
             let popped = this.block_context.pop();
 
             assert!(popped.map_or(false, |bf|bf.is_tail_expr()));

src/librustc_mir/build/expr/as_rvalue.rs

@@ -136,14 +136,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 this.cfg
                     .push_assign(block, source_info, &Place::from(result), box_);
 
-                // Initialize the box contents. No scope is needed since the
-                // `Box` is already scheduled to be dropped.
+                // initialize the box contents:
                 unpack!(
                     block = this.into(
                         &Place::from(result).deref(),
-                        None,
-                        block,
-                        value
+                        block, value
                     )
                 );
                 block.and(Rvalue::Use(Operand::Move(Place::from(result))))

src/librustc_mir/build/expr/as_temp.rs

@@ -109,7 +109,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             }
         }
 
-        unpack!(block = this.into(temp_place, temp_lifetime, block, expr));
+        unpack!(block = this.into(temp_place, block, expr));
+
+        if let Some(temp_lifetime) = temp_lifetime {
+            this.schedule_drop(
+                expr_span,
+                temp_lifetime,
+                temp,
+                DropKind::Value,
+            );
+        }
 
         block.and(temp)
     }

src/librustc_mir/build/expr/into.rs

@@ -2,9 +2,7 @@
 
 use crate::build::expr::category::{Category, RvalueFunc};
 use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
-use crate::build::scope::DropKind;
 use crate::hair::*;
-use rustc::middle::region;
 use rustc::mir::*;
 use rustc::ty;
 
@@ -13,18 +11,15 @@ use rustc_target::spec::abi::Abi;
 impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// Compile `expr`, storing the result into `destination`, which
     /// is assumed to be uninitialized.
-    /// If a `drop_scope` is provided, `destination` is scheduled to be dropped
-    /// in `scope` once it has been initialized.
     pub fn into_expr(
         &mut self,
         destination: &Place<'tcx>,
-        scope: Option<region::Scope>,
         mut block: BasicBlock,
         expr: Expr<'tcx>,
     ) -> BlockAnd<()> {
         debug!(
-            "into_expr(destination={:?}, scope={:?}, block={:?}, expr={:?})",
-            destination, scope, block, expr
+            "into_expr(destination={:?}, block={:?}, expr={:?})",
+            destination, block, expr
         );
 
         // since we frequently have to reference `self` from within a
@@ -40,14 +35,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             _ => false,
         };
 
-        let schedule_drop = move |this: &mut Self| {
-            if let Some(drop_scope) = scope {
-                let local = destination.as_local()
-                    .expect("cannot schedule drop of non-Local place");
-                this.schedule_drop(expr_span, drop_scope, local, DropKind::Value);
-            }
-        };
-
         if !expr_is_block_or_scope {
             this.block_context.push(BlockFrame::SubExpr);
         }
@@ -60,14 +47,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             } => {
                 let region_scope = (region_scope, source_info);
                 this.in_scope(region_scope, lint_level, |this| {
-                    this.into(destination, scope, block, value)
+                    this.into(destination, block, value)
                 })
             }
             ExprKind::Block { body: ast_block } => {
-                this.ast_block(destination, scope, block, ast_block, source_info)
+                this.ast_block(destination, block, ast_block, source_info)
             }
             ExprKind::Match { scrutinee, arms } => {
-                this.match_expr(destination, scope, expr_span, block, scrutinee, arms)
+                this.match_expr(destination, expr_span, block, scrutinee, arms)
             }
             ExprKind::NeverToAny { source } => {
                 let source = this.hir.mirror(source);
@@ -80,7 +67,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
                 // This is an optimization. If the expression was a call then we already have an
                 // unreachable block. Don't bother to terminate it and create a new one.
-                schedule_drop(this);
                 if is_call {
                     block.unit()
                 } else {
@@ -178,9 +164,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     TerminatorKind::Goto { target: loop_block },
                 );
 
-                // Loops assign to their destination on each `break`. Since we
-                // can't easily unschedule drops, we schedule the drop now.
-                schedule_drop(this);
                 this.in_breakable_scope(
                     Some(loop_block),
                     exit_block,
@@ -202,8 +185,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         // introduce a unit temporary as the destination for the loop body.
                         let tmp = this.get_unit_temp();
                         // Execute the body, branching back to the test.
-                        // No scope is provided, since we've scheduled the drop above.
-                        let body_block_end = unpack!(this.into(&tmp, None, body_block, body));
+                        let body_block_end = unpack!(this.into(&tmp, body_block, body));
                         this.cfg.terminate(
                             body_block_end,
                             source_info,
@@ -252,14 +234,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         is_block_tail: None,
                     });
                     let ptr_temp = Place::from(ptr_temp);
-                    // No need for a scope, ptr_temp doesn't need drop
-                    let block = unpack!(this.into(&ptr_temp, None, block, ptr));
-                    // Maybe we should provide a scope here so that
-                    // `move_val_init` wouldn't leak on panic even with an
-                    // arbitrary `val` expression, but `schedule_drop`,
-                    // borrowck and drop elaboration all prevent us from
-                    // dropping `ptr_temp.deref()`.
-                    this.into(&ptr_temp.deref(), None, block, val)
+                    let block = unpack!(this.into(&ptr_temp, block, ptr));
+                    this.into(&ptr_temp.deref(), block, val)
                 } else {
                     let args: Vec<_> = args
                         .into_iter()
@@ -289,12 +265,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             from_hir_call,
                         },
                     );
-                    schedule_drop(this);
                     success.unit()
                 }
             }
             ExprKind::Use { source } => {
-                this.into(destination, scope, block, source)
+                this.into(destination, block, source)
             }
 
             // These cases don't actually need a destination
@@ -321,7 +296,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 let rvalue = Rvalue::Use(this.consume_by_copy_or_move(place));
                 this.cfg
                     .push_assign(block, source_info, destination, rvalue);
-                schedule_drop(this);
                 block.unit()
             }
             ExprKind::Index { .. } | ExprKind::Deref { .. } | ExprKind::Field { .. } => {
@@ -341,7 +315,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 let rvalue = Rvalue::Use(this.consume_by_copy_or_move(place));
                 this.cfg
                     .push_assign(block, source_info, destination, rvalue);
-                schedule_drop(this);
                 block.unit()
             }
 
@@ -373,7 +346,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
                 let rvalue = unpack!(block = this.as_local_rvalue(block, expr));
                 this.cfg.push_assign(block, source_info, destination, rvalue);
-                schedule_drop(this);
                 block.unit()
             }
         };

src/librustc_mir/build/into.rs

@@ -6,31 +6,26 @@
 
 use crate::build::{BlockAnd, Builder};
 use crate::hair::*;
-use rustc::middle::region;
 use rustc::mir::*;
 
 pub(in crate::build) trait EvalInto<'tcx> {
     fn eval_into(
         self,
         builder: &mut Builder<'_, 'tcx>,
         destination: &Place<'tcx>,
-        scope: Option<region::Scope>,
         block: BasicBlock,
     ) -> BlockAnd<()>;
 }
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
-    pub fn into<E>(
-        &mut self,
-        destination: &Place<'tcx>,
-        scope: Option<region::Scope>,
-        block: BasicBlock,
-        expr: E,
-    ) -> BlockAnd<()>
-    where
-        E: EvalInto<'tcx>,
+    pub fn into<E>(&mut self,
+                   destination: &Place<'tcx>,
+                   block: BasicBlock,
+                   expr: E)
+                   -> BlockAnd<()>
+        where E: EvalInto<'tcx>
     {
-        expr.eval_into(self, destination, scope, block)
+        expr.eval_into(self, destination, block)
     }
 }
 
@@ -39,11 +34,10 @@ impl<'tcx> EvalInto<'tcx> for ExprRef<'tcx> {
         self,
         builder: &mut Builder<'_, 'tcx>,
         destination: &Place<'tcx>,
-        scope: Option<region::Scope>,
         block: BasicBlock,
     ) -> BlockAnd<()> {
         let expr = builder.hir.mirror(self);
-        builder.into_expr(destination, scope, block, expr)
+        builder.into_expr(destination, block, expr)
     }
 }
 
@@ -52,9 +46,8 @@ impl<'tcx> EvalInto<'tcx> for Expr<'tcx> {
         self,
         builder: &mut Builder<'_, 'tcx>,
         destination: &Place<'tcx>,
-        scope: Option<region::Scope>,
         block: BasicBlock,
     ) -> BlockAnd<()> {
-        builder.into_expr(destination, scope, block, self)
+        builder.into_expr(destination, block, self)
     }
 }