Rollup merge of rust-lang#63678 - Aaron1011:fix/hrtb-leak, r=nikomatsakis

Improve HRTB error span when -Zno-leak-check is used

As described in rust-lang#57374, NLL currently produces unhelpful higher-ranked
trait bound (HRTB) errors when '-Zno-leak-check' is enabled.

This PR tackles one half of this issue - making the error message point
at the proper span. The error message itself is still the very generic
"higher-ranked subtype error", but this can be improved in a follow-up
PR.

The root cause of the bad spans lies in how NLL attempts to compute the
'blamed' region, for which it will retrieve a span for.
Consider the following code, which (correctly) does not compile:

```rust
let my_val: u8 = 25;
let a: &u8 = &my_val;
let b = a;
let c = b;
let d: &'static u8 = c;
```

This will cause NLL to generate the following subtype constraints:

d :< c
c :< b
b <: a

Since normal Rust lifetimes are covariant, this results in the following
region constraints (I'm using 'd to denote the lifetime of 'd',
'c to denote the lifetime of 'c, etc.):

'c: 'd
'b: 'c
'a: 'b

From this, we can derive that 'a: 'd holds, which implies that 'a: 'static
must hold. However, this is not the case, since 'a refers to 'my_val',
which does not outlive the current function.

When NLL attempts to infer regions for this code, it will see that the
region 'a has grown 'too large' - it will be inferred to outlive
'static, despite the fact that is not declared as outliving 'static
We can find the region responsible, 'd, by starting at the *end* of
the 'constraint chain' we generated above. This works because for normal
(non-higher-ranked) lifetimes, we generally build up a 'chain' of
lifetime constraints *away* from the original variable/lifetime.
That is, our original lifetime 'a is required to outlive progressively
more regions. If it ends up living for too long, we can look at the
'end' of this chain to determine the 'most recent' usage that caused
the lifetime to grow too large.

However, this logic does not work correctly when higher-ranked trait
bounds (HRTBs) come into play. This is because HRTBs have
*contravariance* with respect to their bound regions. For example,
this code snippet compiles:

```rust
let a: for<'a> fn(&'a ()) = |_| {};
let b: fn(&'static ()) = a;
```

Here, we require that 'a' is a subtype of 'b'. Because of
contravariance, we end up with the region constraint 'static: 'a,
*not* 'a: 'static

This means that our 'constraint chains' grow in the opposite direction
of 'normal lifetime' constraint chains. As we introduce subtypes, our
lifetime ends up being outlived by other lifetimes, rather than
outliving other lifetimes. Therefore, starting at the end of the
'constraint chain' will cause us to 'blame' a lifetime close to the original
definition of a variable, instead of close to where the bad lifetime
constraint is introduced.

This PR improves how we select the region to blame for 'too large'
universal lifetimes, when bound lifetimes are involved. If the region
we're checking is a 'placeholder' region (e.g. the region 'a' in
for<'a>, or the implicit region in fn(&())), we start traversing the
constraint chain from the beginning, rather than the end.

There are two (maybe more) different ways we generate region constraints for NLL:
requirements generated from trait queries, and requirements generated
from MIR subtype constraints. While the former always use explicit
placeholder regions, the latter is more tricky. In order to implement
contravariance for HRTBs, TypeRelating replaces placeholder regions with
existential regions. This requires us to keep track of whether or not an
existential region was originally a placeholder region. When we look for
a region to blame, we check if our starting region is either a
placeholder region or is an existential region created from a
placeholder region. If so, we start iterating from the beginning of the
constraint chain, rather than the end.

Author: Centril

src/librustc/infer/mod.rs

