Improve diagnostics for borrow-check errors that result from drops of temporary r-values.

Changed `BorrowExplanation::UsedLaterWhenDropped` to handle both named
locals and also unnamed (aka temporaries).

If the dropped temporary does not implement `Drop`, then we print its
full type; but when the dropped temporary is itself an ADT `D` that
implements `Drop`, then diagnostic points the user directly at `D`.

Author: pnkfelix

src/librustc_mir/borrow_check/error_reporting.rs

@@ -770,7 +770,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         match explanation {
             BorrowExplanation::UsedLater(..)
             | BorrowExplanation::UsedLaterInLoop(..)
-            | BorrowExplanation::UsedLaterWhenDropped(..) => {
+            | BorrowExplanation::UsedLaterWhenDropped { .. } => {
                 // Only give this note and suggestion if it could be relevant.
                 err.note("consider using a `let` binding to create a longer lived value");
             }

src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

@@ -12,18 +12,22 @@ use borrow_check::borrow_set::BorrowData;
 use borrow_check::error_reporting::UseSpans;
 use borrow_check::nll::region_infer::Cause;
 use borrow_check::{Context, MirBorrowckCtxt, WriteKind};
-use rustc::ty::{Region, TyCtxt};
-use rustc::mir::{FakeReadCause, Location, Mir, Operand, Place, StatementKind, TerminatorKind};
+use rustc::ty::{self, Region, TyCtxt};
+use rustc::mir::{FakeReadCause, Local, Location, Mir, Operand};
+use rustc::mir::{Place, StatementKind, TerminatorKind};
 use rustc_errors::DiagnosticBuilder;
 use syntax_pos::Span;
-use syntax_pos::symbol::Symbol;
 
 mod find_use;
 
 pub(in borrow_check) enum BorrowExplanation<'tcx> {
     UsedLater(LaterUseKind, Span),
     UsedLaterInLoop(LaterUseKind, Span),
-    UsedLaterWhenDropped(Span, Symbol, bool),
+    UsedLaterWhenDropped {
+        drop_loc: Location,
+        dropped_local: Local,
+        should_note_order: bool,
+    },
     MustBeValidFor(Region<'tcx>),
     Unexplained,
 }
@@ -40,7 +44,7 @@ impl<'tcx> BorrowExplanation<'tcx> {
     pub(in borrow_check) fn add_explanation_to_diagnostic<'cx, 'gcx>(
         &self,
         tcx: TyCtxt<'cx, 'gcx, 'tcx>,
-        _mir: &Mir<'tcx>,
+        mir: &Mir<'tcx>,
         err: &mut DiagnosticBuilder<'_>,
         borrow_desc: &str,
     ) {
@@ -65,23 +69,76 @@ impl<'tcx> BorrowExplanation<'tcx> {
                 };
                 err.span_label(var_or_use_span, format!("{}{}", borrow_desc, message));
             },
-            BorrowExplanation::UsedLaterWhenDropped(span, local_name, should_note_order) => {
-                err.span_label(
-                    span,
-                    format!(
-                        "{}borrow later used here, when `{}` is dropped",
-                        borrow_desc,
-                        local_name,
-                    ),
-                );
+            BorrowExplanation::UsedLaterWhenDropped { drop_loc, dropped_local,
+                                                      should_note_order } =>
+            {
+                let local_decl = &mir.local_decls[dropped_local];
+                let (dtor_desc, type_desc) = match local_decl.ty.sty {
+                    // If type is an ADT that implements Drop, then
+                    // simplify output by reporting just the ADT name.
+                    ty::Adt(adt, _substs) if adt.has_dtor(tcx) && !adt.is_box() =>
+                        ("`Drop` code", format!("type `{}`", tcx.item_path_str(adt.did))),
+
+                    // Otherwise, just report the whole type (and use
+                    // the intentionally fuzzy phrase "destructor")
+                    ty::Closure(..) =>
+                        ("destructor", format!("closure")),
+                    ty::Generator(..) =>
+                        ("destructor", format!("generator")),
+
+                    _ => ("destructor", format!("type `{}`", local_decl.ty)),
+                };
+
+                match local_decl.name {
+                    Some(local_name) => {
+                        let message =
+                            format!("{B}borrow might be used here, when `{LOC}` is dropped \
+                                     and runs the {DTOR} for {TYPE}",
+                                    B=borrow_desc, LOC=local_name, TYPE=type_desc, DTOR=dtor_desc);
+                        err.span_label(mir.source_info(drop_loc).span, message);
+
+                        if should_note_order {
+                            err.note(
+                                "values in a scope are dropped \
+                                 in the opposite order they are defined",
+                            );
+                        }
+                    }
+                    None => {
+                        err.span_label(local_decl.source_info.span,
+                                       format!("a temporary with access to the {B}borrow \
+                                                is created here ...",
+                                               B=borrow_desc));
+                        let message =
+                            format!("... and the {B}borrow might be used here, \
+                                     when that temporary is dropped \
+                                     and runs the {DTOR} for {TYPE}",
+                                    B=borrow_desc, TYPE=type_desc, DTOR=dtor_desc);
+                        err.span_label(mir.source_info(drop_loc).span, message);
+
+                        if let Some(info) = &local_decl.is_block_tail {
+                            // FIXME: use span_suggestion instead, highlighting the
+                            // whole block tail expression.
+                            let msg = if info.tail_result_is_ignored {
+                                "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."
+                            } else {
+                                "The temporary is part of an expression at the end of a block. \
+                                 Consider forcing this temporary to be dropped sooner, before \
+                                 the block's local variables are dropped. \
+                                 For example, you could save the expression's value in a new \
+                                 local variable `x` and then make `x` be the expression \
+                                 at the end of the block."
+                            };
 
-                if should_note_order {
-                    err.note(
-                        "values in a scope are dropped \
-                         in the opposite order they are defined",
-                    );
+                            err.note(msg);
+                        }
+                    }
                 }
-            },
+            }
+
             BorrowExplanation::MustBeValidFor(region) => {
                 tcx.note_and_explain_free_region(
                     err,
@@ -116,8 +173,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         kind_place: Option<(WriteKind, &Place<'tcx>)>,
     ) -> BorrowExplanation<'tcx> {
         debug!(
-            "explain_why_borrow_contains_point(context={:?}, borrow={:?})",
-            context, borrow,
+            "explain_why_borrow_contains_point(context={:?}, borrow={:?}, kind_place={:?})",
+            context, borrow, kind_place
         );
 
         let regioncx = &self.nonlexical_regioncx;
@@ -154,32 +211,30 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 }
             }
 
-            Some(Cause::DropVar(local, location)) => match &mir.local_decls[local].name {
-                Some(local_name) => {
-                    let mut should_note_order = false;
-                    if let Some((WriteKind::StorageDeadOrDrop, place)) = kind_place {
-                        if let Place::Local(borrowed_local) = place {
-                            let dropped_local_scope = mir.local_decls[local].visibility_scope;
-                            let borrowed_local_scope =
-                                mir.local_decls[*borrowed_local].visibility_scope;
+             Some(Cause::DropVar(local, location)) => {
+                 let mut should_note_order = false;
+                 if mir.local_decls[local].name.is_some() {
+                     if let Some((WriteKind::StorageDeadOrDrop, place)) = kind_place {
+                         if let Place::Local(borrowed_local) = place {
+                             let dropped_local_scope = mir.local_decls[local].visibility_scope;
+                             let borrowed_local_scope =
+                                 mir.local_decls[*borrowed_local].visibility_scope;
 
-                            if mir.is_sub_scope(borrowed_local_scope, dropped_local_scope)
-                                && local != *borrowed_local
-                            {
-                                should_note_order = true;
-                            }
-                        }
-                    }
-
-                    BorrowExplanation::UsedLaterWhenDropped(
-                        mir.source_info(location).span,
-                        *local_name,
-                        should_note_order
-                    )
-                },
+                             if mir.is_sub_scope(borrowed_local_scope, dropped_local_scope)
+                                 && local != *borrowed_local
+                             {
+                                 should_note_order = true;
+                             }
+                         }
+                     }
+                 }
 
-                None => BorrowExplanation::Unexplained,
-            },
+                 BorrowExplanation::UsedLaterWhenDropped {
+                     drop_loc: location,
+                     dropped_local: local,
+                     should_note_order,
+                 }
+            }
 
             None => if let Some(region) = regioncx.to_error_region(region_sub) {
                 BorrowExplanation::MustBeValidFor(region)

src/librustc_mir/build/expr/stmt.rs

@@ -20,6 +20,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         let source_info = this.source_info(expr.span);
         // Handle a number of expressions that don't need a destination at all. This
         // avoids needing a mountain of temporary `()` variables.
+        let expr2 = expr.clone();
         match expr.kind {
             ExprKind::Scope {
                 region_scope,
@@ -40,6 +41,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 // is better for borrowck interaction with overloaded
                 // operators like x[j] = x[i].
 
+                debug!("stmt_expr Assign block_context.push(SubExpr) : {:?}", expr2);
                 this.block_context.push(BlockFrame::SubExpr);
 
                 // Generate better code for things that don't need to be
@@ -69,6 +71,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 let lhs = this.hir.mirror(lhs);
                 let lhs_ty = lhs.ty;
 
+                debug!("stmt_expr AssignOp block_context.push(SubExpr) : {:?}", expr2);
                 this.block_context.push(BlockFrame::SubExpr);
 
                 // As above, RTL.
@@ -120,6 +123,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                     (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();
@@ -132,6 +136,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             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::Local(RETURN_PLACE), block, value));
                         this.block_context.pop();
@@ -153,6 +158,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 outputs,
                 inputs,
             } => {
+                debug!("stmt_expr InlineAsm block_context.push(SubExpr) : {:?}", expr2);
                 this.block_context.push(BlockFrame::SubExpr);
                 let outputs = outputs
                     .into_iter()