From 1245467322dd8a93f375fc8965a6102d84a8303b Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 18 Aug 2019 01:20:24 -0400 Subject: [PATCH 1/4] Improve HRTB error span when -Zno-leak-check is used As described in #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. --- src/librustc/infer/mod.rs | 6 +- src/librustc/infer/nll_relate/mod.rs | 4 +- .../nll/region_infer/error_reporting/mod.rs | 63 +++++++++++++++---- .../borrow_check/nll/region_infer/mod.rs | 14 +++-- src/librustc_mir/borrow_check/nll/renumber.rs | 2 +- .../borrow_check/nll/type_check/relate_tys.rs | 8 ++- src/librustc_traits/chalk_context/unify.rs | 2 +- src/test/ui/hrtb/issue-30786.rs | 1 + src/test/ui/nll/relate_tys/fn-subtype.rs | 10 +++ src/test/ui/nll/relate_tys/fn-subtype.stderr | 8 +++ src/test/ui/nll/relate_tys/trait-hrtb.rs | 16 +++++ src/test/ui/nll/relate_tys/trait-hrtb.stderr | 8 +++ 12 files changed, 116 insertions(+), 26 deletions(-) create mode 100644 src/test/ui/nll/relate_tys/fn-subtype.rs create mode 100644 src/test/ui/nll/relate_tys/fn-subtype.stderr create mode 100644 src/test/ui/nll/relate_tys/trait-hrtb.rs create mode 100644 src/test/ui/nll/relate_tys/trait-hrtb.stderr diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs index b06b63455ba4b..99297d7222b07 100644 --- a/src/librustc/infer/mod.rs +++ b/src/librustc/infer/mod.rs @@ -418,7 +418,9 @@ pub enum NLLRegionVariableOrigin { /// from a `for<'a> T` binder). Meant to represent "any region". Placeholder(ty::PlaceholderRegion), - Existential, + Existential { + was_placeholder: bool + }, } impl NLLRegionVariableOrigin { @@ -426,7 +428,7 @@ impl NLLRegionVariableOrigin { match self { NLLRegionVariableOrigin::FreeRegion => true, NLLRegionVariableOrigin::Placeholder(..) => true, - NLLRegionVariableOrigin::Existential => false, + NLLRegionVariableOrigin::Existential{ .. } => false, } } diff --git a/src/librustc/infer/nll_relate/mod.rs b/src/librustc/infer/nll_relate/mod.rs index 47e5c2b59ef36..4649f3f9567e7 100644 --- a/src/librustc/infer/nll_relate/mod.rs +++ b/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) } } }; diff --git a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs index 6d3f2e566382f..826be88e36572 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs @@ -98,9 +98,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) = @@ -153,19 +155,50 @@ 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(); + + let should_reverse = match from_region_origin { + NLLRegionVariableOrigin::FreeRegion + | NLLRegionVariableOrigin::Existential { was_placeholder: false } => { + true + } + NLLRegionVariableOrigin::Placeholder(_) + | NLLRegionVariableOrigin::Existential { was_placeholder: 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 should_reverse { + 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 should_reverse { + range.rev().find(find_region) + } else { + range.find(find_region) + }; + + debug!("best_blame_constraint: best_choice={:?} should_reverse={}", + best_choice, should_reverse); + if let Some(i) = best_choice { if let Some(next) = categorized_path.get(i + 1) { if categorized_path[i].0 == ConstraintCategory::Return @@ -297,12 +330,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) }); @@ -709,6 +743,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) ); @@ -768,11 +803,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) @@ -825,7 +862,9 @@ impl<'tcx> RegionInferenceContext<'tcx> { universe1.cannot_name(placeholder.universe) } - NLLRegionVariableOrigin::FreeRegion | NLLRegionVariableOrigin::Existential => false, + NLLRegionVariableOrigin::FreeRegion | NLLRegionVariableOrigin::Existential { .. } => { + false + } } } } diff --git a/src/librustc_mir/borrow_check/nll/region_infer/mod.rs b/src/librustc_mir/borrow_check/nll/region_infer/mod.rs index 38df803539712..164f7b0627c2d 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/mod.rs +++ b/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 { was_placeholder: false }, }; Self { origin, universe, external_name: None } diff --git a/src/librustc_mir/borrow_check/nll/renumber.rs b/src/librustc_mir/borrow_check/nll/renumber.rs index cd13803875b5d..6e4db36bdce03 100644 --- a/src/librustc_mir/borrow_check/nll/renumber.rs +++ b/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 { was_placeholder: false }; infcx.next_nll_region_var(origin) }) } diff --git a/src/librustc_mir/borrow_check/nll/type_check/relate_tys.rs b/src/librustc_mir/borrow_check/nll/type_check/relate_tys.rs index 2549aa4fbff93..919fcdbd39ba0 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/relate_tys.rs +++ b/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, was_placeholder: bool) -> ty::Region<'tcx> { if let Some(_) = &mut self.borrowck_context { - let origin = NLLRegionVariableOrigin::Existential; + let origin = NLLRegionVariableOrigin::Existential { was_placeholder }; 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 { + was_placeholder: false + }, universe) } fn push_outlives(&mut self, sup: ty::Region<'tcx>, sub: ty::Region<'tcx>) { diff --git a/src/librustc_traits/chalk_context/unify.rs b/src/librustc_traits/chalk_context/unify.rs index 1f9090324414b..5959c2ea5ca14 100644 --- a/src/librustc_traits/chalk_context/unify.rs +++ b/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)) } diff --git a/src/test/ui/hrtb/issue-30786.rs b/src/test/ui/hrtb/issue-30786.rs index c42297ca68346..1bbef995fe885 100644 --- a/src/test/ui/hrtb/issue-30786.rs +++ b/src/test/ui/hrtb/issue-30786.rs @@ -114,4 +114,5 @@ fn main() { //[nll]~^ ERROR higher-ranked subtype error let count = filter.count(); // Assert that we still have a valid stream. //[nll]~^ ERROR higher-ranked subtype error + } diff --git a/src/test/ui/nll/relate_tys/fn-subtype.rs b/src/test/ui/nll/relate_tys/fn-subtype.rs new file mode 100644 index 0000000000000..ac00627ad00eb --- /dev/null +++ b/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 +} diff --git a/src/test/ui/nll/relate_tys/fn-subtype.stderr b/src/test/ui/nll/relate_tys/fn-subtype.stderr new file mode 100644 index 0000000000000..b089b5aaa2535 --- /dev/null +++ b/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 + diff --git a/src/test/ui/nll/relate_tys/trait-hrtb.rs b/src/test/ui/nll/relate_tys/trait-hrtb.rs new file mode 100644 index 0000000000000..80f31ca6b4782 --- /dev/null +++ b/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> { + panic!() +} + +fn main() { + let x: Box> = make_foo(); + let y: Box Foo<'a>> = x; //~ ERROR higher-ranked subtype error +} diff --git a/src/test/ui/nll/relate_tys/trait-hrtb.stderr b/src/test/ui/nll/relate_tys/trait-hrtb.stderr new file mode 100644 index 0000000000000..4df2f352522a3 --- /dev/null +++ b/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 Foo<'a>> = x; + | ^ + +error: aborting due to previous error + From 1caa6dc5461292611fcad75c8114c9fd35d7e923 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 1 Oct 2019 21:22:46 -0400 Subject: [PATCH 2/4] Fixup tests --- src/test/ui/hrtb/issue-30786.nll.stderr | 12 +++++++++--- src/test/ui/hrtb/issue-30786.rs | 3 ++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/test/ui/hrtb/issue-30786.nll.stderr b/src/test/ui/hrtb/issue-30786.nll.stderr index 1cfd93e59d935..cd1272da2a66e 100644 --- a/src/test/ui/hrtb/issue-30786.nll.stderr +++ b/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 diff --git a/src/test/ui/hrtb/issue-30786.rs b/src/test/ui/hrtb/issue-30786.rs index 1bbef995fe885..34d6b19f602f7 100644 --- a/src/test/ui/hrtb/issue-30786.rs +++ b/src/test/ui/hrtb/issue-30786.rs @@ -106,7 +106,8 @@ impl 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 From 9c5a5c471aa9229b82bafa8ec18e55722d9ba8c3 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 1 Oct 2019 21:39:19 -0400 Subject: [PATCH 3/4] Rename to `was_placeholder` to `from_forall` --- src/librustc/infer/mod.rs | 12 +++++++++++- .../nll/region_infer/error_reporting/mod.rs | 4 ++-- .../borrow_check/nll/region_infer/mod.rs | 2 +- src/librustc_mir/borrow_check/nll/renumber.rs | 2 +- .../borrow_check/nll/type_check/relate_tys.rs | 6 +++--- 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs index 99297d7222b07..35db82406ab46 100644 --- a/src/librustc/infer/mod.rs +++ b/src/librustc/infer/mod.rs @@ -419,7 +419,17 @@ pub enum NLLRegionVariableOrigin { Placeholder(ty::PlaceholderRegion), Existential { - was_placeholder: bool + /// 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 }, } diff --git a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs index 826be88e36572..fa02e2dd234ef 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs @@ -159,11 +159,11 @@ impl<'tcx> RegionInferenceContext<'tcx> { let should_reverse = match from_region_origin { NLLRegionVariableOrigin::FreeRegion - | NLLRegionVariableOrigin::Existential { was_placeholder: false } => { + | NLLRegionVariableOrigin::Existential { from_forall: false } => { true } NLLRegionVariableOrigin::Placeholder(_) - | NLLRegionVariableOrigin::Existential { was_placeholder: true } => { + | NLLRegionVariableOrigin::Existential { from_forall: true } => { false } }; diff --git a/src/librustc_mir/borrow_check/nll/region_infer/mod.rs b/src/librustc_mir/borrow_check/nll/region_infer/mod.rs index 164f7b0627c2d..dbb810db555b4 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/mod.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/mod.rs @@ -1612,7 +1612,7 @@ impl<'tcx> RegionDefinition<'tcx> { let origin = match rv_origin { RegionVariableOrigin::NLL(origin) => origin, - _ => NLLRegionVariableOrigin::Existential { was_placeholder: false }, + _ => NLLRegionVariableOrigin::Existential { from_forall: false }, }; Self { origin, universe, external_name: None } diff --git a/src/librustc_mir/borrow_check/nll/renumber.rs b/src/librustc_mir/borrow_check/nll/renumber.rs index 6e4db36bdce03..315c369716e38 100644 --- a/src/librustc_mir/borrow_check/nll/renumber.rs +++ b/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 { was_placeholder: false }; + let origin = NLLRegionVariableOrigin::Existential { from_forall: false }; infcx.next_nll_region_var(origin) }) } diff --git a/src/librustc_mir/borrow_check/nll/type_check/relate_tys.rs b/src/librustc_mir/borrow_check/nll/type_check/relate_tys.rs index 919fcdbd39ba0..80bf0478128c7 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/relate_tys.rs +++ b/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, was_placeholder: bool) -> 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 { was_placeholder }; + let origin = NLLRegionVariableOrigin::Existential { from_forall }; self.infcx.next_nll_region_var(origin) } else { self.infcx.tcx.lifetimes.re_erased @@ -89,7 +89,7 @@ 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 { - was_placeholder: false + from_forall: false }, universe) } From ba54ef8110db16475b19ff9a1eaa46c696fa9584 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 1 Oct 2019 21:44:13 -0400 Subject: [PATCH 4/4] Add comments, rename variable --- .../nll/region_infer/error_reporting/mod.rs | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs index fa02e2dd234ef..984da4432861e 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs @@ -157,7 +157,42 @@ impl<'tcx> RegionInferenceContext<'tcx> { let target_scc = self.constraint_sccs.scc(target_region); let mut range = 0..path.len(); - let should_reverse = match from_region_origin { + // 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 @@ -173,7 +208,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { let constraint_sup_scc = self.constraint_sccs.scc(constraint.sup); - if should_reverse { + if blame_source { match categorized_path[*i].0 { ConstraintCategory::OpaqueType | ConstraintCategory::Boring | ConstraintCategory::BoringNoLocation | ConstraintCategory::Internal => false, @@ -190,14 +225,14 @@ impl<'tcx> RegionInferenceContext<'tcx> { } }; - let best_choice = if should_reverse { + let best_choice = if blame_source { range.rev().find(find_region) } else { range.find(find_region) }; - debug!("best_blame_constraint: best_choice={:?} should_reverse={}", - best_choice, should_reverse); + 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) {