Auto merge of #53088 - matthewjasper:closure-region-spans, r=nikomatsakis

[NLL] Use span of the closure args in free region errors

Also adds a note when one of the free regions is BrEnv.
In a future PR I'll change these messages to say "return requires", which should improve them a bit more.

r? @nikomatsakis

6 files changed, 112 insertions, 63 deletions

diff --git a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/region_name.rs b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/region_name.rs
index 79165276430..8c2a5f19038 100644
--- a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/region_name.rs
+++ b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/region_name.rs
@@ -10,6 +10,7 @@
 
 use borrow_check::nll::region_infer::RegionInferenceContext;
 use borrow_check::nll::ToRegionVid;
+use borrow_check::nll::universal_regions::DefiningTy;
 use rustc::hir;
 use rustc::hir::def_id::DefId;
 use rustc::infer::InferCtxt;
@@ -72,7 +73,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             })
             .or_else(|| {
                 self.give_name_if_anonymous_region_appears_in_output(
-                    infcx.tcx, mir, fr, counter, diag)
+                    infcx, mir, mir_def_id, fr, counter, diag)
             })
             .unwrap_or_else(|| span_bug!(mir.span, "can't make a name for free region {:?}", fr))
     }
@@ -107,13 +108,46 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 },
 
                 ty::BoundRegion::BrEnv => {
-                    let closure_span = tcx.hir.span_if_local(mir_def_id).unwrap();
-                    let region_name = self.synthesize_region_name(counter);
-                    diag.span_label(
-                        closure_span,
-                        format!("lifetime `{}` represents the closure body", region_name),
-                    );
-                    Some(region_name)
+                    let mir_node_id = tcx.hir.as_local_node_id(mir_def_id).expect("non-local mir");
+                    let def_ty = self.universal_regions.defining_ty;
+
+                    if let DefiningTy::Closure(def_id, substs) = def_ty {
+                        let args_span = if let hir::ExprKind::Closure(_, _, _, span, _)
+                            = tcx.hir.expect_expr(mir_node_id).node
+                        {
+                            span
+                        } else {
+                            bug!("Closure is not defined by a closure expr");
+                        };
+                        let region_name = self.synthesize_region_name(counter);
+                        diag.span_label(
+                            args_span,
+                            format!("lifetime `{}` represents this closure's body", region_name),
+                        );
+
+                        let closure_kind_ty = substs.closure_kind_ty(def_id, tcx);
+                        let note = match closure_kind_ty.to_opt_closure_kind() {
+                            Some(ty::ClosureKind::Fn) => {
+                                "closure implements `Fn`, so references to captured variables \
+                                 can't escape the closure"
+                            }
+                            Some(ty::ClosureKind::FnMut) => {
+                                "closure implements `FnMut`, so references to captured variables \
+                                 can't escape the closure"
+                            }
+                            Some(ty::ClosureKind::FnOnce) => {
+                                bug!("BrEnv in a `FnOnce` closure");
+                            }
+                            None => bug!("Closure kind not inferred in borrow check"),
+                        };
+
+                        diag.note(note);
+
+                        Some(region_name)
+                    } else {
+                        // Can't have BrEnv in functions, constants or generators.
+                        bug!("BrEnv outside of closure.");
+                    }
                 }
 
                 ty::BoundRegion::BrAnon(_) | ty::BoundRegion::BrFresh(_) => None,
