Auto merge of #65385 - matthewjasper:drop-on-into-panic-otp, r=<try>

[WIP] Make `into` schedule drop for the destination again

#61430 triggered some degenerate behavior in llvm where it would inline functions far too aggressively.

This is marked as WIP because it doesn't really solve the problem in general. I've opened this PR more so that there's a place to discuss fixes.

<details>

<summary>Minimized example of the problem</summary>

Uncommenting the `#[inline]` will cause this to take a long time to compile on nightly, since llvm will inline all of the next calls into one enormous function that it spends a very long time handling.

```rust
pub trait Iterator {
    type Item;

    fn next(&mut self) -> Self::Item;
}

pub struct Empty;

impl Iterator for Empty {
    type Item = ();
    fn next(&mut self) {}
}

pub struct Chain<A, B=Empty> {
    a: A,
    b: B,
    state: ChainState,
}

#[allow(dead_code)]
enum ChainState {
    Both,
    Front,
    Back,
}

impl<A, B> Iterator for Chain<A, B> where
    A: Iterator,
    B: Iterator<Item = A::Item>
{
    type Item = A::Item;

    //#[inline]
    //^ uncomment me for degenerate llvm behvaiour with `-O`
    fn next(&mut self) -> A::Item {
        let ret;
        match self.state {
            ChainState::Both => {
                let _x = self.a.next();
                self.state = ChainState::Back;
                ret = self.b.next()
            },
            ChainState::Front => ret = self.a.next(),
            ChainState::Back => ret = self.b.next(),
        };
        ret
    }
}

type Chain2<T> = Chain<Chain<T>>;
type Chain4<T> = Chain2<Chain2<T>>;
type Chain8<T> = Chain4<Chain4<T>>;
type Chain16<T> = Chain8<Chain8<T>>;

pub fn call_next(x: &mut Chain16<Empty>) {
    x.next()
}
```

</details>

Closes #47949

Author: bors

src/libcore/iter/adapters/chain.rs

@@ -54,7 +54,6 @@ impl<A, B> Iterator for Chain<A, B> where
 {
     type Item = A::Item;
 
-    #[inline]
     fn next(&mut self) -> Option<A::Item> {
         match self.state {
             ChainState::Both => match self.a.next() {
@@ -117,7 +116,6 @@ impl<A, B> Iterator for Chain<A, B> where
         accum
     }
 
-    #[inline]
     fn nth(&mut self, mut n: usize) -> Option<A::Item> {
         match self.state {
             ChainState::Both | ChainState::Front => {
@@ -157,7 +155,6 @@ impl<A, B> Iterator for Chain<A, B> where
         }
     }
 
-    #[inline]
     fn last(self) -> Option<A::Item> {
         match self.state {
             ChainState::Both => {
@@ -198,7 +195,6 @@ impl<A, B> DoubleEndedIterator for Chain<A, B> where
     A: DoubleEndedIterator,
     B: DoubleEndedIterator<Item=A::Item>,
 {
-    #[inline]
     fn next_back(&mut self) -> Option<A::Item> {
         match self.state {
             ChainState::Both => match self.b.next_back() {
@@ -213,7 +209,6 @@ impl<A, B> DoubleEndedIterator for Chain<A, B> where
         }
     }
 
-    #[inline]
     fn nth_back(&mut self, mut n: usize) -> Option<A::Item> {
         match self.state {
             ChainState::Both | ChainState::Back => {

src/librustc_mir/build/block.rs

@@ -1,18 +1,22 @@
 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>,
-                     block: BasicBlock,
-                     ast_block: &'tcx hir::Block,
-                     source_info: SourceInfo)
-                     -> BlockAnd<()> {
+    pub fn ast_block(
+        &mut self,
+        destination: &Place<'tcx>,
+        scope: Option<region::Scope>,
+        block: BasicBlock,
+        ast_block: &'tcx hir::Block,
+        source_info: SourceInfo,
+    ) -> BlockAnd<()> {
         let Block {
             region_scope,
             opt_destruction_scope,
@@ -21,37 +25,61 @@ 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, block, span, stmts, expr,
-                                                 safety_mode)
+                            this.ast_block_stmts(
+                                destination,
+                                None,
+                                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, block, span, stmts, expr,
-                                         safety_mode)
+                    this.ast_block_stmts(
+                        destination,
+                        scope,
+                        block,
+                        span,
+                        stmts,
+                        expr,
+                        safety_mode,
+                    )
                 }
             })
         })
     }
 
-    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<()> {
+    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<()> {
         let this = self;
 
         // This convoluted structure is to avoid using recursion as we walk down a list
@@ -177,7 +205,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, block, expr));
+            unpack!(block = this.into(destination, scope, 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,11 +136,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 this.cfg
                     .push_assign(block, source_info, &Place::from(result), box_);
 
-                // initialize the box contents:
+                // Initialize the box contents. No scope is needed since the
+                // `Box` is already scheduled to be dropped.
                 unpack!(
                     block = this.into(
                         &this.hir.tcx().mk_place_deref(Place::from(result)),
-                        block, value
+                        None,
+                        block,
+                        value,
                     )
                 );
                 block.and(Rvalue::Use(Operand::Move(Place::from(result))))

src/librustc_mir/build/expr/as_temp.rs

@@ -109,16 +109,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             }
         }
 
-        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,
-            );
-        }
+        unpack!(block = this.into(temp_place, temp_lifetime, block, expr));
 
         block.and(temp)
     }

src/librustc_mir/build/expr/into.rs

@@ -2,7 +2,9 @@
 
 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;
 
@@ -11,15 +13,18 @@ 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={:?}, block={:?}, expr={:?})",
-            destination, block, expr
+            "into_expr(destination={:?}, scope={:?}, block={:?}, expr={:?})",
+            destination, scope, block, expr
         );
 
         // since we frequently have to reference `self` from within a
@@ -35,6 +40,14 @@ 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);
         }