@@ -418,15 +418,27 @@ pub enum NLLRegionVariableOrigin {
     /// from a `for<'a> T` binder). Meant to represent "any region".
     Placeholder(ty::PlaceholderRegion),
 
-    Existential,
+    Existential {
+        /// If this is true, then this variable was created to represent a lifetime
+        /// bound in a `for` binder. For example, it might have been created to
+        /// represent the lifetime `'a` in a type like `for<'a> fn(&'a u32)`.
+        /// Such variables are created when we are trying to figure out if there
+        /// is any valid instantiation of `'a` that could fit into some scenario.
+        ///
+        /// This is used to inform error reporting: in the case that we are trying to
+        /// determine whether there is any valid instantiation of a `'a` variable that meets
+        /// some constraint C, we want to blame the "source" of that `for` type,
+        /// rather than blaming the source of the constraint C.
+        from_forall: bool
+    },
 }
 
 impl NLLRegionVariableOrigin {
     pub fn is_universal(self) -> bool {
         match self {
             NLLRegionVariableOrigin::FreeRegion => true,
             NLLRegionVariableOrigin::Placeholder(..) => true,
-            NLLRegionVariableOrigin::Existential => false,
+            NLLRegionVariableOrigin::Existential{ .. } => false,
         }
     }
 

src/librustc/infer/nll_relate/mod.rs

@@ -93,7 +93,7 @@ pub trait TypeRelatingDelegate<'tcx> {
     /// we will invoke this method to instantiate `'a` with an
     /// inference variable (though `'b` would be instantiated first,
     /// as a placeholder).
-    fn next_existential_region_var(&mut self) -> ty::Region<'tcx>;
+    fn next_existential_region_var(&mut self, was_placeholder: bool) -> ty::Region<'tcx>;
 
     /// Creates a new region variable representing a
     /// higher-ranked region that is instantiated universally.
@@ -193,7 +193,7 @@ where
                     let placeholder = ty::PlaceholderRegion { universe, name: br };
                     delegate.next_placeholder_region(placeholder)
                 } else {
-                    delegate.next_existential_region_var()
+                    delegate.next_existential_region_var(true)
                 }
             }
         };

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

@@ -97,9 +97,11 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         &self,
         body: &Body<'tcx>,
         from_region: RegionVid,
