Rollup merge of rust-lang#55781 - pnkfelix:issue-54382-more-precise-spans-for-temps-and-their-drops, r=davidtwco

More precise spans for temps and their drops

This PR has two main enhancements:

 1. when possible during code generation for a statement (like `expr();`), pass along the span of a statement, and then attribute the drops of temporaries from that statement to the statement's end-point (which will be the semicolon if it is a statement that is terminating by a semicolon).
 2. when evaluating a block expression into a MIR temp, use the span of the block's tail expression (rather than the span of whole block including its statements and curly-braces) for the span of the temp.

Each of these individually increases the precision of our diagnostic output; together they combine to make a much clearer picture about the control flow through the spans.

Fix rust-lang#54382

Author: pietroalbini

src/librustc_mir/build/block.rs

@@ -90,7 +90,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
         let source_info = this.source_info(span);
         for stmt in stmts {
-            let Stmt { kind, opt_destruction_scope } = this.hir.mirror(stmt);
+            let Stmt { kind, opt_destruction_scope, span: stmt_span } = this.hir.mirror(stmt);
             match kind {
                 StmtKind::Expr { scope, expr } => {
                     this.block_context.push(BlockFrame::Statement { ignores_expr_result: true });
@@ -99,7 +99,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                             let si = (scope, source_info);
                             this.in_scope(si, LintLevel::Inherited, block, |this| {
                                 let expr = this.hir.mirror(expr);
-                                this.stmt_expr(block, expr)
+                                this.stmt_expr(block, expr, Some(stmt_span))
                             })
                         }));
                 }
@@ -177,17 +177,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         let destination_ty = destination.ty(&this.local_decls, tcx).to_ty(tcx);
         if let Some(expr) = expr {
             let tail_result_is_ignored = destination_ty.is_unit() ||
-                match this.block_context.last() {
-                    // no context: conservatively assume result is read
-                    None => false,
-
-                    // sub-expression: block result feeds into some computation
-                    Some(BlockFrame::SubExpr) => false,
-
-                    // otherwise: use accumualated is_ignored state.
-                    Some(BlockFrame::TailExpr { tail_result_is_ignored: ignored }) |
-                    Some(BlockFrame::Statement { ignores_expr_result: ignored }) => *ignored,
-                };
+                this.block_context.currently_ignores_tail_results();
             this.block_context.push(BlockFrame::TailExpr { tail_result_is_ignored });
 
             unpack!(block = this.into(destination, block, expr));

src/librustc_mir/build/expr/as_rvalue.rs

@@ -351,7 +351,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 block.and(Rvalue::Aggregate(adt, fields))
             }
             ExprKind::Assign { .. } | ExprKind::AssignOp { .. } => {
-                block = unpack!(this.stmt_expr(block, expr));
+                block = unpack!(this.stmt_expr(block, expr, None));
                 block.and(this.unit_rvalue())
             }
             ExprKind::Yield { value } => {

src/librustc_mir/build/expr/as_temp.rs

@@ -10,7 +10,7 @@
 
 //! See docs in build/expr/mod.rs
 
-use build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
+use build::{BlockAnd, BlockAndExtension, Builder};
 use hair::*;
 use rustc::middle::region;
 use rustc::mir::*;
@@ -68,19 +68,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             debug!("creating temp {:?} with block_context: {:?}", local_decl, this.block_context);
             // Find out whether this temp is being created within the
             // tail expression of a block whose result is ignored.
-            for bf in this.block_context.iter().rev() {
-                match bf {
-                    BlockFrame::SubExpr => continue,
-                    BlockFrame::Statement { .. } => break,
-                    &BlockFrame::TailExpr { tail_result_is_ignored } => {
-                        local_decl = local_decl.block_tail(BlockTailInfo {
-                            tail_result_is_ignored
-                        });
-                        break;
-                    }
-                }
+            if let Some(tail_info) = this.block_context.currently_in_block_tail() {
+                local_decl = local_decl.block_tail(tail_info);
             }
-
             this.local_decls.push(local_decl)
         };
         if !expr_ty.is_never() {

src/librustc_mir/build/expr/into.rs

@@ -351,7 +351,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             | ExprKind::Break { .. }
             | ExprKind::InlineAsm { .. }
             | ExprKind::Return { .. } => {
-                unpack!(block = this.stmt_expr(block, expr));
+                unpack!(block = this.stmt_expr(block, expr, None));
                 this.cfg.push_assign_unit(block, source_info, destination);
                 block.unit()
             }

src/librustc_mir/build/expr/stmt.rs

@@ -14,7 +14,18 @@ use hair::*;
 use rustc::mir::*;
 
 impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
-    pub fn stmt_expr(&mut self, mut block: BasicBlock, expr: Expr<'tcx>) -> BlockAnd<()> {
+    /// 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<()>
+    {
         let this = self;
         let expr_span = expr.span;
         let source_info = this.source_info(expr.span);
@@ -29,7 +40,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             } => {
                 let value = this.hir.mirror(value);
                 this.in_scope((region_scope, source_info), lint_level, block, |this| {
-                    this.stmt_expr(block, value)
+                    this.stmt_expr(block, value, opt_stmt_span)
                 })
             }
             ExprKind::Assign { lhs, rhs } => {
@@ -190,9 +201,56 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             }
             _ => {
                 let expr_ty = expr.ty;
-                let temp = this.temp(expr.ty.clone(), expr_span);
+
+                // Issue #54382: When creating temp for the value of
+                // expression like:
+                //
+                // `{ side_effects(); { let l = stuff(); the_value } }`
+                //
+                // 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;
+                            }
+                        }
+                        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 {
+                                tail_result_is_ignored: true
+                            });
+                        }
+                    }
+                    let temp = this.local_decls.push(local_decl);
+                    let place = Place::Local(temp);
+                    debug!("created temp {:?} for expr {:?} in block_context: {:?}",
+                           temp, expr, this.block_context);
+                    place
+                };
                 unpack!(block = this.into(&temp, block, expr));
