Auto merge of #53175 - matthewjasper:more-return-stuff, r=nikomatsakis

[NLL] Returns are interesting for free regions

Based on #53088 - creating now to get feedback.

Closes #51175

* Make assigning to the return type interesting.
* Use "returning this value" instead of "return" in error messages.
* Prefer one of the explanations that we have a name for to a generic interesting cause in some cases.
* Treat causes that involve the destination of a call like assignments.

Author: bors

src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs

@@ -14,7 +14,7 @@ use rustc::hir::def_id::DefId;
 use rustc::infer::error_reporting::nice_region_error::NiceRegionError;
 use rustc::infer::InferCtxt;
 use rustc::mir::{self, Location, Mir, Place, Rvalue, StatementKind, TerminatorKind};
-use rustc::ty::RegionVid;
+use rustc::ty::{TyCtxt, RegionVid};
 use rustc_data_structures::indexed_vec::IndexVec;
 use rustc_errors::Diagnostic;
 use std::collections::VecDeque;
@@ -42,7 +42,7 @@ impl fmt::Display for ConstraintCategory {
         // Must end with a space. Allows for empty names to be provided.
         match self {
             ConstraintCategory::Assignment => write!(f, "assignment "),
-            ConstraintCategory::Return => write!(f, "return "),
+            ConstraintCategory::Return => write!(f, "returning this value "),
             ConstraintCategory::Cast => write!(f, "cast "),
             ConstraintCategory::CallArgument => write!(f, "argument "),
             _ => write!(f, ""),
@@ -67,6 +67,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     fn best_blame_constraint(
         &self,
         mir: &Mir<'tcx>,
+        tcx: TyCtxt<'_, '_, 'tcx>,
         from_region: RegionVid,
         target_test: impl Fn(RegionVid) -> bool,
     ) -> (ConstraintCategory, Span, RegionVid) {
@@ -92,7 +93,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         // Classify each of the constraints along the path.
         let mut categorized_path: Vec<(ConstraintCategory, Span)> = path
             .iter()
-            .map(|&index| self.classify_constraint(index, mir))
+            .map(|&index| self.classify_constraint(index, mir, tcx))
             .collect();
         debug!(
             "best_blame_constraint: categorized_path={:#?}",
@@ -123,13 +124,15 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             let constraint = &self.constraints[path[i]];
 
             let constraint_sup_scc = self.constraint_sccs.scc(constraint.sup);
-            if constraint_sup_scc == target_scc {
-                return false;
-            }
 
             match categorized_path[i].0 {
                 ConstraintCategory::Boring => false,
-                _ => true,
+                ConstraintCategory::Other => {
+                    // other isn't interesting when the two lifetimes
+                    // are unified.
+                    constraint_sup_scc != self.constraint_sccs.scc(constraint.sub)
+                }
+                _ => constraint_sup_scc != target_scc,
             }
         });
         if let Some(i) = best_choice {
@@ -231,6 +234,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         &self,
         index: ConstraintIndex,
         mir: &Mir<'tcx>,
+        tcx: TyCtxt<'_, '_, 'tcx>,
     ) -> (ConstraintCategory, Span) {
         let constraint = self.constraints[index];
         debug!("classify_constraint: constraint={:?}", constraint);
@@ -254,7 +258,34 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 debug!("classify_constraint: terminator.kind={:?}", terminator.kind);
                 match terminator.kind {
                     TerminatorKind::DropAndReplace { .. } => ConstraintCategory::Assignment,
-                    TerminatorKind::Call { .. } => ConstraintCategory::CallArgument,
+                    // Classify calls differently depending on whether or not
+                    // the sub region appears in the destination type (so the
+                    // sup region is in the return type). If the return type
+                    // contains the sub-region, then this is either an
+                    // assignment or a return, depending on whether we are
+                    // writing to the RETURN_PLACE or not.
+                    //
+                    // The idea here is that the region is being propagated
+                    // from an input into the output place, so it's a kind of
+                    // assignment. Otherwise, if the sub-region only appears in
+                    // the argument types, then use the CallArgument
+                    // classification.
+                    TerminatorKind::Call { destination: Some((ref place, _)), .. } => {
+                        if tcx.any_free_region_meets(
+                            &place.ty(mir, tcx).to_ty(tcx),
+                            |region| self.to_region_vid(region) == constraint.sub,
+                        ) {
+                            match place {
+                                Place::Local(mir::RETURN_PLACE) => ConstraintCategory::Return,
+                                _ => ConstraintCategory::Assignment,
+                            }
+                        } else {
+                            ConstraintCategory::CallArgument
+                        }
+                    }
+                    TerminatorKind::Call { destination: None, .. } => {
+                        ConstraintCategory::CallArgument
+                    }
                     _ => ConstraintCategory::Other,
                 }
             } else {
@@ -304,7 +335,12 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     ) {
         debug!("report_error(fr={:?}, outlived_fr={:?})", fr, outlived_fr);
 
-        let (category, span, _) = self.best_blame_constraint(mir, fr, |r| r == outlived_fr);
+        let (category, span, _) = self.best_blame_constraint(
+            mir,
+            infcx.tcx,
+            fr,
+            |r| r == outlived_fr
+        );
 
         // Check if we can use one of the "nice region errors".
         if let (Some(f), Some(o)) = (self.to_error_region(fr), self.to_error_region(outlived_fr)) {
@@ -417,7 +453,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 diag.span_label(span, format!(
                     "{} was supposed to return data with lifetime `{}` but it is returning \
                     data with lifetime `{}`",
-                    mir_def_name, fr_name, outlived_fr_name,
+                    mir_def_name, outlived_fr_name, fr_name
                 ));
             },
             _ => {
@@ -446,10 +482,11 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     crate fn find_outlives_blame_span(
         &self,
         mir: &Mir<'tcx>,
+        tcx: TyCtxt<'_, '_, 'tcx>,
         fr1: RegionVid,
         fr2: RegionVid,
     ) -> Span {
-        let (_, span, _) = self.best_blame_constraint(mir, fr1, |r| r == fr2);
+        let (_, span, _) = self.best_blame_constraint(mir, tcx, fr1, |r| r == fr2);
         span
     }
 }

src/librustc_mir/borrow_check/nll/region_infer/error_reporting/region_name.rs

@@ -587,7 +587,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             infcx.extract_type_name(&return_ty)
         });
 
-                let mir_node_id = tcx.hir.as_local_node_id(mir_def_id).expect("non-local mir");
+        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

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

@@ -1040,7 +1040,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 longer_fr, shorter_fr,
             );
 