@@ -543,28 +577,51 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     /// or be early bound (named, not in argument).
     fn give_name_if_anonymous_region_appears_in_output(
         &self,
-        tcx: TyCtxt<'_, '_, 'tcx>,
+        infcx: &InferCtxt<'_, '_, 'tcx>,
         mir: &Mir<'tcx>,
+        mir_def_id: DefId,
         fr: RegionVid,
         counter: &mut usize,
         diag: &mut DiagnosticBuilder<'_>,
     ) -> Option<InternedString> {
+        let tcx = infcx.tcx;
+
         let return_ty = self.universal_regions.unnormalized_output_ty;
         debug!(
             "give_name_if_anonymous_region_appears_in_output: return_ty = {:?}",
             return_ty
         );
-        if !tcx.any_free_region_meets(&return_ty, |r| r.to_region_vid() == fr) {
+        if !infcx.tcx.any_free_region_meets(&return_ty, |r| r.to_region_vid() == fr) {
             return None;
         }
 
-        let region_name = self.synthesize_region_name(counter);
+        let type_name = with_highlight_region(fr, *counter, || {
+            infcx.extract_type_name(&return_ty)
+        });
+
+                let mir_node_id = tcx.hir.as_local_node_id(mir_def_id).expect("non-local mir");
+
+        let (return_span, mir_description) = if let hir::ExprKind::Closure(_, _, _, span, gen_move)
+            = tcx.hir.expect_expr(mir_node_id).node
+        {
+            (
+                tcx.sess.codemap().end_point(span),
+                if gen_move.is_some() { " of generator" } else { " of closure" }
+            )
+        } else {
+            // unreachable?
+            (mir.span, "")
+        };
+
         diag.span_label(
-            mir.span,
-            format!("lifetime `{}` appears in return type", region_name),
+            return_span,
+            format!("return type{} is {}", mir_description, type_name),
         );
 
-        Some(region_name)
+        // This counter value will already have been used, so this function will increment it
+        // so the next value will be used next and return the region name that would have been
+        // used.
+        Some(self.synthesize_region_name(counter))
     }
 
     /// Create a synthetic region named `'1`, incrementing the
diff --git a/src/test/ui/error-codes/E0621-does-not-trigger-for-closures.nll.stderr b/src/test/ui/error-codes/E0621-does-not-trigger-for-closures.nll.stderr
index 0ac295c54bc..3f9104373d6 100644
--- a/src/test/ui/error-codes/E0621-does-not-trigger-for-closures.nll.stderr
+++ b/src/test/ui/error-codes/E0621-does-not-trigger-for-closures.nll.stderr
@@ -8,11 +8,10 @@ error: unsatisfied lifetime constraints
   --> $DIR/E0621-does-not-trigger-for-closures.rs:25:26
    |
 LL |     invoke(&x, |a, b| if a > b { a } else { b }); //~ ERROR E0495
-   |                ----------^^^^^-----------------
-   |                |   |     |
-   |                |   |     requires that `'1` must outlive `'2`
-   |                |   has type `&'1 i32`
-   |                lifetime `'2` appears in return type
+   |                    --    ^^^^^ requires that `'1` must outlive `'2`
+   |                    ||
+   |                    |return type of closure is &'2 i32
+   |                    has type `&'1 i32`
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/issue-40510-1.nll.stderr b/src/test/ui/issue-40510-1.nll.stderr
index 312ec6e742e..6c77bcb2757 100644
--- a/src/test/ui/issue-40510-1.nll.stderr
+++ b/src/test/ui/issue-40510-1.nll.stderr
@@ -1,17 +1,15 @@
 error: unsatisfied lifetime constraints
   --> $DIR/issue-40510-1.rs:18:9
    |
-LL |        || {
-   |   _____-
-   |  |_____|
-   | ||
-LL | ||         &mut x
-   | ||         ^^^^^^ return requires that `'1` must outlive `'2`
-LL | ||     };
-   | ||     -
-   | ||_____|
-   | |______lifetime `'1` represents the closure body
-   |        lifetime `'2` appears in return type
+LL |     || {
+   |     --
+   |     ||
+   |     |return type of closure is &'2 mut std::boxed::Box<()>
+   |     lifetime `'1` represents this closure's body
+LL |         &mut x
+   |         ^^^^^^ return requires that `'1` must outlive `'2`
+   |
+   = note: closure implements `FnMut`, so references to captured variables can't escape the closure
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/issue-40510-3.nll.stderr b/src/test/ui/issue-40510-3.nll.stderr
index eb44850e639..8aeef86c2e8 100644
--- a/src/test/ui/issue-40510-3.nll.stderr
+++ b/src/test/ui/issue-40510-3.nll.stderr
@@ -1,20 +1,17 @@
 error: unsatisfied lifetime constraints
   --> $DIR/issue-40510-3.rs:18:9
    |
-LL |         || {
-   |    _____-
-   |   |_____|
-   |  ||
-LL |  ||         || {
-   |  ||_________^
-LL | |||             x.push(())
-LL | |||         }
-   | |||_________^ requires that `'1` must outlive `'2`
-LL |  ||     };
-   |  ||     -
-   |  ||_____|
-   |  |______lifetime `'1` represents the closure body
-   |         lifetime `'2` appears in return type
+LL |       || {
+   |       --
+   |       ||
+   |       |return type of closure is [closure@$DIR/issue-40510-3.rs:18:9: 20:10 x:&'2 mut std::vec::Vec<()>]
+   |       lifetime `'1` represents this closure's body
+LL | /         || {
+LL | |             x.push(())
+LL | |         }
+   | |_________^ requires that `'1` must outlive `'2`
+   |
+   = note: closure implements `FnMut`, so references to captured variables can't escape the closure
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/issue-49824.nll.stderr b/src/test/ui/issue-49824.nll.stderr
index 59345754e9f..fb4bed76a71 100644
--- a/src/test/ui/issue-49824.nll.stderr
+++ b/src/test/ui/issue-49824.nll.stderr
@@ -1,20 +1,17 @@
 error: unsatisfied lifetime constraints
   --> $DIR/issue-49824.rs:22:9
    |
-LL |         || {
-   |    _____-
-   |   |_____|
-   |  ||
-LL |  ||         || {
-   |  ||_________^
-LL | |||             let _y = &mut x;
-LL | |||         }
-   | |||_________^ requires that `'1` must outlive `'2`
-LL |  ||     };
-   |  ||     -
-   |  ||_____|
-   |  |______lifetime `'1` represents the closure body
-   |         lifetime `'2` appears in return type
+LL |       || {
+   |       --
+   |       ||
+   |       |return type of closure is [closure@$DIR/issue-49824.rs:22:9: 24:10 x:&'2 mut i32]
+   |       lifetime `'1` represents this closure's body
+LL | /         || {
+LL | |             let _y = &mut x;
+LL | |         }
+   | |_________^ requires that `'1` must outlive `'2`
+   |
+   = note: closure implements `FnMut`, so references to captured variables can't escape the closure
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/nll/issue-48238.stderr b/src/test/ui/nll/issue-48238.stderr
index 7bdac345e90..84d0730025a 100644
--- a/src/test/ui/nll/issue-48238.stderr
+++ b/src/test/ui/nll/issue-48238.stderr
@@ -2,11 +2,12 @@ error: unsatisfied lifetime constraints
   --> $DIR/issue-48238.rs:21:13
    |
 LL |     move || use_val(&orig); //~ ERROR
-   |     --------^^^^^^^^^^^^^^
-   |     |       |
-   |     |       argument requires that `'1` must outlive `'2`
-   |     lifetime `'1` represents the closure body
-   |     lifetime `'2` appears in return type
+   |     ------- ^^^^^^^^^^^^^^ argument requires that `'1` must outlive `'2`
+   |     |     |
+   |     |     return type of closure is &'2 u8
+   |     lifetime `'1` represents this closure's body
+   |
+   = note: closure implements `Fn`, so references to captured variables can't escape the closure
 
 error: aborting due to previous error