From 2c6094e5d043087e6f7539bd5d484ff3ba4dcf5d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Esteban=20K=C3=BCber?= <esteban@kuber.com.ar>
Date: Thu, 16 Apr 2020 12:43:40 -0700
Subject: [PATCH] Suggest `;` or assignment to drop borrows in tail exprs

Address the diagnostics part of #70844.

```
error[E0597]: `counter` does not live long enough
  --> $DIR/issue-54556-niconii.rs:22:20
   |
LL |     if let Ok(_) = counter.lock() { }
   |                    ^^^^^^^-------
   |                    |
   |                    borrowed value does not live long enough
   |                    a temporary with access to the borrow is created here ...
...
LL | }
   | -
   | |
   | `counter` dropped here while still borrowed
   | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `std::result::Result<MutexGuard<'_>, ()>`
   |
help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
   |
LL |     if let Ok(_) = counter.lock() { };
   |                                      ^
```
---
 src/librustc_middle/mir/mod.rs                |  3 ++
 .../diagnostics/explain_borrow.rs             | 39 ++++++++------
 src/librustc_mir_build/build/block.rs         |  6 ++-
 src/librustc_mir_build/build/expr/stmt.rs     |  2 +-
 src/librustc_mir_build/build/mod.rs           |  9 ++--
 ...sue-54382-use-span-of-tail-of-block.stderr |  5 +-
 src/test/ui/nll/issue-54556-niconii.stderr    |  5 +-
 .../ui/nll/issue-54556-stephaneyfx.stderr     |  7 ++-
 ...ssue-54556-temps-in-tail-diagnostic.stderr |  5 +-
 .../issue-54556-used-vs-unused-tails.stderr   | 51 +++++++++++++++----
 .../ui/span/destructor-restrictions.stderr    |  7 ++-
 ...338-locals-die-before-temps-of-body.stderr | 14 ++++-
 12 files changed, 117 insertions(+), 36 deletions(-)

diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs
index 70ae2fd8ac9fd..4a388def5c17a 100644
--- a/src/librustc_middle/mir/mod.rs
+++ b/src/librustc_middle/mir/mod.rs
@@ -625,6 +625,9 @@ pub struct BlockTailInfo {
     /// Examples include `{ ...; tail };` and `let _ = { ...; tail };`
     /// but not e.g., `let _x = { ...; tail };`
     pub tail_result_is_ignored: bool,
+
+    /// `Span` of the tail expression.
+    pub span: Span,
 }
 
 /// A MIR local.
diff --git a/src/librustc_mir/borrow_check/diagnostics/explain_borrow.rs b/src/librustc_mir/borrow_check/diagnostics/explain_borrow.rs
index 7645182ad1fb3..5253acbba7f1c 100644
--- a/src/librustc_mir/borrow_check/diagnostics/explain_borrow.rs
+++ b/src/librustc_mir/borrow_check/diagnostics/explain_borrow.rs
@@ -156,23 +156,32 @@ impl BorrowExplanation {
                         err.span_label(body.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."
+                            if info.tail_result_is_ignored {
+                                err.span_suggestion_verbose(
+                                    info.span.shrink_to_hi(),
+                                    "consider adding semicolon after the expression so its \
+                                     temporaries are dropped sooner, before the local variables \
+                                     declared by the block are dropped",
+                                    ";".to_string(),
+                                    Applicability::MaybeIncorrect,
+                                );
                             } 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."
+                                err.note(
+                                    "the temporary is part of an expression at the end of a \
+                                     block;\nconsider forcing this temporary to be dropped sooner, \
+                                     before the block's local variables are dropped",
+                                );
+                                err.multipart_suggestion(
+                                    "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",
+                                    vec![
+                                        (info.span.shrink_to_lo(), "let x = ".to_string()),
+                                        (info.span.shrink_to_hi(), "; x".to_string()),
+                                    ],
+                                    Applicability::MaybeIncorrect,
+                                );
                             };
-
-                            err.note(msg);
                         }
                     }
                 }
diff --git a/src/librustc_mir_build/build/block.rs b/src/librustc_mir_build/build/block.rs
index 3f94fb8289031..c2fe41169c117 100644
--- a/src/librustc_mir_build/build/block.rs
+++ b/src/librustc_mir_build/build/block.rs
@@ -173,7 +173,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         if let Some(expr) = expr {
             let tail_result_is_ignored =
                 destination_ty.is_unit() || this.block_context.currently_ignores_tail_results();
-            this.block_context.push(BlockFrame::TailExpr { tail_result_is_ignored });
+            let span = match expr {
+                ExprRef::Hair(expr) => expr.span,
+                ExprRef::Mirror(ref expr) => expr.span,
+            };
+            this.block_context.push(BlockFrame::TailExpr { tail_result_is_ignored, span });
 
             unpack!(block = this.into(destination, block, expr));
             let popped = this.block_context.pop();
diff --git a/src/librustc_mir_build/build/expr/stmt.rs b/src/librustc_mir_build/build/expr/stmt.rs
index 92e1e10518506..49d6ce39ddfa4 100644
--- a/src/librustc_mir_build/build/expr/stmt.rs
+++ b/src/librustc_mir_build/build/expr/stmt.rs
@@ -151,7 +151,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                                 }
                             }
                             this.block_context
-                                .push(BlockFrame::TailExpr { tail_result_is_ignored: true });
+                                .push(BlockFrame::TailExpr { tail_result_is_ignored: true, span: expr.span });
                             return Some(expr.span);
                         }
                     }
diff --git a/src/librustc_mir_build/build/mod.rs b/src/librustc_mir_build/build/mod.rs
index ee45ff5a495be..7d99a8d61d26e 100644
--- a/src/librustc_mir_build/build/mod.rs
+++ b/src/librustc_mir_build/build/mod.rs
@@ -245,6 +245,9 @@ enum BlockFrame {
         ///
         /// Example: `let _ = { STMT_1; EXPR };`
         tail_result_is_ignored: bool,
+
+        /// `Span` of the tail expression.
+        span: Span,
     },
 
     /// Generic mark meaning that the block occurred as a subexpression
@@ -372,8 +375,8 @@ impl BlockContext {
             match bf {
                 BlockFrame::SubExpr => continue,
                 BlockFrame::Statement { .. } => break,
-                &BlockFrame::TailExpr { tail_result_is_ignored } => {
-                    return Some(BlockTailInfo { tail_result_is_ignored });
+                &BlockFrame::TailExpr { tail_result_is_ignored, span } => {
+                    return Some(BlockTailInfo { tail_result_is_ignored, span });
                 }
             }
         }
@@ -397,7 +400,7 @@ impl BlockContext {
 
             // otherwise: use accumulated is_ignored state.
             Some(
-                BlockFrame::TailExpr { tail_result_is_ignored: ignored }
+                BlockFrame::TailExpr { tail_result_is_ignored: ignored, .. }
                 | BlockFrame::Statement { ignores_expr_result: ignored },
             ) => *ignored,
         }
diff --git a/src/test/ui/nll/issue-54382-use-span-of-tail-of-block.stderr b/src/test/ui/nll/issue-54382-use-span-of-tail-of-block.stderr
index 8412cbdc54b3b..2dca92e2be1f3 100644
--- a/src/test/ui/nll/issue-54382-use-span-of-tail-of-block.stderr
+++ b/src/test/ui/nll/issue-54382-use-span-of-tail-of-block.stderr
@@ -13,7 +13,10 @@ 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |             D("other").next(&_thing1);
+   |                                      ^
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/nll/issue-54556-niconii.stderr b/src/test/ui/nll/issue-54556-niconii.stderr
index 40cd04de5ecc1..b4791fd22b4ad 100644
--- a/src/test/ui/nll/issue-54556-niconii.stderr
+++ b/src/test/ui/nll/issue-54556-niconii.stderr
@@ -13,7 +13,10 @@ LL | }
    | `counter` dropped here while still borrowed
    | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `std::result::Result<MutexGuard<'_>, ()>`
    |