+        from_region_origin: NLLRegionVariableOrigin,
         target_test: impl Fn(RegionVid) -> bool,
     ) -> (ConstraintCategory, bool, Span) {
-        debug!("best_blame_constraint(from_region={:?})", from_region);
+        debug!("best_blame_constraint(from_region={:?}, from_region_origin={:?})",
+            from_region, from_region_origin);
 
         // Find all paths
         let (path, target_region) =
@@ -152,19 +154,85 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         // we still want to screen for an "interesting" point to
         // highlight (e.g., a call site or something).
         let target_scc = self.constraint_sccs.scc(target_region);
-        let best_choice = (0..path.len()).rev().find(|&i| {
-            let constraint = path[i];
+        let mut range = 0..path.len();
+
+        // As noted above, when reporting an error, there is typically a chain of constraints
+        // leading from some "source" region which must outlive some "target" region.
+        // In most cases, we prefer to "blame" the constraints closer to the target --
+        // but there is one exception. When constraints arise from higher-ranked subtyping,
+        // we generally prefer to blame the source value,
+        // as the "target" in this case tends to be some type annotation that the user gave.
+        // Therefore, if we find that the region origin is some instantiation
+        // of a higher-ranked region, we start our search from the "source" point
+        // rather than the "target", and we also tweak a few other things.
+        //
+        // An example might be this bit of Rust code:
+        //
+        // ```rust
+        // let x: fn(&'static ()) = |_| {};
+        // let y: for<'a> fn(&'a ()) = x;
+        // ```
+        //
+        // In MIR, this will be converted into a combination of assignments and type ascriptions.
+        // In particular, the 'static is imposed through a type ascription:
+        //
+        // ```rust
+        // x = ...;
+        // AscribeUserType(x, fn(&'static ())
+        // y = x;
+        // ```
+        //
+        // We wind up ultimately with constraints like
+        //
+        // ```rust
+        // !a: 'temp1 // from the `y = x` statement
+        // 'temp1: 'temp2
+        // 'temp2: 'static // from the AscribeUserType
+        // ```
+        //
+        // and here we prefer to blame the source (the y = x statement).
+        let blame_source = match from_region_origin {
+            NLLRegionVariableOrigin::FreeRegion
+                | NLLRegionVariableOrigin::Existential { from_forall: false  } => {
+                    true
+            }
+            NLLRegionVariableOrigin::Placeholder(_)
+                | NLLRegionVariableOrigin::Existential { from_forall: true  } => {
+                    false
+            }
+        };
+
+        let find_region = |i: &usize| {
+            let constraint = path[*i];
 
             let constraint_sup_scc = self.constraint_sccs.scc(constraint.sup);
 
-            match categorized_path[i].0 {
-                ConstraintCategory::OpaqueType | ConstraintCategory::Boring |
-                ConstraintCategory::BoringNoLocation | ConstraintCategory::Internal => false,
-                ConstraintCategory::TypeAnnotation | ConstraintCategory::Return |
-                ConstraintCategory::Yield => true,
-                _ => constraint_sup_scc != target_scc,
+            if blame_source {
+                match categorized_path[*i].0 {
+                    ConstraintCategory::OpaqueType | ConstraintCategory::Boring |
+                    ConstraintCategory::BoringNoLocation | ConstraintCategory::Internal => false,
+                    ConstraintCategory::TypeAnnotation | ConstraintCategory::Return |
+                    ConstraintCategory::Yield => true,
+                    _ => constraint_sup_scc != target_scc,
+                }
+            } else {
+                match categorized_path[*i].0 {
+                    ConstraintCategory::OpaqueType | ConstraintCategory::Boring |
+                    ConstraintCategory::BoringNoLocation | ConstraintCategory::Internal => false,
+                    _ => true
+                }
             }
-        });
+        };
+
+        let best_choice = if blame_source {
+            range.rev().find(find_region)
+        } else {
+            range.find(find_region)
+        };
+
+        debug!("best_blame_constraint: best_choice={:?} blame_source={}",
+            best_choice, blame_source);
+
         if let Some(i) = best_choice {
             if let Some(next) = categorized_path.get(i + 1) {
                 if categorized_path[i].0 == ConstraintCategory::Return
@@ -300,12 +368,13 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         infcx: &'a InferCtxt<'a, 'tcx>,
         mir_def_id: DefId,
         fr: RegionVid,
+        fr_origin: NLLRegionVariableOrigin,
         outlived_fr: RegionVid,
         renctx: &mut RegionErrorNamingCtx,
     ) -> DiagnosticBuilder<'a> {
         debug!("report_error(fr={:?}, outlived_fr={:?})", fr, outlived_fr);
 
-        let (category, _, span) = self.best_blame_constraint(body, fr, |r| {
+        let (category, _, span) = self.best_blame_constraint(body, fr, fr_origin, |r| {
             self.provides_universal_region(r, fr, outlived_fr)
         });
 
@@ -712,6 +781,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         let (category, from_closure, span) = self.best_blame_constraint(
             body,
             borrow_region,
+            NLLRegionVariableOrigin::FreeRegion,
             |r| self.provides_universal_region(r, borrow_region, outlived_region)
         );
 
@@ -771,11 +841,13 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         &self,
         body: &Body<'tcx>,
         fr1: RegionVid,
+        fr1_origin: NLLRegionVariableOrigin,
         fr2: RegionVid,
     ) -> (ConstraintCategory, Span) {
         let (category, _, span) = self.best_blame_constraint(
             body,
             fr1,
+            fr1_origin,
             |r| self.provides_universal_region(r, fr1, fr2),
         );
         (category, span)
@@ -828,7 +900,9 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 universe1.cannot_name(placeholder.universe)
             }
 
-            NLLRegionVariableOrigin::FreeRegion | NLLRegionVariableOrigin::Existential => false,
+            NLLRegionVariableOrigin::FreeRegion | NLLRegionVariableOrigin::Existential { .. } => {
+                false
+            }
         }
     }
 }

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

@@ -406,7 +406,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                     }
                 }
 
