Skip to content

Commit

Permalink
best_blame_constraint: don't filter constraints by sup SCC
Browse files Browse the repository at this point in the history 
The SCCs of the region graph are not a reliable heuristic to use for blaming an interesting
constraint for diagnostics. For region errors, if the outlived region is `'static`, or the involved
types are invariant in their lifetiems, there will be cycles in the constraint graph containing both
the target region and the most interesting constraints to blame. To get better diagnostics in these
cases, this commit removes that heuristic.
  • Loading branch information
@dianne
dianne committed Dec 14, 2024
1 parent 5f27c95 commit 59512d7
Show file tree
Hide file tree
Showing 101 changed files with 588 additions and 510 deletions.
60 changes: 11 additions & 49 deletions compiler/rustc_borrowck/src/region_infer/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1940,7 +1940,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
target_test: impl Fn(RegionVid) -> bool,
) -> (BlameConstraint<'tcx>, Vec<OutlivesConstraint<'tcx>>) {
// Find all paths
let (path, target_region) =
let (path, _) =
self.find_constraint_paths_between_regions(from_region, target_test).unwrap();
debug!(
"path={:#?}",
Expand Down Expand Up @@ -1972,29 +1972,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
})
.unwrap_or_else(|| ObligationCauseCode::Misc);

// To find the best span to cite, we first try to look for the
// final constraint that is interesting and where the `sup` is
// not unified with the ultimate target region. The reason
// for this is that we have a chain of constraints that lead
// from the source to the target region, something like:
//
// '0: '1 ('0 is the source)
// '1: '2
// '2: '3
// '3: '4
// '4: '5
// '5: '6 ('6 is the target)
//
// Some of those regions are unified with `'6` (in the same
// SCC). We want to screen those out. After that point, the
// "closest" constraint we have to the end is going to be the
// most likely to be the point where the value escapes -- but
// 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);

// 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.
// 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,
Expand Down Expand Up @@ -2036,30 +2015,14 @@ impl<'tcx> RegionInferenceContext<'tcx> {
};

let interesting_to_blame = |constraint: &OutlivesConstraint<'tcx>| {
let constraint_sup_scc = self.constraint_sccs.scc(constraint.sup);

if blame_source {
match constraint.category {
ConstraintCategory::OpaqueType
!matches!(
constraint.category,
ConstraintCategory::OpaqueType
| ConstraintCategory::Boring
| ConstraintCategory::BoringNoLocation
| ConstraintCategory::Internal
| ConstraintCategory::Predicate(_) => false,
ConstraintCategory::TypeAnnotation
| ConstraintCategory::Return(_)
| ConstraintCategory::Yield => true,
_ => constraint_sup_scc != target_scc,
}
} else {
!matches!(
constraint.category,
ConstraintCategory::OpaqueType
| ConstraintCategory::Boring
| ConstraintCategory::BoringNoLocation
| ConstraintCategory::Internal
| ConstraintCategory::Predicate(_)
)
}
| ConstraintCategory::Predicate(_)
)
};