-                unpack!(block = this.build_drop(block, expr_span, temp, expr_ty));
+
+                // 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,
+                });
+
+                unpack!(block = this.build_drop(block, drop_point, temp, expr_ty));
                 block.unit()
             }
         }

src/librustc_mir/build/mod.rs

@@ -336,6 +336,9 @@ impl BlockFrame {
     }
  }
 
+#[derive(Debug)]
+struct BlockContext(Vec<BlockFrame>);
+
 struct Builder<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
     hir: Cx<'a, 'gcx, 'tcx>,
     cfg: CFG<'tcx>,
@@ -359,7 +362,7 @@ struct Builder<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
     /// start just throwing new entries onto that vector in order to
     /// distinguish the context of EXPR1 from the context of EXPR2 in
     /// `{ STMTS; EXPR1 } + EXPR2`
-    block_context: Vec<BlockFrame>,
+    block_context: BlockContext,
 
     /// The current unsafe block in scope, even if it is hidden by
     /// a PushUnsafeBlock
@@ -409,6 +412,55 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
     }
 }
 
+impl BlockContext {
+    fn new() -> Self { BlockContext(vec![]) }
+    fn push(&mut self, bf: BlockFrame) { self.0.push(bf); }
+    fn pop(&mut self) -> Option<BlockFrame> { self.0.pop() }
+
+    /// Traverses the frames on the BlockContext, searching for either
+    /// the first block-tail expression frame with no intervening
+    /// statement frame.
+    ///
+    /// Notably, this skips over `SubExpr` frames; this method is
+    /// meant to be used in the context of understanding the
+    /// relationship of a temp (created within some complicated
+    /// expression) with its containing expression, and whether the
+    /// value of that *containing expression* (not the temp!) is
+    /// ignored.
+    fn currently_in_block_tail(&self) -> Option<BlockTailInfo> {
+        for bf in self.0.iter().rev() {
+            match bf {
+                BlockFrame::SubExpr => continue,
+                BlockFrame::Statement { .. } => break,
+                &BlockFrame::TailExpr { tail_result_is_ignored } =>
+                    return Some(BlockTailInfo { tail_result_is_ignored })
+            }
+        }
+
+        return None;
+    }
+
+    /// Looks at the topmost frame on the BlockContext and reports
+    /// whether its one that would discard a block tail result.
+    ///
+    /// Unlike `currently_within_ignored_tail_expression`, this does
+    /// *not* skip over `SubExpr` frames: here, we want to know
+    /// whether the block result itself is discarded.
+    fn currently_ignores_tail_results(&self) -> bool {
+        match self.0.last() {
+            // no context: conservatively assume result is read
+            None => false,
+
+            // sub-expression: block result feeds into some computation
+            Some(BlockFrame::SubExpr) => false,
+
+            // otherwise: use accumulated is_ignored state.
+            Some(BlockFrame::TailExpr { tail_result_is_ignored: ignored }) |
+            Some(BlockFrame::Statement { ignores_expr_result: ignored }) => *ignored,
+        }
+    }
+}
+
 #[derive(Debug)]
 enum LocalsForNode {
     /// In the usual case, a node-id for an identifier maps to at most
@@ -764,7 +816,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             fn_span: span,
             arg_count,
             scopes: vec![],
-            block_context: vec![],
+            block_context: BlockContext::new(),
             source_scopes: IndexVec::new(),
             source_scope: OUTERMOST_SOURCE_SCOPE,
             source_scope_local_data: IndexVec::new(),

src/librustc_mir/hair/cx/block.rs

@@ -57,6 +57,7 @@ fn mirror_stmts<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>,
     for (index, stmt) in stmts.iter().enumerate() {
         let hir_id = cx.tcx.hir.node_to_hir_id(stmt.node.id());
         let opt_dxn_ext = cx.region_scope_tree.opt_destruction_scope(hir_id.local_id);
+        let stmt_span = StatementSpan(cx.tcx.hir.span(stmt.node.id()));
         match stmt.node {
             hir::StmtKind::Expr(ref expr, _) |
             hir::StmtKind::Semi(ref expr, _) => {
@@ -69,6 +70,7 @@ fn mirror_stmts<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>,
                         expr: expr.to_ref(),
                     },
                     opt_destruction_scope: opt_dxn_ext,
+                    span: stmt_span,
                 })))
             }
             hir::StmtKind::Decl(ref decl, _) => {
@@ -111,6 +113,7 @@ fn mirror_stmts<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>,
                                 lint_level: cx.lint_level_of(local.id),
                             },
                             opt_destruction_scope: opt_dxn_ext,
