Skip to content

Commit

Permalink
Auto merge of #133858 - dianne:better-blame-constraints-for-static, r…
Browse files Browse the repository at this point in the history
…=<try>

`best_blame_constraint`: Blame better constraints when failing to outlive `'static`

This fixes #132749 by changing which constraint is blamed for region errors when failing to prove a region outlives `'static`. The comments give a better explanation, but very roughly, all the `'static: R` edges in the region graph made `best_blame_constraint` consider every constraint (other than those from ascriptions, returns, and yields) uninteresting to point to, so it chose semi-randomly based on an ordering of how interesting each variant of `ConstraintCategory` is expected to be. This PR (admittedly hackily) makes it ignore all those edges, so that the existing logic works the same as it would when failing to outlive any other region.

Looking at UI test diffs, most of them that changed I think changed for the better. Unfortunately, since a lot of borrowck's diagnostics depend on exactly which constraint is blamed, some things broke. A list of what I'm not happy with:
- For `CopyBound` constraints, e.g. [`tests/ui/nll/do-not-ignore-lifetime-bounds-in-copy.stderr`](https://github.com/rust-lang/rust/compare/master...dianne:rust:better-blame-constraints-for-static?expand=1#diff-e220f1e433c5e48d9afd431787f6ff27fc66b653762ee8a0283e370c2d88e7d0), I think it's helpful to surface that the `Copy` impl introduces the bound. If this is an issue, maybe it's worth prioritizing it or adding a variant of `ExtraConstraintInfo` for it.
- Likewise for `UseAsConst` and `UseAsStatic`; I've already added a special case for those. Without a special case, this was blaming `CallArgument` in [`tests/ui/consts/const-mut-refs/mut_ref_in_final.stderr`](https://github.com/dianne/rust/blob/189b2f892e6d0809a77fc92fe1108a07d8de9be0/tests/ui/consts/const-mut-refs/mut_ref_in_final.stderr#L38), which seemed pretty egregious. I'm assuming we want to blame `UseAsConst`/`UseAsStatic` when possible, rather than add something to `ExtraConstraintInfo`, since it's pretty unambiguously to-blame for "outlives `'static`" constraints. The special-casing admittedly also changes the output for [`tests/ui/inline-const/const-match-pat-lifetime-err.rs`](https://github.com/rust-lang/rust/compare/master...dianne:rust:better-blame-constraints-for-static?expand=1#diff-e4d2c147aa96dd8dd963ec3d98ead9a8096c9de809d19ab379be3c53951ca1ca), but I think the new output there is fine; it's more direct about how `'a` and `'static` are getting related.
- The subdiagnostic [`BorrowExplanation::add_object_lifetime_default_note`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_borrowck/diagnostics/explain_borrow/enum.BorrowExplanation.html#method.add_object_lifetime_default_note) depends on `Cast` being reported as the constraint category, so I've added a special case for it to keep it disappearing from [`tests/ui/traits/trait-object-lifetime-default-note.stderr`](https://github.com/dianne/rust/blob/better-blame-constraints-for-static/tests/ui/traits/trait-object-lifetime-default-note.stderr)[^1] . As I've outlined in a `FIXME` comment though, I think that subdiagnostic needs changing. There's multiple cases throughout `tests/ui` where it would be helpful, but doesn't fire, because a different constraint is picked. #131008 would also benefit from that note, but there's not a coercion there at all. I tried making it fire in more cases, but fundamentally, since it doesn't *actually* check if an object lifetime default was used in the HIR, it produced some extraneous notes[^2]. I tried seeing if it'd be easy enough to fix that first, but it seems nontrivial[^3] enough to warrant a separate PR.

A final note: this may have perf consequences, since `best_blame_constraint` gets called on happy code too. If it's an issue, I can try to make it faster, or only do the expensive stuff when an error's been hit, or something. If nothing else, this makes the fallback logic (still relevant for invariant lifetimes[^4]) a bit faster.

[^1]: Incidentally, without the special-casing, this would pick `CallArgument`, which I think produces a nicer message, apart from the missing note.

[^2]: There's even some in current UI test output: [`tests/ui/borrowck/two-phase-surprise-no-conflict.stderr`](https://github.com/rust-lang/rust/blob/733616f7236b4be140ce851a30b3bb06532b9364/tests/ui/borrowck/two-phase-surprise-no-conflict.stderr#L68). The object lifetime is [explicitly provided](https://github.com/rust-lang/rust/blob/733616f7236b4be140ce851a30b3bb06532b9364/tests/ui/borrowck/two-phase-surprise-no-conflict.rs#L94), and despite the note, it's not `'static`.

[^3]: In case anyone reading this has advice: ultimately I think there has to be a choice between extraneous notes and missing notes unless "this object type had a missing lifetime in the HIR" is in crate metadata for some reason, since rustdoc doesn't elaborate object lifetime defaults, and the note is potentially helpful for library-users who may be wondering where a `'static` lifetime came from. That said, it's a bit confusing to point out lifetime defaults when they're not relevant[^5], so it's at least worth a best effort to look at user annotations. However, the HIR missing an object lifetime could be anywhere: a cast, an ascription, in the return type, in an input, in the signature for a function that's called, in an ADT field, in a type alias... I've considered looking at the entire output of `RegionInferenceContext::find_constraint_path_between_regions` and doing a best-effort association between typeck results (ideally MIR typeck for the region information) and whatever HIR seems relevant. But that seems like a lot of work for that note.

[^4]: Cycles in the region graph due to invariance also confuse `better_blame_constraint`'s attempt to pick a constraint where the regions aren't unified. I'm not sure if there's a better heuristic to use there, though; I played around a bit with it, but my experiments only made diagnostic output worse.

[^5]: Unless maybe there's a way of rewording the note so that it always makes sense to output when object lifetimes are involved in region errors?
  • Loading branch information
bors committed Dec 4, 2024
2 parents acabb52 + 189b2f8 commit 889e047
Show file tree
Hide file tree
Showing 23 changed files with 147 additions and 111 deletions.
46 changes: 37 additions & 9 deletions compiler/rustc_borrowck/src/region_infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2027,7 +2027,14 @@ impl<'tcx> RegionInferenceContext<'tcx> {
// 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 a special case, if the target region is 'static, it will always outlive the source,
// so they'll be in the same SCC. To get better diagnostics, we pretend those `'static: R`
// edges don't exist and use the resulting graph's SCCs.
let target_is_static = target_region == self.universal_regions().fr_static;
let sccs_without_static = target_is_static
.then(|| self.constraints.compute_sccs(RegionVid::MAX, &self.definitions));
let constraint_sccs = sccs_without_static.as_ref().unwrap_or(&self.constraint_sccs);
let target_scc = constraint_sccs.scc(target_region);
let mut range = 0..path.len();

// As noted above, when reporting an error, there is typically a chain of constraints
Expand Down Expand Up @@ -2075,7 +2082,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
let find_region = |i: &usize| {
let constraint = &path[*i];

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

if blame_source {
match categorized_path[*i].category {
Expand Down Expand Up @@ -2133,17 +2140,38 @@ impl<'tcx> RegionInferenceContext<'tcx> {
}
}

// If an "outlives 'static" constraint was from use as a const or static, blame that.
if target_is_static
&& blame_source
&& let Some(old_best) = categorized_path.iter().min_by_key(|p| p.category)
&& matches!(
old_best.category,
ConstraintCategory::UseAsConst
| ConstraintCategory::UseAsStatic
| ConstraintCategory::Cast {
is_implicit_coercion: true,
unsize_to: Some(_)
}
)
{
// FIXME(dianne): `BorrowExplanation::add_object_lifetime_default_note` depends on a
// coercion being blamed, so revert to the old blaming logic to prioritize that.
// The note's logic should be reworked, though; it's flaky (#131008 doesn't have a
// coercion, and even with this hack, one isn't always blamed when present).
// Only checking for a coercion also makes the note appear where it shouldn't
// shouldn't (e.g. `tests/ui/borrowck/two-phase-surprise-no-conflict.stderr`).
return (old_best.clone(), extra_info);
}

return (categorized_path[i].clone(), extra_info);
}

// 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.
categorized_path.sort_by_key(|p| p.category);
debug!("sorted_path={:#?}", categorized_path);
// If that search fails, everything may be in the same SCC. In particular, this will be the
// case when dealing with invariant lifetimes. Find what appears to be the most interesting
// point to report to the user via an even more ad-hoc guess.
let best_choice = categorized_path.into_iter().min_by_key(|p| p.category).unwrap();

(categorized_path.remove(0), extra_info)
(best_choice, extra_info)
}

pub(crate) fn universe_info(&self, universe: ty::UniverseIndex) -> UniverseInfo<'tcx> {
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/borrowck/fn-item-check-type-params.stderr
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: 1 addition & 1 deletion tests/ui/coroutine/resume-arg-outlives-2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ fn demo<'not_static>(s: &'not_static str) -> thread::JoinHandle<()> {
// exploit:
generator.as_mut().resume("");
generator.as_mut().resume(s); // <- generator hoards it as `let ctx`.
//~^ ERROR borrowed data escapes outside of function
thread::spawn(move || {
//~^ ERROR borrowed data escapes outside of function
thread::sleep(time::Duration::from_millis(200));
generator.as_mut().resume(""); // <- resumes from the last `yield`, running `dbg!(ctx)`.
})
Expand Down
24 changes: 14 additions & 10 deletions tests/ui/coroutine/resume-arg-outlives-2.stderr
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
error[E0521]: borrowed data escapes outside of function
--> $DIR/resume-arg-outlives-2.rs:20:5
--> $DIR/resume-arg-outlives-2.rs:21:5
|
LL | fn demo<'not_static>(s: &'not_static str) -> thread::JoinHandle<()> {
| ----------- - `s` is a reference that is only valid in the function body
| |
| lifetime `'not_static` defined here
LL | fn demo<'not_static>(s: &'not_static str) -> thread::JoinHandle<()> {
| ----------- - `s` is a reference that is only valid in the function body
| |
| lifetime `'not_static` defined here
...
LL | generator.as_mut().resume(s); // <- generator hoards it as `let ctx`.
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| `s` escapes the function body here
| argument requires that `'not_static` must outlive `'static`
LL | / thread::spawn(move || {
LL | |
LL | | thread::sleep(time::Duration::from_millis(200));
LL | | generator.as_mut().resume(""); // <- resumes from the last `yield`, running `dbg!(ctx)`.
LL | | })
| | ^
| | |
| |______`s` escapes the function body here
| argument requires that `'not_static` must outlive `'static`

error: aborting due to 1 previous error

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/impl-trait/precise-capturing/migration-note.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ fn needs_static() {
let a = display_len(&x);
//~^ ERROR `x` does not live long enough
//~| NOTE this call may capture more lifetimes than intended
//~| NOTE argument requires that `x` is borrowed for `'static`
//~| NOTE borrowed value does not live long enoug

fn needs_static(_: impl Sized + 'static) {}
needs_static(a);
//~^ NOTE argument requires that `x` is borrowed for `'static`
}
//~^ NOTE `x` dropped here while still borrowed

Expand Down Expand Up @@ -76,11 +76,11 @@ fn needs_static_mut() {
let a = display_len_mut(&mut x);
//~^ ERROR `x` does not live long enough
//~| NOTE this call may capture more lifetimes than intended
//~| NOTE argument requires that `x` is borrowed for `'static`
//~| NOTE borrowed value does not live long enough

fn needs_static(_: impl Sized + 'static) {}
needs_static(a);
//~^ NOTE argument requires that `x` is borrowed for `'static`
}
//~^ NOTE `x` dropped here while still borrowed

Expand Down
16 changes: 8 additions & 8 deletions tests/ui/impl-trait/precise-capturing/migration-note.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ LL | let x = vec![1];
| - binding `x` declared here
LL |
LL | let a = display_len(&x);
| ------------^^-
| | |
| | borrowed value does not live long enough
| argument requires that `x` is borrowed for `'static`
| ^^ borrowed value does not live long enough
...
LL | needs_static(a);
| --------------- argument requires that `x` is borrowed for `'static`
LL |
LL | }
| - `x` dropped here while still borrowed
|
Expand Down Expand Up @@ -118,11 +118,11 @@ LL | let mut x = vec![1];
| ----- binding `x` declared here
LL |
LL | let a = display_len_mut(&mut x);
| ----------------^^^^^^-
| | |
| | borrowed value does not live long enough
| argument requires that `x` is borrowed for `'static`
| ^^^^^^ borrowed value does not live long enough
...
LL | needs_static(a);
| --------------- argument requires that `x` is borrowed for `'static`
LL |
LL | }
| - `x` dropped here while still borrowed
|
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/inline-const/const-match-pat-lifetime-err.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ fn match_covariant_ref<'a>() {
// `y.0`), but using the associated const directly in the pattern also
// errors.
let y: (CovariantRef<'static, _>,) = (CovariantRef(&()),);
//~^ ERROR lifetime may not live long enough
match y.0 {
const { CovariantRef::<'a>::NEW } => (),
//~^ ERROR lifetime may not live long enough
}
}

Expand Down
6 changes: 3 additions & 3 deletions tests/ui/inline-const/const-match-pat-lifetime-err.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ LL | }
| - `y` dropped here while still borrowed

error: lifetime may not live long enough
--> $DIR/const-match-pat-lifetime-err.rs:37:12
--> $DIR/const-match-pat-lifetime-err.rs:39:17
|
LL | fn match_covariant_ref<'a>() {
| -- lifetime `'a` defined here
...
LL | let y: (CovariantRef<'static, _>,) = (CovariantRef(&()),);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`
LL | const { CovariantRef::<'a>::NEW } => (),
| ^^^^^^^^^^^^^^^^^^^^^^^ using this value as a constant requires that `'a` must outlive `'static`

error: aborting due to 2 previous errors

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/kindck/kindck-impl-type-params.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,13 @@ LL | struct Foo; // does not impl Copy
|

error: lifetime may not live long enough
--> $DIR/kindck-impl-type-params.rs:30:19
--> $DIR/kindck-impl-type-params.rs:30:13
|
LL | fn foo<'a>() {
| -- lifetime `'a` defined here
LL | let t: S<&'a isize> = S(marker::PhantomData);
LL | let a = &t as &dyn Gettable<&'a isize>;
| ^^^^^^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cast requires that `'a` must outlive `'static`

error: aborting due to 7 previous errors

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ fn inner(mut foo: &[u8]) {
let refcell = RefCell::new(&mut foo);
//~^ ERROR `foo` does not live long enough
let read = &refcell as &RefCell<dyn Read>;
//~^ ERROR lifetime may not live long enough

read_thing(read);
//~^ ERROR lifetime may not live long enough
}

fn read_thing(refcell: &RefCell<dyn Read>) {}
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,21 @@ LL | fn inner(mut foo: &[u8]) {
| ------- binding `foo` declared here
LL | let refcell = RefCell::new(&mut foo);
| ^^^^^^^^ borrowed value does not live long enough
LL |
LL | let read = &refcell as &RefCell<dyn Read>;
| ------------------------------ cast requires that `foo` is borrowed for `'static`
...
LL | read_thing(read);
| ---- coercion requires that `foo` is borrowed for `'static`
LL |
LL | }
| - `foo` dropped here while still borrowed

error: lifetime may not live long enough
--> $DIR/issue-90600-expected-return-static-indirect.rs:9:16
--> $DIR/issue-90600-expected-return-static-indirect.rs:11:16
|
LL | fn inner(mut foo: &[u8]) {
| - let's call the lifetime of this reference `'1`
...
LL | let read = &refcell as &RefCell<dyn Read>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cast requires that `'1` must outlive `'static`
LL | read_thing(read);
| ^^^^ coercion requires that `'1` must outlive `'static`

error: aborting due to 2 previous errors

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ error[E0597]: `a` does not live long enough
LL | let a = 0;
| - binding `a` declared here
LL | let cell = Cell::new(&a);
| ----------^^-
| | |
| | borrowed value does not live long enough
| argument requires that `a` is borrowed for `'static`
| ^^ borrowed value does not live long enough
...
LL | cell_x.set(cell_a.get()); // forces 'a: 'x, implies 'a = 'static -> borrow error
| ------------------------ argument requires that `a` is borrowed for `'static`
LL | })
LL | }
| - `a` dropped here while still borrowed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,9 @@ fn demand_y<'x, 'y>(_cell_x: &Cell<&'x u32>, _cell_y: &Cell<&'y u32>, _y: &'y u3
#[rustc_regions]
fn supply<'a, 'b>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>) {
establish_relationships(&cell_a, &cell_b, |_outlives, x, y| {
//~^ ERROR borrowed data escapes outside of function

// Only works if 'x: 'y:
demand_y(x, y, x.get())
//~^ ERROR borrowed data escapes outside of function
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,18 @@ LL | fn supply<'a, 'b>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>) {
= note: defining type: supply

error[E0521]: borrowed data escapes outside of function
--> $DIR/propagate-approximated-shorter-to-static-no-bound.rs:32:5
--> $DIR/propagate-approximated-shorter-to-static-no-bound.rs:34:9
|
LL | fn supply<'a, 'b>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>) {
| -- ------ `cell_a` is a reference that is only valid in the function body
| |
| lifetime `'a` defined here
LL | / establish_relationships(&cell_a, &cell_b, |_outlives, x, y| {
LL | |
LL | |
LL | | // Only works if 'x: 'y:
LL | | demand_y(x, y, x.get())
LL | | });
| | ^
| | |
| |______`cell_a` escapes the function body here
| argument requires that `'a` must outlive `'static`
|
= note: requirement occurs because of the type `Cell<&'?9 u32>`, which makes the generic argument `&'?9 u32` invariant
= note: the struct `Cell<T>` is invariant over the parameter `T`
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
LL | fn supply<'a, 'b>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>) {
| -- ------ `cell_a` is a reference that is only valid in the function body
| |
| lifetime `'a` defined here
...
LL | demand_y(x, y, x.get())
| ^^^^^^^^^^^^^^^^^^^^^^^
| |
| `cell_a` escapes the function body here
| argument requires that `'a` must outlive `'static`

error: aborting due to 1 previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@ fn demand_y<'x, 'y>(_cell_x: &Cell<&'x u32>, _cell_y: &Cell<&'y u32>, _y: &'y u3
#[rustc_regions]
fn supply<'a, 'b>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>) {
establish_relationships(&cell_a, &cell_b, |_outlives1, _outlives2, x, y| {
//~^ ERROR borrowed data escapes outside of function

// Only works if 'x: 'y:
demand_y(x, y, x.get())
//~^ ERROR borrowed data escapes outside of function
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,18 @@ LL | fn supply<'a, 'b>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>) {
= note: defining type: supply

error[E0521]: borrowed data escapes outside of function
--> $DIR/propagate-approximated-shorter-to-static-wrong-bound.rs:35:5
--> $DIR/propagate-approximated-shorter-to-static-wrong-bound.rs:37:9
|
LL | fn supply<'a, 'b>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>) {
| -- ------ `cell_a` is a reference that is only valid in the function body
| |
| lifetime `'a` defined here
LL | / establish_relationships(&cell_a, &cell_b, |_outlives1, _outlives2, x, y| {
LL | |
LL | |
LL | | // Only works if 'x: 'y:
LL | | demand_y(x, y, x.get())
LL | | });
| | ^
| | |
| |______`cell_a` escapes the function body here
| argument requires that `'a` must outlive `'static`
|
= note: requirement occurs because of the type `Cell<&'?10 u32>`, which makes the generic argument `&'?10 u32` invariant
= note: the struct `Cell<T>` is invariant over the parameter `T`
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
LL | fn supply<'a, 'b>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>) {
| -- ------ `cell_a` is a reference that is only valid in the function body
| |
| lifetime `'a` defined here
...
LL | demand_y(x, y, x.get())
| ^^^^^^^^^^^^^^^^^^^^^^^
| |
| `cell_a` escapes the function body here
| argument requires that `'a` must outlive `'static`

error: aborting due to 1 previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ error[E0597]: `s` does not live long enough
LL | let s = 2;
| - binding `s` declared here
LL | let a = (Foo(&s),);
| ^^ borrowed value does not live long enough
LL | drop(a.0);
| --- copying this value requires that `s` is borrowed for `'static`
LL | drop(a.0);
| -----^^---
| | |
| | borrowed value does not live long enough
| assignment requires that `s` is borrowed for `'static`
...
LL | }
| - `s` dropped here while still borrowed

Expand Down
Loading

0 comments on commit 889e047

Please sign in to comment.