-   = 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |     if let Ok(_) = counter.lock() { };
+   |                                      ^
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/nll/issue-54556-stephaneyfx.stderr b/src/test/ui/nll/issue-54556-stephaneyfx.stderr
index 0bf76485eef2a..77065f0b8d213 100644
--- a/src/test/ui/nll/issue-54556-stephaneyfx.stderr
+++ b/src/test/ui/nll/issue-54556-stephaneyfx.stderr
@@ -12,7 +12,12 @@ LL | }
    | `stmt` dropped here while still borrowed
    | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `std::iter::Map<Rows<'_>, [closure@$DIR/issue-54556-stephaneyfx.rs:28:14: 28:23]>`
    |
-   = note: 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.
+   = note: 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
+help: 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
+   |
+LL |     let x = rows.map(|row| row).next(); x
+   |     ^^^^^^^                           ^^^
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/nll/issue-54556-temps-in-tail-diagnostic.stderr b/src/test/ui/nll/issue-54556-temps-in-tail-diagnostic.stderr
index 513dca7950af9..047fdbc9148ac 100644
--- a/src/test/ui/nll/issue-54556-temps-in-tail-diagnostic.stderr
+++ b/src/test/ui/nll/issue-54556-temps-in-tail-diagnostic.stderr
@@ -12,7 +12,10 @@ 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |         D(&_thing1).end();
+   |                          ^
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/nll/issue-54556-used-vs-unused-tails.stderr b/src/test/ui/nll/issue-54556-used-vs-unused-tails.stderr
index 52d0870b78f95..85920a8e7394c 100644
--- a/src/test/ui/nll/issue-54556-used-vs-unused-tails.stderr
+++ b/src/test/ui/nll/issue-54556-used-vs-unused-tails.stderr
@@ -8,7 +8,10 @@ LL |     {              let mut _t1 = D(Box::new("t1")); D(&_t1).end()    } ; //
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |     {              let mut _t1 = D(Box::new("t1")); D(&_t1).end();    } ; // suggest `;`
+   |                                                                  ^
 
 error[E0597]: `_t1` does not live long enough
   --> $DIR/issue-54556-used-vs-unused-tails.rs:13:55