@@ -47,14 +60,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, block, value)
+                    this.into(destination, scope, block, value)
                 })
             }
             ExprKind::Block { body: ast_block } => {
-                this.ast_block(destination, block, ast_block, source_info)
+                this.ast_block(destination, scope, block, ast_block, source_info)
             }
             ExprKind::Match { scrutinee, arms } => {
-                this.match_expr(destination, expr_span, block, scrutinee, arms)
+                this.match_expr(destination, scope, expr_span, block, scrutinee, arms)
             }
             ExprKind::NeverToAny { source } => {
                 let source = this.hir.mirror(source);
@@ -67,6 +80,7 @@ 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 {
@@ -164,6 +178,9 @@ 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,
@@ -185,7 +202,8 @@ 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.
-                        let body_block_end = unpack!(this.into(&tmp, body_block, body));
+                        // No scope is provided, since we've scheduled the drop above.
+                        let body_block_end = unpack!(this.into(&tmp, None, body_block, body));
                         this.cfg.terminate(
                             body_block_end,
                             source_info,
@@ -234,8 +252,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         is_block_tail: None,
                     });
                     let ptr_temp = Place::from(ptr_temp);
-                    let block = unpack!(this.into(&ptr_temp, block, ptr));
-                    this.into(&this.hir.tcx().mk_place_deref(ptr_temp), block, val)
+                    // 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(&this.hir.tcx().mk_place_deref(ptr_temp), None, block, val)
                 } else {
                     let args: Vec<_> = args
                         .into_iter()
@@ -265,11 +289,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             from_hir_call,
                         },
                     );
+                    schedule_drop(this);
                     success.unit()
                 }
             }
             ExprKind::Use { source } => {
-                this.into(destination, block, source)
+                this.into(destination, scope, block, source)
             }
 
             // These cases don't actually need a destination
@@ -296,6 +321,7 @@ 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 { .. } => {
@@ -315,6 +341,7 @@ 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()
             }
 
@@ -346,6 +373,7 @@ 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,26 +6,31 @@
 
 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>,
-                   block: BasicBlock,
-                   expr: E)
-                   -> BlockAnd<()>
-        where E: EvalInto<'tcx>
+    pub fn into<E>(
+        &mut self,
+        destination: &Place<'tcx>,
+        scope: Option<region::Scope>,
+        block: BasicBlock,
+        expr: E,
+    ) -> BlockAnd<()>
+    where
+        E: EvalInto<'tcx>,
     {
-        expr.eval_into(self, destination, block)
+        expr.eval_into(self, destination, scope, block)
     }
 }
 
@@ -34,10 +39,11 @@ 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, block, expr)
+        builder.into_expr(destination, scope, block, expr)
     }
 }
 
@@ -46,8 +52,9 @@ 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, block, self)
+        builder.into_expr(destination, scope, block, self)
     }
 }