+                            span: stmt_span,
                         })));
                     }
                 }

src/librustc_mir/hair/mod.rs

@@ -72,10 +72,14 @@ pub enum StmtRef<'tcx> {
     Mirror(Box<Stmt<'tcx>>),
 }
 
+#[derive(Clone, Debug)]
+pub struct StatementSpan(pub Span);
+
 #[derive(Clone, Debug)]
 pub struct Stmt<'tcx> {
     pub kind: StmtKind<'tcx>,
     pub opt_destruction_scope: Option<region::Scope>,
+    pub span: StatementSpan,
 }
 
 #[derive(Clone, Debug)]

src/test/ui/nll/issue-54382-use-span-of-tail-of-block.nll.stderr

@@ -0,0 +1,20 @@
+error[E0597]: `_thing1` does not live long enough
+  --> $DIR/issue-54382-use-span-of-tail-of-block.rs:7:29
+   |
+LL |             D("other").next(&_thing1)
+   |             ----------------^^^^^^^^-
+   |             |               |
+   |             |               borrowed value does not live long enough
+   |             a temporary with access to the borrow is created here ...
+LL |         }
+LL |     }
+   |     - `_thing1` dropped here while still borrowed
+LL | 
+LL |     ;
+   |     - ... and the borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `D`
+   |
+   = note: The temporary is part of an expression at the end of a block. Consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped.
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0597`.

src/test/ui/nll/issue-54382-use-span-of-tail-of-block.rs

@@ -0,0 +1,28 @@
+fn main() {
+    {
+        let mut _thing1 = D(Box::new("thing1"));
+        {
+            let _thing2 = D("thing2");
+            side_effects();
+            D("other").next(&_thing1)
+        }
+    }
+
+    ;
+}
+
+#[derive(Debug)]
+struct D<T: std::fmt::Debug>(T);
+
+impl<T: std::fmt::Debug>  Drop for D<T> {
+    fn drop(&mut self) {
+        println!("dropping {:?})", self);
+    }
+}
+
+impl<T: std::fmt::Debug> D<T> {
+    fn next<U: std::fmt::Debug>(&self, _other: U) -> D<U> { D(_other) }
+    fn end(&self) { }
+}
+
+fn side_effects() { }

src/test/ui/nll/issue-54382-use-span-of-tail-of-block.stderr

@@ -0,0 +1,15 @@
+error[E0597]: `_thing1` does not live long enough
+  --> $DIR/issue-54382-use-span-of-tail-of-block.rs:7:30
+   |
+LL |             D("other").next(&_thing1)
+   |                              ^^^^^^^ borrowed value does not live long enough
+LL |         }
+LL |     }
+   |     - `_thing1` dropped here while still borrowed
+LL | 
+LL |     ;
+   |     - borrowed value needs to live until here
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0597`.