@@ -20,7 +23,10 @@ LL |     {            { let mut _t1 = D(Box::new("t1")); D(&_t1).end() }  } ; //
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |     {            { let mut _t1 = D(Box::new("t1")); D(&_t1).end(); }  } ; // suggest `;`
+   |                                                                  ^
 
 error[E0597]: `_t1` does not live long enough
   --> $DIR/issue-54556-used-vs-unused-tails.rs:16:55
@@ -32,7 +38,10 @@ LL |     {            { let mut _t1 = D(Box::new("t1")); D(&_t1).end() }; }   //
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |     {            { let mut _t1 = D(Box::new("t1")); D(&_t1).end(); }; }   // suggest `;`
+   |                                                                  ^
 
 error[E0597]: `_t1` does not live long enough
   --> $DIR/issue-54556-used-vs-unused-tails.rs:19:55
@@ -44,7 +53,10 @@ LL |     let _ =      { let mut _t1 = D(Box::new("t1")); D(&_t1).end()    } ; //
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |     let _ =      { let mut _t1 = D(Box::new("t1")); D(&_t1).end();    } ; // suggest `;`
+   |                                                                  ^
 
 error[E0597]: `_t1` does not live long enough
   --> $DIR/issue-54556-used-vs-unused-tails.rs:22:55
@@ -56,7 +68,10 @@ LL |     let _u =     { let mut _t1 = D(Box::new("t1")); D(&_t1).unit()   } ; //
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL |     let _u =     { let mut _t1 = D(Box::new("t1")); D(&_t1).unit();   } ; // suggest `;`
+   |                                                                   ^
 
 error[E0597]: `_t1` does not live long enough
   --> $DIR/issue-54556-used-vs-unused-tails.rs:25:55