let best_choice = if blame_source {
Expand Down Expand Up @@ -2100,10 +2063,9 @@ impl<'tcx> RegionInferenceContext<'tcx> {
Some(i) => path[i],

None => {
// If that search fails, that is.. unusual. Maybe everything
// is in the same SCC or something. In that case, find what
// appears to be the most interesting point to report to the
// user via an even more ad-hoc guess.
// If that search fails, the only constraints on the path are those that we try not
// to blame. In that case, find what appears to be the most interesting point to
// report to the user via an even more ad-hoc guess.
*path.iter().min_by_key(|p| p.category).unwrap()
}
};
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/associated-types/associated-types-project-from-hrtb-in-fn-body.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ LL | fn bar<'a, 'b, I : for<'x> Foo<&'x isize>>(
| lifetime `'a` defined here
...
LL | let z: I::A = if cond { x } else { y };
| ^ assignment requires that `'a` must outlive `'b`
| ^ assignment requires that `'b` must outlive `'a`
|
= help: consider adding the following bound: `'a: 'b`
= help: consider adding the following bound: `'b: 'a`

error: lifetime may not live long enough
--> $DIR/associated-types-project-from-hrtb-in-fn-body.rs:22:40
Expand All @@ -20,9 +20,9 @@ LL | fn bar<'a, 'b, I : for<'x> Foo<&'x isize>>(
| lifetime `'a` defined here
...
LL | let z: I::A = if cond { x } else { y };
| ^ assignment requires that `'b` must outlive `'a`
| ^ assignment requires that `'a` must outlive `'b`
|
= help: consider adding the following bound: `'b: 'a`
= help: consider adding the following bound: `'a: 'b`

help: `'a` and `'b` must be the same: replace one with the other

Expand Down
8 changes: 4 additions & 4 deletions tests/ui/associated-types/cache/project-fn-ret-invariant.oneuse.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ LL | fn baz<'a, 'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
| lifetime `'a` defined here
LL | let f = foo; // <-- No consistent type can be inferred for `f` here.
LL | let a = bar(f, x);
| ^^^^^^^^^ argument requires that `'a` must outlive `'b`
| ^^^^^^^^^ argument requires that `'b` must outlive `'a`
|
= help: consider adding the following bound: `'a: 'b`
= help: consider adding the following bound: `'b: 'a`
= note: requirement occurs because of the type `Type<'_>`, which makes the generic argument `'_` invariant
= note: the struct `Type<'a>` is invariant over the parameter `'a`
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
Expand All @@ -23,9 +23,9 @@ LL | fn baz<'a, 'b>(x: Type<'a>, y: Type<'b>) -> (Type<'a>, Type<'b>) {
| lifetime `'a` defined here
...
LL | let b = bar(f, y);
| ^^^^^^^^^ argument requires that `'b` must outlive `'a`
| ^^^^^^^^^ argument requires that `'a` must outlive `'b`
|
= help: consider adding the following bound: `'b: 'a`
= help: consider adding the following bound: `'a: 'b`
= note: requirement occurs because of the type `Type<'_>`, which makes the generic argument `'_` invariant
= note: the struct `Type<'a>` is invariant over the parameter `'a`
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
Expand Down
48 changes: 24 additions & 24 deletions tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ LL | let c = async || { println!("{}", *x); };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough
LL | outlives::<'a>(c());
LL | outlives::<'a>(call_once(c));
| ------------ argument requires that `x` is borrowed for `'a`
| ---------------------------- argument requires that `x` is borrowed for `'a`
...
LL | }
| - `x` dropped here while still borrowed
Expand All @@ -21,10 +21,10 @@ LL | fn simple<'a>(x: &'a i32) {
LL | let c = async move || { println!("{}", *x); };
| - binding `c` declared here
LL | outlives::<'a>(c());
| ^--
| |
| borrowed value does not live long enough
| argument requires that `c` is borrowed for `'a`
| ---------------^---
| | |
| | borrowed value does not live long enough
| argument requires that `c` is borrowed for `'a`
LL | outlives::<'a>(call_once(c));
LL | }
| - `c` dropped here while still borrowed
Expand All @@ -38,7 +38,7 @@ LL | let c = async || { println!("{}", *x.0); };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough
LL | outlives::<'a>(c());
LL | outlives::<'a>(call_once(c));
| ------------ argument requires that `x` is borrowed for `'a`
| ---------------------------- argument requires that `x` is borrowed for `'a`
...
LL | }
| - `x` dropped here while still borrowed
Expand All @@ -52,7 +52,7 @@ LL | let c = async || { println!("{}", *x.0); };
| ---------------------------------- borrow of `x` occurs here
LL | outlives::<'a>(c());
LL | outlives::<'a>(call_once(c));
| ------------ argument requires that `x` is borrowed for `'a`
| ---------------------------- argument requires that `x` is borrowed for `'a`
LL |
LL | let c = async move || { println!("{}", *x.0); };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move out of `x` occurs here
Expand All @@ -66,10 +66,10 @@ LL | fn through_field<'a>(x: S<'a>) {
LL | let c = async move || { println!("{}", *x.0); };
| - binding `c` declared here
LL | outlives::<'a>(c());
| ^--
| |
| borrowed value does not live long enough
| argument requires that `c` is borrowed for `'a`
| ---------------^---
| | |
| | borrowed value does not live long enough
| argument requires that `c` is borrowed for `'a`
LL | outlives::<'a>(call_once(c));
LL | }
| - `c` dropped here while still borrowed
Expand All @@ -83,10 +83,10 @@ LL | fn through_field<'a>(x: S<'a>) {
LL | let c = async move || { println!("{}", *x.0); };
| - binding `c` declared here
LL | outlives::<'a>(c());
| ---
| |
| borrow of `c` occurs here
| argument requires that `c` is borrowed for `'a`
| -------------------
| | |
| | borrow of `c` occurs here
| argument requires that `c` is borrowed for `'a`
LL | outlives::<'a>(call_once(c));
| ^ move out of `c` occurs here

Expand All @@ -99,18 +99,18 @@ LL | let c = async || { println!("{}", *x.0); };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough
LL | outlives::<'a>(c());
LL | outlives::<'a>(call_once(c));
| ------------ argument requires that `x` is borrowed for `'a`
| ---------------------------- argument requires that `x` is borrowed for `'a`
LL | }
| - `x` dropped here while still borrowed

