Skip to content

Commit 1245467

Browse files
committed
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.
1 parent 702b45e commit 1245467

File tree

12 files changed

+116
-26
lines changed

12 files changed

+116
-26
lines changed

src/librustc/infer/mod.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -418,15 +418,17 @@ pub enum NLLRegionVariableOrigin {
418418
/// from a `for<'a> T` binder). Meant to represent "any region".
419419
Placeholder(ty::PlaceholderRegion),
420420

421-
Existential,
421+
Existential {
422+
was_placeholder: bool
423+
},
422424
}
423425

424426
impl NLLRegionVariableOrigin {
425427
pub fn is_universal(self) -> bool {
426428
match self {
427429
NLLRegionVariableOrigin::FreeRegion => true,
428430
NLLRegionVariableOrigin::Placeholder(..) => true,
429-
NLLRegionVariableOrigin::Existential => false,
431+
NLLRegionVariableOrigin::Existential{ .. } => false,
430432
}
431433
}
432434

src/librustc/infer/nll_relate/mod.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ pub trait TypeRelatingDelegate<'tcx> {
9393
/// we will invoke this method to instantiate `'a` with an
9494
/// inference variable (though `'b` would be instantiated first,
9595
/// as a placeholder).
96-
fn next_existential_region_var(&mut self) -> ty::Region<'tcx>;
96+
fn next_existential_region_var(&mut self, was_placeholder: bool) -> ty::Region<'tcx>;
9797

9898
/// Creates a new region variable representing a
9999
/// higher-ranked region that is instantiated universally.
@@ -193,7 +193,7 @@ where
193193
let placeholder = ty::PlaceholderRegion { universe, name: br };
194194
delegate.next_placeholder_region(placeholder)
195195
} else {
196-
delegate.next_existential_region_var()
196+
delegate.next_existential_region_var(true)
197197
}
198198
}
199199
};

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