@@ -68,7 +83,12 @@ LL |     let _x =     { let mut _t1 = D(Box::new("t1")); D(&_t1).end()    } ; //
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = note: 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.
+   = note: 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
+help: 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
+   |
+LL |     let _x =     { let mut _t1 = D(Box::new("t1")); let x = D(&_t1).end(); x    } ; // `let x = ...; x`
+   |                                                     ^^^^^^^              ^^^
 
 error[E0597]: `_t1` does not live long enough
   --> $DIR/issue-54556-used-vs-unused-tails.rs:30:55
@@ -80,7 +100,12 @@ LL |     _y =         { let mut _t1 = D(Box::new("t1")); D(&_t1).end() } ; // `l
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = note: 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.
+   = note: 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
+help: 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
+   |
+LL |     _y =         { let mut _t1 = D(Box::new("t1")); let x = D(&_t1).end(); x } ; // `let x = ...; x`
+   |                                                     ^^^^^^^              ^^^
 
 error[E0597]: `_t1` does not live long enough
   --> $DIR/issue-54556-used-vs-unused-tails.rs:37:55
@@ -93,7 +118,10 @@ LL | fn f_local_ref() { let mut _t1 = D(Box::new("t1")); D(&_t1).unit()   }  //
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = 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.
+help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
+   |
+LL | fn f_local_ref() { let mut _t1 = D(Box::new("t1")); D(&_t1).unit();   }  // suggest `;`
+   |                                                                   ^
 
 error[E0597]: `_t1` does not live long enough
   --> $DIR/issue-54556-used-vs-unused-tails.rs:40:55
@@ -106,7 +134,12 @@ LL | fn f() -> String { let mut _t1 = D(Box::new("t1")); D(&_t1).end()   }   //
    |                                                     | borrowed value does not live long enough
    |                                                     a temporary with access to the borrow is created here ...
    |
-   = note: 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.
+   = note: 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
+help: 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
+   |
+LL | fn f() -> String { let mut _t1 = D(Box::new("t1")); let x = D(&_t1).end(); x   }   // `let x = ...; x`
+   |                                                     ^^^^^^^              ^^^
 
 error: aborting due to 9 previous errors
 
diff --git a/src/test/ui/span/destructor-restrictions.stderr b/src/test/ui/span/destructor-restrictions.stderr
index a3c6cfb6ae447..b5cd29f99c6b4 100644
--- a/src/test/ui/span/destructor-restrictions.stderr
+++ b/src/test/ui/span/destructor-restrictions.stderr
@@ -11,7 +11,12 @@ LL |     };
    |     |
    |     `*a` dropped here while still borrowed
    |
-   = note: 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.
+   = note: 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
+help: 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
+   |
+LL |         let x = *a.borrow() + 1; x
+   |         ^^^^^^^                ^^^
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/span/issue-23338-locals-die-before-temps-of-body.stderr b/src/test/ui/span/issue-23338-locals-die-before-temps-of-body.stderr
index 4696945814586..46702d364a8f3 100644
--- a/src/test/ui/span/issue-23338-locals-die-before-temps-of-body.stderr
+++ b/src/test/ui/span/issue-23338-locals-die-before-temps-of-body.stderr
@@ -12,7 +12,12 @@ LL | }
    | `y` dropped here while still borrowed
    | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `std::cell::Ref<'_, std::string::String>`
    |
-   = note: 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.
+   = note: 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
+help: 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
+   |
+LL |     let x = y.borrow().clone(); x
+   |     ^^^^^^^                   ^^^
 
 error[E0597]: `y` does not live long enough
   --> $DIR/issue-23338-locals-die-before-temps-of-body.rs:17:9
@@ -27,7 +32,12 @@ LL |     };
    |     |
    |     `y` dropped here while still borrowed
    |
-   = note: 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.
+   = note: 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
+help: 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
+   |
+LL |         let x = y.borrow().clone(); x
+   |         ^^^^^^^                   ^^^
 
 error: aborting due to 2 previous errors