error[E0621]: explicit lifetime required in the type of `x`
--> $DIR/without-precise-captures-we-are-powerless.rs:38:20
--> $DIR/without-precise-captures-we-are-powerless.rs:38:5
|
LL | fn through_field_and_ref<'a>(x: &S<'a>) {
| ------ help: add explicit lifetime `'a` to the type of `x`: `&'a S<'a>`
...
LL | outlives::<'a>(call_once(c));
| ^^^^^^^^^^^^ lifetime `'a` required
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime `'a` required

error[E0597]: `c` does not live long enough
--> $DIR/without-precise-captures-we-are-powerless.rs:43:20
Expand All @@ -120,22 +120,22 @@ LL | fn through_field_and_ref_move<'a>(x: &S<'a>) {
LL | let c = async move || { println!("{}", *x.0); };
| - binding `c` declared here
LL | outlives::<'a>(c());
| ^--
| |
| borrowed value does not live long enough
| argument requires that `c` is borrowed for `'a`
| ---------------^---
| | |
| | borrowed value does not live long enough
| argument requires that `c` is borrowed for `'a`
LL | outlives::<'a>(call_once(c));
LL | }
| - `c` dropped here while still borrowed

error[E0621]: explicit lifetime required in the type of `x`
--> $DIR/without-precise-captures-we-are-powerless.rs:44:20
--> $DIR/without-precise-captures-we-are-powerless.rs:44:5
|
LL | fn through_field_and_ref_move<'a>(x: &S<'a>) {
| ------ help: add explicit lifetime `'a` to the type of `x`: `&'a S<'a>`
...
LL | outlives::<'a>(call_once(c));
| ^^^^^^^^^^^^ lifetime `'a` required
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime `'a` required

error: aborting due to 10 previous errors

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/async-await/issue-76547.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ impl<'a> Future for ListFut<'a> {
}

async fn fut(bufs: &mut [&mut [u8]]) {
ListFut(bufs).await
//~^ ERROR lifetime may not live long enough
ListFut(bufs).await
}

pub struct ListFut2<'a>(&'a mut [&'a mut [u8]]);
Expand All @@ -31,8 +31,8 @@ impl<'a> Future for ListFut2<'a> {
}

async fn fut2(bufs: &mut [&mut [u8]]) -> i32 {
ListFut2(bufs).await
//~^ ERROR lifetime may not live long enough
ListFut2(bufs).await
}

fn main() {}
22 changes: 10 additions & 12 deletions tests/ui/async-await/issue-76547.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,27 +1,25 @@
error: lifetime may not live long enough
--> $DIR/issue-76547.rs:20:13
--> $DIR/issue-76547.rs:19:14
|
LL | async fn fut(bufs: &mut [&mut [u8]]) {
| - - let's call the lifetime of this reference `'2`
| |
| let's call the lifetime of this reference `'1`
LL | ListFut(bufs).await
| ^^^^ this usage requires that `'1` must outlive `'2`
| ^^^^ - - let's call the lifetime of this reference `'2`
| | |
| | let's call the lifetime of this reference `'1`
| assignment requires that `'1` must outlive `'2`
|
help: consider introducing a named lifetime parameter
|
LL | async fn fut<'a>(bufs: &'a mut [&'a mut [u8]]) {
| ++++ ++ ++

error: lifetime may not live long enough
--> $DIR/issue-76547.rs:34:14
--> $DIR/issue-76547.rs:33:15
|
LL | async fn fut2(bufs: &mut [&mut [u8]]) -> i32 {
| - - let's call the lifetime of this reference `'2`
| |
| let's call the lifetime of this reference `'1`
LL | ListFut2(bufs).await
| ^^^^ this usage requires that `'1` must outlive `'2`
| ^^^^ - - let's call the lifetime of this reference `'2`
| | |
| | let's call the lifetime of this reference `'1`
| assignment requires that `'1` must outlive `'2`
|
help: consider introducing a named lifetime parameter
|
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/borrowck/fn-item-check-type-params.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ LL | extend_lt(val);
| argument requires that `'a` must outlive `'static`

error: lifetime may not live long enough
--> $DIR/fn-item-check-type-params.rs:39:12
--> $DIR/fn-item-check-type-params.rs:39:31
|
LL | pub fn test_coercion<'a>() {
| -- lifetime `'a` defined here
LL | let _: fn(&'a str) -> _ = extend_lt;
| ^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`
| ^^^^^^^^^ coercion requires that `'a` must outlive `'static`

error[E0716]: temporary value dropped while borrowed
--> $DIR/fn-item-check-type-params.rs:48:11
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/borrowck/issue-7573.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ pub fn remove_package_from_database() {
lines_to_use.push(installed_id);
//~^ ERROR borrowed data escapes outside of closure
//~| NOTE `installed_id` escapes the closure body here
//~| NOTE requirement occurs because of a mutable reference to `Vec<&CrateId>`
//~| NOTE mutable references are invariant over their type parameter
};
list_database(push_id);

Expand Down
4 changes: 4 additions & 0 deletions tests/ui/borrowck/issue-7573.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ LL | let push_id = |installed_id: &CrateId| {
LL |
LL | lines_to_use.push(installed_id);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `installed_id` escapes the closure body here
|
= note: requirement occurs because of a mutable reference to `Vec<&CrateId>`
= note: mutable references are invariant over their type parameter
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

error: aborting due to 1 previous error

Expand Down
22 changes: 10 additions & 12 deletions tests/ui/borrowck/two-phase-surprise-no-conflict.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,13 @@ error[E0502]: cannot borrow `*reg` as mutable because it is also borrowed as imm
LL | fn register_plugins<'a>(mk_reg: impl Fn() -> &'a mut Registry<'a>) {
| -- lifetime `'a` defined here
...
LL | let reg = mk_reg();
| -------- assignment requires that `reg.sess_mut` is borrowed for `'a`
LL | reg.register_univ(Box::new(CapturePass::new(&reg.sess_mut)));
| ^^^^^^^^^^^^^^^^^^-----------------------------------------^
| | | |
| | | immutable borrow occurs here
| | coercion requires that `reg.sess_mut` is borrowed for `'a`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-------------^^^
| | |
| | immutable borrow occurs here
| mutable borrow occurs here
|
= note: due to object lifetime defaults, `Box<dyn for<'b> LateLintPass<'b>>` actually means `Box<(dyn for<'b> LateLintPass<'b> + 'static)>`

error[E0502]: cannot borrow `*reg` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-surprise-no-conflict.rs:144:5
Expand Down Expand Up @@ -115,14 +114,13 @@ error[E0499]: cannot borrow `*reg` as mutable more than once at a time
LL | fn register_plugins<'a>(mk_reg: impl Fn() -> &'a mut Registry<'a>) {
| -- lifetime `'a` defined here
...
LL | let reg = mk_reg();
| -------- assignment requires that `reg.sess_mut` is borrowed for `'a`
LL | reg.register_univ(Box::new(CapturePass::new_mut(&mut reg.sess_mut)));
| ^^^^^^^^^^^^^^^^^^-------------------------------------------------^
| | | |
| | | first mutable borrow occurs here
| | coercion requires that `reg.sess_mut` is borrowed for `'a`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------------^^^
| | |
| | first mutable borrow occurs here
| second mutable borrow occurs here
|
= note: due to object lifetime defaults, `Box<dyn for<'b> LateLintPass<'b>>` actually means `Box<(dyn for<'b> LateLintPass<'b> + 'static)>`

error[E0499]: cannot borrow `reg.sess_mut` as mutable more than once at a time
--> $DIR/two-phase-surprise-no-conflict.rs:158:53
Expand Down
Loading

0 comments on commit 59512d7

Please sign in to comment.