-            let blame_span = self.find_outlives_blame_span(mir, longer_fr, shorter_fr);
+            let blame_span = self.find_outlives_blame_span(mir, infcx.tcx, longer_fr, shorter_fr);
 
             if let Some(propagated_outlives_requirements) = propagated_outlives_requirements {
                 // Shrink `fr` until we find a non-local region (if we do).
@@ -1128,7 +1128,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         };
 
         // Find the code to blame for the fact that `longer_fr` outlives `error_fr`.
-        let span = self.find_outlives_blame_span(mir, longer_fr, error_region);
+        let span = self.find_outlives_blame_span(mir, infcx.tcx, longer_fr, error_region);
 
         // Obviously, this error message is far from satisfactory.
         // At present, though, it only appears in unit tests --

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

@@ -877,8 +877,9 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
                 // they are not caused by the user, but rather artifacts
                 // of lowering. Assignments to other sorts of places *are* interesting
                 // though.
-                let is_temp = if let Place::Local(l) = place {
-                    !mir.local_decls[*l].is_user_variable.is_some()
+                let is_temp = if let Place::Local(l) = *place {
+                    l != RETURN_PLACE &&
+                    !mir.local_decls[l].is_user_variable.is_some()
                 } else {
                     false
                 };
@@ -1119,7 +1120,19 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
         match *destination {
             Some((ref dest, _target_block)) => {
                 let dest_ty = dest.ty(mir, tcx).to_ty(tcx);
-                let locations = term_location.interesting();
+                let is_temp = if let Place::Local(l) = *dest {
+                    l != RETURN_PLACE &&
+                    !mir.local_decls[l].is_user_variable.is_some()
+                } else {
+                    false
+                };
+
+                let locations = if is_temp {
+                    term_location.boring()
+                } else {
+                    term_location.interesting()
+                };
+
                 if let Err(terr) = self.sub_types(sig.output(), dest_ty, locations) {
                     span_mirbug!(
                         self,

src/test/ui/associated-types/cache/project-fn-ret-contravariant.krisskross.nll.stderr

@@ -18,7 +18,7 @@ LL | fn transmute<'a,'b>(x: &'a u32, y: &'b u32) -> (&'a u32, &'b u32) {
    |              |
    |              lifetime `'a` defined here
 LL |    let a = bar(foo, y);
-   |            ^^^^^^^^^^^ argument requires that `'b` must outlive `'a`
+   |            ^^^^^^^^^^^ assignment requires that `'b` must outlive `'a`
 
 error: unsatisfied lifetime constraints
   --> $DIR/project-fn-ret-contravariant.rs:54:12
@@ -29,7 +29,7 @@ LL | fn transmute<'a,'b>(x: &'a u32, y: &'b u32) -> (&'a u32, &'b u32) {
    |              lifetime `'a` defined here
 LL |    let a = bar(foo, y);
 LL |    let b = bar(foo, x);
-   |            ^^^^^^^^^^^ argument requires that `'a` must outlive `'b`
+   |            ^^^^^^^^^^^ assignment requires that `'a` must outlive `'b`
 
 error: aborting due to 2 previous errors
 

src/test/ui/associated-types/cache/project-fn-ret-contravariant.transmute.nll.stderr

@@ -4,13 +4,13 @@ warning: not reporting region error due to nll
 LL |    bar(foo, x) //[transmute]~ ERROR E0495
    |        ^^^
 
-error: borrowed data escapes outside of function
+error: unsatisfied lifetime constraints
   --> $DIR/project-fn-ret-contravariant.rs:48:4
    |
 LL | fn baz<'a,'b>(x: &'a u32) -> &'static u32 {
-   |               - `x` is a reference that is only valid in the function body
+   |        -- lifetime `'a` defined here
 LL |    bar(foo, x) //[transmute]~ ERROR E0495
-   |    ^^^^^^^^^^^ `x` escapes the function body here
+   |    ^^^^^^^^^^^ returning this value requires that `'a` must outlive `'static`
 
 error: aborting due to previous error
 

src/test/ui/associated-types/cache/project-fn-ret-invariant.krisskross.nll.stderr

@@ -18,7 +18,7 @@ LL | fn transmute<'a,'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
    |              |
    |              lifetime `'a` defined here
 LL |    let a = bar(foo, y); //[krisskross]~ ERROR E0623
-   |            ^^^^^^^^^^^ argument requires that `'b` must outlive `'a`
+   |            ^^^^^^^^^^^ assignment requires that `'b` must outlive `'a`
 
 error: unsatisfied lifetime constraints
   --> $DIR/project-fn-ret-invariant.rs:64:12
@@ -29,7 +29,7 @@ LL | fn transmute<'a,'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
    |              lifetime `'a` defined here
 LL |    let a = bar(foo, y); //[krisskross]~ ERROR E0623
 LL |    let b = bar(foo, x);
-   |            ^^^^^^^^^^^ argument requires that `'a` must outlive `'b`
+   |            ^^^^^^^^^^^ assignment requires that `'a` must outlive `'b`
 
 error: aborting due to 2 previous errors
 

src/test/ui/associated-types/cache/project-fn-ret-invariant.transmute.nll.stderr

@@ -4,14 +4,14 @@ warning: not reporting region error due to nll
 LL |    bar(foo, x) //[transmute]~ ERROR E0495
    |        ^^^
 
-error: borrowed data escapes outside of function
+error: unsatisfied lifetime constraints
   --> $DIR/project-fn-ret-invariant.rs:58:4
    |
 LL | fn baz<'a,'b>(x: Type<'a>) -> Type<'static> {
-   |               - `x` is a reference that is only valid in the function body
+   |        -- lifetime `'a` defined here
 ...
 LL |    bar(foo, x) //[transmute]~ ERROR E0495
-   |    ^^^^^^^^^^^ `x` escapes the function body here
+   |    ^^^^^^^^^^^ returning this value requires that `'a` must outlive `'static`
 
 error: aborting due to previous error
 

src/test/ui/borrowck/borrowck-describe-lvalue.ast.nll.stderr

@@ -35,7 +35,7 @@ LL | |                    //[mir]~^ ERROR cannot borrow `x` as mutable more than
 LL | |                    *y = 1;
 LL | |                    drop(y);
 LL | |                 }
-   | |_________________^ requires that `'1` must outlive `'2`
+   | |_________________^ returning this value requires that `'1` must outlive `'2`
    |
    = note: closure implements `FnMut`, so references to captured variables can't escape the closure
 

src/test/ui/borrowck/borrowck-describe-lvalue.mir.stderr

@@ -35,7 +35,7 @@ LL | |                    //[mir]~^ ERROR cannot borrow `x` as mutable more than
 LL | |                    *y = 1;
 LL | |                    drop(y);
 LL | |                 }
-   | |_________________^ requires that `'1` must outlive `'2`
+   | |_________________^ returning this value requires that `'1` must outlive `'2`
    |
    = note: closure implements `FnMut`, so references to captured variables can't escape the closure