+51-12
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,11 @@ impl<'tcx> RegionInferenceContext<'tcx> {
9898
&self,
9999
body: &Body<'tcx>,
100100
from_region: RegionVid,
101+
from_region_origin: NLLRegionVariableOrigin,
101102
target_test: impl Fn(RegionVid) -> bool,
102103
) -> (ConstraintCategory, bool, Span) {
103-
debug!("best_blame_constraint(from_region={:?})", from_region);
104+
debug!("best_blame_constraint(from_region={:?}, from_region_origin={:?})",
105+
from_region, from_region_origin);
104106

105107
// Find all paths
106108
let (path, target_region) =
@@ -153,19 +155,50 @@ impl<'tcx> RegionInferenceContext<'tcx> {
153155
// we still want to screen for an "interesting" point to
154156
// highlight (e.g., a call site or something).
155157
let target_scc = self.constraint_sccs.scc(target_region);
156-
let best_choice = (0..path.len()).rev().find(|&i| {
157-
let constraint = path[i];
158+
let mut range = 0..path.len();
159+
160+
let should_reverse = match from_region_origin {
161+
NLLRegionVariableOrigin::FreeRegion
162+
| NLLRegionVariableOrigin::Existential { was_placeholder: false } => {
163+
true
164+
}
165+
NLLRegionVariableOrigin::Placeholder(_)
166+
| NLLRegionVariableOrigin::Existential { was_placeholder: true } => {
167+
false
168+
}
169+
};
170+
171+
let find_region = |i: &usize| {
172+
let constraint = path[*i];
158173

159174
let constraint_sup_scc = self.constraint_sccs.scc(constraint.sup);
160175

161-
match categorized_path[i].0 {
162-
ConstraintCategory::OpaqueType | ConstraintCategory::Boring |
163-
ConstraintCategory::BoringNoLocation | ConstraintCategory::Internal => false,
164-
ConstraintCategory::TypeAnnotation | ConstraintCategory::Return |
165-
ConstraintCategory::Yield => true,
166-
_ => constraint_sup_scc != target_scc,
176+
if should_reverse {
177+
match categorized_path[*i].0 {
178+
ConstraintCategory::OpaqueType | ConstraintCategory::Boring |
179+
ConstraintCategory::BoringNoLocation | ConstraintCategory::Internal => false,
180+
ConstraintCategory::TypeAnnotation | ConstraintCategory::Return |
181+
ConstraintCategory::Yield => true,
182+
_ => constraint_sup_scc != target_scc,
183+
}
184+
} else {
185+
match categorized_path[*i].0 {
186+
ConstraintCategory::OpaqueType | ConstraintCategory::Boring |
187+
ConstraintCategory::BoringNoLocation | ConstraintCategory::Internal => false,
188+
_ => true
189+
}
167190
}
168-
});
191+
};
192+
193+
let best_choice = if should_reverse {
194+
range.rev().find(find_region)
195+
} else {
196+
range.find(find_region)
197+
};
198+
199+
debug!("best_blame_constraint: best_choice={:?} should_reverse={}",
200+
best_choice, should_reverse);
201+
169202
if let Some(i) = best_choice {
170203
if let Some(next) = categorized_path.get(i + 1) {
171204
if categorized_path[i].0 == ConstraintCategory::Return
@@ -297,12 +330,13 @@ impl<'tcx> RegionInferenceContext<'tcx> {
297330
infcx: &'a InferCtxt<'a, 'tcx>,
298331
mir_def_id: DefId,
299332
fr: RegionVid,
333+
fr_origin: NLLRegionVariableOrigin,
300334
outlived_fr: RegionVid,
301335
renctx: &mut RegionErrorNamingCtx,
302336
) -> DiagnosticBuilder<'a> {
303337
debug!("report_error(fr={:?}, outlived_fr={:?})", fr, outlived_fr);
304338

305-
let (category, _, span) = self.best_blame_constraint(body, fr, |r| {
339+
let (category, _, span) = self.best_blame_constraint(body, fr, fr_origin, |r| {
306340
self.provides_universal_region(r, fr, outlived_fr)
307341
});
308342

@@ -709,6 +743,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
709743
let (category, from_closure, span) = self.best_blame_constraint(
710744
body,
711745
borrow_region,
746+
NLLRegionVariableOrigin::FreeRegion,
712747
|r| self.provides_universal_region(r, borrow_region, outlived_region)
713748
);
714749

@@ -768,11 +803,13 @@ impl<'tcx> RegionInferenceContext<'tcx> {
768803
&self,
769804
body: &Body<'tcx>,
770805
fr1: RegionVid,
806+
fr1_origin: NLLRegionVariableOrigin,
771807
fr2: RegionVid,
772808
) -> (ConstraintCategory, Span) {
773809
let (category, _, span) = self.best_blame_constraint(
774810
body,
775811
fr1,
812+
fr1_origin,
776813
|r| self.provides_universal_region(r, fr1, fr2),
777814
);
778815
(category, span)
@@ -825,7 +862,9 @@ impl<'tcx> RegionInferenceContext<'tcx> {
825862
universe1.cannot_name(placeholder.universe)
826863
}
827864

828-
NLLRegionVariableOrigin::FreeRegion | NLLRegionVariableOrigin::Existential => false,
865+
NLLRegionVariableOrigin::FreeRegion | NLLRegionVariableOrigin::Existential { .. } => {
866+
false
867+
}
829868
}
830869
}
831870
}

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

+9-5
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
406406
}
407407
}
408408

409-
NLLRegionVariableOrigin::Existential => {
409+
NLLRegionVariableOrigin::Existential { .. } => {
410410
// For existential, regions, nothing to do.
411411
}
412412
}
@@ -1348,7 +1348,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
13481348
self.check_bound_universal_region(infcx, body, mir_def_id, fr, placeholder);
13491349
}
13501350

1351-
NLLRegionVariableOrigin::Existential => {
1351+
NLLRegionVariableOrigin::Existential { .. } => {
13521352
// nothing to check here
13531353
}
13541354
}
@@ -1461,7 +1461,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
14611461
debug!("check_universal_region: fr_minus={:?}", fr_minus);
14621462

14631463
let blame_span_category =
1464-
self.find_outlives_blame_span(body, longer_fr, shorter_fr);
1464+
self.find_outlives_blame_span(body, longer_fr,
1465+
NLLRegionVariableOrigin::FreeRegion,shorter_fr);
14651466

14661467
// Grow `shorter_fr` until we find some non-local regions. (We
14671468
// always will.) We'll call them `shorter_fr+` -- they're ever
@@ -1494,6 +1495,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
14941495
infcx,
14951496
mir_def_id,
14961497
longer_fr,
1498+
NLLRegionVariableOrigin::FreeRegion,
14971499
shorter_fr,
14981500
region_naming,
14991501
);
@@ -1547,7 +1549,9 @@ impl<'tcx> RegionInferenceContext<'tcx> {
15471549
};
15481550

15491551
// Find the code to blame for the fact that `longer_fr` outlives `error_fr`.
1550-
let (_, span) = self.find_outlives_blame_span(body, longer_fr, error_region);
1552+
let (_, span) = self.find_outlives_blame_span(
1553+
body, longer_fr, NLLRegionVariableOrigin::Placeholder(placeholder), error_region
1554+
);
15511555

15521556
// Obviously, this error message is far from satisfactory.
15531557
// At present, though, it only appears in unit tests --
@@ -1608,7 +1612,7 @@ impl<'tcx> RegionDefinition<'tcx> {
16081612

16091613
let origin = match rv_origin {
16101614
RegionVariableOrigin::NLL(origin) => origin,
1611-
_ => NLLRegionVariableOrigin::Existential,
1615+
_ => NLLRegionVariableOrigin::Existential { was_placeholder: false },
16121616
};
16131617

16141618
Self { origin, universe, external_name: None }

src/librustc_mir/borrow_check/nll/renumber.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ where
3535
infcx
3636
.tcx
3737
.fold_regions(value, &mut false, |_region, _depth| {
38-
let origin = NLLRegionVariableOrigin::Existential;
38+
let origin = NLLRegionVariableOrigin::Existential { was_placeholder: false };
3939
infcx.next_nll_region_var(origin)
4040
})
4141
}

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

+5-3
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,9 @@ impl TypeRelatingDelegate<'tcx> for NllTypeRelatingDelegate<'_, '_, 'tcx> {
6666
self.infcx.create_next_universe()
6767
}
6868

69-
fn next_existential_region_var(&mut self) -> ty::Region<'tcx> {
69+
fn next_existential_region_var(&mut self, was_placeholder: bool) -> ty::Region<'tcx> {
7070
if let Some(_) = &mut self.borrowck_context {
71-
let origin = NLLRegionVariableOrigin::Existential;
71+
let origin = NLLRegionVariableOrigin::Existential { was_placeholder };
7272
self.infcx.next_nll_region_var(origin)
7373
} else {
7474
self.infcx.tcx.lifetimes.re_erased
@@ -88,7 +88,9 @@ impl TypeRelatingDelegate<'tcx> for NllTypeRelatingDelegate<'_, '_, 'tcx> {
8888

8989
fn generalize_existential(&mut self, universe: ty::UniverseIndex) -> ty::Region<'tcx> {
9090
self.infcx
91-
.next_nll_region_var_in_universe(NLLRegionVariableOrigin::Existential, universe)
91+
.next_nll_region_var_in_universe(NLLRegionVariableOrigin::Existential {
92+
was_placeholder: false
93+
}, universe)
9294
}
9395

9496
fn push_outlives(&mut self, sup: ty::Region<'tcx>, sub: ty::Region<'tcx>) {

src/librustc_traits/chalk_context/unify.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ impl TypeRelatingDelegate<'tcx> for &mut ChalkTypeRelatingDelegate<'_, 'tcx> {
6565
self.infcx.create_next_universe()
6666
}
6767

68-
fn next_existential_region_var(&mut self) -> ty::Region<'tcx> {
68+
fn next_existential_region_var(&mut self, _was_placeholder: bool) -> ty::Region<'tcx> {
6969
self.infcx.next_region_var(RegionVariableOrigin::MiscVariable(DUMMY_SP))
7070
}
7171

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

+1
Original file line numberDiff line numberDiff line change
@@ -114,4 +114,5 @@ fn main() {
114114
//[nll]~^ ERROR higher-ranked subtype error
115115
let count = filter.count(); // Assert that we still have a valid stream.
116116
//[nll]~^ ERROR higher-ranked subtype error
117+
117118
}
+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// Test that NLL produces correct spans for higher-ranked subtyping errors.
2+
//
3+
// compile-flags:-Zno-leak-check
4+
5+
#![feature(nll)]
6+
7+
fn main() {
8+
let x: fn(&'static ()) = |_| {};
9+
let y: for<'a> fn(&'a ()) = x; //~ ERROR higher-ranked subtype error
10+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
error: higher-ranked subtype error
2+
--> $DIR/fn-subtype.rs:9:33
3+
|
4+
LL | let y: for<'a> fn(&'a ()) = x;
5+
| ^
6+
7+
error: aborting due to previous error
8+
+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// Test that NLL generates proper error spans for trait HRTB errors
2+
//
3+
// compile-flags:-Zno-leak-check
4+
5+
#![feature(nll)]
6+
7+
trait Foo<'a> {}
8+
9+
fn make_foo<'a>() -> Box<dyn Foo<'a>> {
10+
panic!()
11+
}
12+
13+
fn main() {
14+
let x: Box<dyn Foo<'static>> = make_foo();
15+
let y: Box<dyn for<'a> Foo<'a>> = x; //~ ERROR higher-ranked subtype error
16+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
error: higher-ranked subtype error
2+
--> $DIR/trait-hrtb.rs:15:39
3+
|
4+
LL | let y: Box<dyn for<'a> Foo<'a>> = x;
5+
| ^
6+
7+
error: aborting due to previous error
8+

0 commit comments

Comments
 (0)