-                NLLRegionVariableOrigin::Existential => {
+                NLLRegionVariableOrigin::Existential { .. } => {
                     // For existential, regions, nothing to do.
                 }
             }
@@ -1348,7 +1348,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                     self.check_bound_universal_region(infcx, body, mir_def_id, fr, placeholder);
                 }
 
-                NLLRegionVariableOrigin::Existential => {
+                NLLRegionVariableOrigin::Existential { .. } => {
                     // nothing to check here
                 }
             }
@@ -1461,7 +1461,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 debug!("check_universal_region: fr_minus={:?}", fr_minus);
 
                 let blame_span_category =
-                    self.find_outlives_blame_span(body, longer_fr, shorter_fr);
+                    self.find_outlives_blame_span(body, longer_fr,
+                                                  NLLRegionVariableOrigin::FreeRegion,shorter_fr);
 
                 // Grow `shorter_fr` until we find some non-local regions. (We
                 // always will.)  We'll call them `shorter_fr+` -- they're ever
@@ -1494,6 +1495,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             infcx,
             mir_def_id,
             longer_fr,
+            NLLRegionVariableOrigin::FreeRegion,
             shorter_fr,
             region_naming,
         );
@@ -1547,7 +1549,9 @@ 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(body, longer_fr, error_region);
+        let (_, span) = self.find_outlives_blame_span(
+            body, longer_fr, NLLRegionVariableOrigin::Placeholder(placeholder), error_region
+        );
 
         // Obviously, this error message is far from satisfactory.
         // At present, though, it only appears in unit tests --
@@ -1608,7 +1612,7 @@ impl<'tcx> RegionDefinition<'tcx> {
 
         let origin = match rv_origin {
             RegionVariableOrigin::NLL(origin) => origin,
-            _ => NLLRegionVariableOrigin::Existential,
+            _ => NLLRegionVariableOrigin::Existential { from_forall: false },
         };
 
         Self { origin, universe, external_name: None }

src/librustc_mir/borrow_check/nll/renumber.rs

@@ -35,7 +35,7 @@ where
     infcx
         .tcx
         .fold_regions(value, &mut false, |_region, _depth| {
-            let origin = NLLRegionVariableOrigin::Existential;
+            let origin = NLLRegionVariableOrigin::Existential { from_forall: false };
             infcx.next_nll_region_var(origin)
         })
 }

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

@@ -66,9 +66,9 @@ impl TypeRelatingDelegate<'tcx> for NllTypeRelatingDelegate<'_, '_, 'tcx> {
         self.infcx.create_next_universe()
     }
 
-    fn next_existential_region_var(&mut self) -> ty::Region<'tcx> {
+    fn next_existential_region_var(&mut self, from_forall: bool) -> ty::Region<'tcx> {
         if let Some(_) = &mut self.borrowck_context {
-            let origin = NLLRegionVariableOrigin::Existential;
+            let origin = NLLRegionVariableOrigin::Existential { from_forall };
             self.infcx.next_nll_region_var(origin)
         } else {
             self.infcx.tcx.lifetimes.re_erased
@@ -88,7 +88,9 @@ impl TypeRelatingDelegate<'tcx> for NllTypeRelatingDelegate<'_, '_, 'tcx> {
 
     fn generalize_existential(&mut self, universe: ty::UniverseIndex) -> ty::Region<'tcx> {
         self.infcx
-            .next_nll_region_var_in_universe(NLLRegionVariableOrigin::Existential, universe)
+            .next_nll_region_var_in_universe(NLLRegionVariableOrigin::Existential {
+                from_forall: false
+            }, universe)
     }
 
     fn push_outlives(&mut self, sup: ty::Region<'tcx>, sub: ty::Region<'tcx>) {

src/librustc_traits/chalk_context/unify.rs

@@ -65,7 +65,7 @@ impl TypeRelatingDelegate<'tcx> for &mut ChalkTypeRelatingDelegate<'_, 'tcx> {
         self.infcx.create_next_universe()
     }
 
-    fn next_existential_region_var(&mut self) -> ty::Region<'tcx> {
+    fn next_existential_region_var(&mut self, _was_placeholder: bool) -> ty::Region<'tcx> {
         self.infcx.next_region_var(RegionVariableOrigin::MiscVariable(DUMMY_SP))
     }
 

src/test/ui/hrtb/issue-30786.nll.stderr

@@ -1,14 +1,20 @@
 error: higher-ranked subtype error
-  --> $DIR/issue-30786.rs:113:18
+  --> $DIR/issue-30786.rs:108:15
+   |
+LL |     let map = source.map(|x: &_| x);
+   |               ^^^^^^^^^^^^^^^^^^^^^
+
+error: higher-ranked subtype error
+  --> $DIR/issue-30786.rs:114:18
    |
 LL |     let filter = map.filter(|x: &_| true);
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: higher-ranked subtype error
-  --> $DIR/issue-30786.rs:115:17
+  --> $DIR/issue-30786.rs:116:17
    |
 LL |     let count = filter.count(); // Assert that we still have a valid stream.
    |                 ^^^^^^^^^^^^^^
 
-error: aborting due to 2 previous errors
+error: aborting due to 3 previous errors
 

src/test/ui/hrtb/issue-30786.rs

@@ -106,12 +106,14 @@ impl<T> StreamExt for T where for<'a> &'a mut T: Stream { }
 fn main() {
     let source = Repeat(10);
     let map = source.map(|x: &_| x);
-    //[migrate]~^ ERROR implementation of `Stream` is not general enough
+    //[nll]~^ ERROR higher-ranked subtype error
+    //[migrate]~^^ ERROR implementation of `Stream` is not general enough
     //[migrate]~| NOTE  `Stream` would have to be implemented for the type `&'0 mut Map
     //[migrate]~| NOTE  but `Stream` is actually implemented for the type `&'1
     //[migrate]~| NOTE  implementation of `Stream` is not general enough
     let filter = map.filter(|x: &_| true);
     //[nll]~^ ERROR higher-ranked subtype error
     let count = filter.count(); // Assert that we still have a valid stream.
     //[nll]~^ ERROR higher-ranked subtype error
+
 }

src/test/ui/nll/relate_tys/fn-subtype.rs

@@ -0,0 +1,10 @@
+// Test that NLL produces correct spans for higher-ranked subtyping errors.
+//
+// compile-flags:-Zno-leak-check
+
+#![feature(nll)]
+
+fn main() {
+    let x: fn(&'static ()) = |_| {};
+    let y: for<'a> fn(&'a ()) = x; //~ ERROR higher-ranked subtype error
+}

src/test/ui/nll/relate_tys/fn-subtype.stderr

@@ -0,0 +1,8 @@
+error: higher-ranked subtype error
+  --> $DIR/fn-subtype.rs:9:33
+   |
+LL |     let y: for<'a> fn(&'a ()) = x;
+   |                                 ^
+
+error: aborting due to previous error
+

src/test/ui/nll/relate_tys/trait-hrtb.rs

@@ -0,0 +1,16 @@
+// Test that NLL generates proper error spans for trait HRTB errors
+//
+// compile-flags:-Zno-leak-check
+
+#![feature(nll)]
+
+trait Foo<'a> {}
+
+fn make_foo<'a>() -> Box<dyn Foo<'a>> {
+    panic!()
+}
+
+fn main() {
+    let x: Box<dyn Foo<'static>> = make_foo();
+    let y: Box<dyn for<'a> Foo<'a>> = x; //~ ERROR higher-ranked subtype error
+}

src/test/ui/nll/relate_tys/trait-hrtb.stderr

@@ -0,0 +1,8 @@
+error: higher-ranked subtype error
+  --> $DIR/trait-hrtb.rs:15:39
+   |
+LL |     let y: Box<dyn for<'a> Foo<'a>> = x;
+   |                                       ^
+
+error: aborting due to previous error
+