Add variance-related information to lifetime error messages #85343

Aaron1011 · 2021-05-15T19:19:28Z

This PR adds a basic framework for displaying variance-related information in error messages. For example:

error: lifetime may not live long enough
  --> $DIR/type-check-pointer-comparisons.rs:12:5
   |
LL | fn compare_mut<'a, 'b>(x: *mut &'a i32, y: *mut &'b i32) {
   |                --  -- lifetime `'b` defined here
   |                |
   |                lifetime `'a` defined here
LL |     x == y;
   |     ^ requires that `'a` must outlive `'b`
   |
   = help: consider adding the following bound: `'a: 'b`
   = note: requirement occurs because of a mutable pointer to &i32
   = note: mutable pointers are invariant over their type parameter
   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

The last three lines are new.

This is accomplished by adding a new struct VarianceDiagInfo, and passing it along through the various relation methods. When relating types that change the variance (e.g. &mut T or *mut T), we pass a more specific VarianceDiagInfo storing information about the cause of the variance change. When an error, we use the VarianceDiagInfo to add additional information to the error message.

This PR doesn't change any variance-related computation or behavior - only diagnostic messages. Therefore, the implementation is quite incomplete - more detailed error messages can be filled in in subsequent PRs.

Limitations:

We only attempt to deal with invariance - since it's at the bottom of the 'variance lattice', our variance will never change again after it becomes invariant. Handling contravariance would be trickier, since we can change between contravariance and covariance multiple times (e.g. fn(fn(&'static u8))). Since contravariance (AFAIK) is only used for function arguments, we can probably get away without a very fancy message for cases involving contravariance.
VarianceDiagInfo currently only handles mutable pointers/references. However, user-defined types (structs, enums, and unions) have the variance of their type parameters inferred, so it would be good to eventually display information about that. We'll want to try to find a balance between displaying too much and too little information about how the variance was inferred.
The improved error messages are only displayed when #![feature(nll)] / -Z borrowck=mir is enabled. If issue NLL: turn off migration mode #58781 is not resolved relatively soon, then we might want to duplicate some of this logic in the 'current' (non-NLL) region/outlives handling code.

Aaron1011 · 2021-05-15T19:19:43Z

@bors try @rust-timer queue

rust-timer · 2021-05-15T19:19:44Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-05-15T19:19:51Z

⌛ Trying commit 0ee4fb25d9e8033977fef46a035e5eca3c832ae5 with merge 76be1e69fe41762f3f733a494530f4ea13eaaa20...

bors · 2021-05-15T20:08:31Z

☀️ Try build successful - checks-actions
Build commit: 76be1e69fe41762f3f733a494530f4ea13eaaa20 (76be1e69fe41762f3f733a494530f4ea13eaaa20)

rust-timer · 2021-05-15T20:08:33Z

Queued 76be1e69fe41762f3f733a494530f4ea13eaaa20 with parent eac3c7c, future comparison URL.

rust-timer · 2021-05-16T04:07:47Z

Finished benchmarking try commit (76be1e69fe41762f3f733a494530f4ea13eaaa20): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

Aaron1011 · 2021-05-17T01:23:24Z

@bors try @rust-timer queue

rust-timer · 2021-05-17T01:23:25Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-05-17T01:23:32Z

⌛ Trying commit 1f59727a4560671bb299267a0beb00b7978ae31b with merge a5e251503d529369b35269793fdeed56b46e8561...

bors · 2021-05-17T02:23:54Z

☀️ Try build successful - checks-actions
Build commit: a5e251503d529369b35269793fdeed56b46e8561 (a5e251503d529369b35269793fdeed56b46e8561)

rust-timer · 2021-05-17T02:23:55Z

Queued a5e251503d529369b35269793fdeed56b46e8561 with parent fe72845, future comparison URL.

rust-timer · 2021-05-17T05:18:09Z

Finished benchmarking try commit (a5e251503d529369b35269793fdeed56b46e8561): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

estebank · 2021-05-20T16:22:56Z

compiler/rustc_mir/src/borrow_check/diagnostics/region_errors.rs

+                    ty
+                ));
+                diag.note("mutable references/pointers are invariant over their type parameter");
+                diag.help("See <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance");


Suggested change

diag.help("See <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance");

diag.help("see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance");

Actually, it might be even better to join this help with the note above. The length is not ideal, but we can rely on the user's cli reflow for text.

I think it makes sense to have this message as a distinct 'footer', which gets applied to all variance-related messages.

estebank · 2021-05-20T16:25:16Z

src/test/ui/nll/type-check-pointer-comparisons.stderr

   = help: consider adding the following bound: `'a: 'b`
+   = note: requirement occurs because of a mutable reference/pointer to &i32
+   = note: mutable references/pointers are invariant over their type parameter
+   = help: See <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance


It seems like we could use the new logic to trigger the existing lifetime bounding help.

I'm not quite sure what you mean - are you saying that we would start emitting the 'consider adding the following bound' messages in new places, based on whether or not we have extra variance info available? We currently only add these extra notes in cases where a region error has already occurred, so I don't think that would currently be possible/useful.

Aaron1011 · 2021-05-30T20:54:36Z

This PR is now ready for review.

r? @estebank

bors · 2021-06-04T07:36:41Z

☔ The latest upstream changes (presumably #85984) made this pull request unmergeable. Please resolve the merge conflicts.

estebank · 2021-06-05T22:01:03Z

@bors r+

bors · 2021-06-06T00:21:21Z

⌛ Testing commit 4844c4de7c2308788e2f1d186bcccd2d0eb3c60e with merge 52598462a66eeefd6c08837602666a94f4e9f421...

bors · 2021-06-06T00:48:26Z

💔 Test failed - checks-actions

Aaron1011 · 2021-06-06T17:38:32Z

@bors r=estebank

bors · 2021-06-06T17:38:33Z

📌 Commit fad2242 has been approved by estebank

bors · 2021-06-06T19:12:11Z

⌛ Testing commit fad2242 with merge 35fff69...

bors · 2021-06-06T21:32:36Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 35fff69 to master...

klensy · 2021-06-07T22:51:09Z

Regressed in wg-grammar bench https://perf.rust-lang.org/compare.html?start=5b638c1d3751b7ab31cac9739add516bdf39e10a&end=35fff69d043b1c0f5c29894e7f4b0da8b039c131&stat=instructions%3Au

Mark-Simulacrum · 2021-06-08T14:30:48Z

This caused a regression in a number of benchmarks in the final merge run, which seems to have been mostly expected based on the PR runs done -- @Aaron1011 did we try to mitigate that? It looks like this is just tracking more information, so presumably the regression is mostly unavoidable?

Looks like the most serious regression came from wg-grammar, which is interesting as the PR run reported a net win on that benchmark. It was ~22 days ago, though, so maybe something changed in this PR or on master since then?

Make `relate_type_and_mut` public rust-lang#85343 improved diagnostics around `Relate` impls but made `relate_type_and_mut` private, which was accessible as `relate` previously. This makes it public so that we can use it on rust-semverver. r? `@Aaron1011`

Make `relate_type_and_mut` public rust-lang#85343 improved diagnostics around `Relate` impls but made `relate_type_and_mut` private, which was accessible as `relate` previously. This makes it public so that we can use it on rust-semverver. r? ``@Aaron1011``

Make `relate_type_and_mut` public rust-lang#85343 improved diagnostics around `Relate` impls but made `relate_type_and_mut` private, which was accessible as `relate` previously. This makes it public so that we can use it on rust-semverver. r? ```@Aaron1011```

Aaron1011 · 2021-06-27T18:27:41Z

I've opened #86670, which addresses some of the performance regressions for this PR.

Make the lifetime accurate which is used in the region constraints part This PR fixes the FIXME about lifetime using in the region constraints part. We cannot write `<'graph, 'tcx, D>` because the definition of `Successors<'0, '1, D>` requires `'1 : '0`. We cannot add bound to `'graph` either because `'graph` is required to be an arbitrary value in the definition of `WithSuccessors` So the most accurate way is to use `<'s, 'tcx, D>`. cc `@Aaron1011` who added this FIXME in rust-lang#85343

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 15, 2021

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 16, 2021

Aaron1011 force-pushed the variance-diag branch from 0ee4fb2 to 1f59727 Compare May 17, 2021 01:23

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 17, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 17, 2021

estebank reviewed May 20, 2021

View reviewed changes

Aaron1011 force-pushed the variance-diag branch from 1f59727 to 4bf62de Compare May 30, 2021 20:37

Aaron1011 changed the title ~~[WIP] Add variance-related information to lifetime error messages~~ Add variance-related information to lifetime error messages May 30, 2021

rust-highfive assigned estebank May 30, 2021

Aaron1011 force-pushed the variance-diag branch from 4bf62de to 4844c4d Compare June 4, 2021 14:43

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 6, 2021

Add variance-related information to lifetime error messages

fad2242

Aaron1011 force-pushed the variance-diag branch from 4844c4d to fad2242 Compare June 6, 2021 17:38

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 6, 2021

bors merged commit 35fff69 into rust-lang:master Jun 6, 2021

rustbot added this to the 1.54.0 milestone Jun 6, 2021

JohnTitor mentioned this pull request Jun 10, 2021

Make relate_type_and_mut public #86189

Merged

JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Jun 14, 2021

Rustup to rust-lang/rust#85343

609b7d2

JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Jun 14, 2021

Rustup to rust-lang/rust#85343

75d8361

SparrowLii mentioned this pull request Apr 22, 2022

Make the lifetime accurate which is used in the region constraints part #96315

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variance-related information to lifetime error messages #85343

Add variance-related information to lifetime error messages #85343

Aaron1011 commented May 15, 2021 •

edited

Loading

Aaron1011 commented May 15, 2021

rust-timer commented May 15, 2021

bors commented May 15, 2021

This comment has been minimized.

bors commented May 15, 2021

rust-timer commented May 15, 2021

rust-timer commented May 16, 2021

Aaron1011 commented May 17, 2021

rust-timer commented May 17, 2021

bors commented May 17, 2021

bors commented May 17, 2021

rust-timer commented May 17, 2021

rust-timer commented May 17, 2021

estebank May 20, 2021

estebank May 20, 2021

Aaron1011 May 30, 2021

estebank May 20, 2021

Aaron1011 May 30, 2021

Aaron1011 commented May 30, 2021

bors commented Jun 4, 2021

estebank commented Jun 5, 2021

bors commented Jun 6, 2021

This comment has been minimized.

bors commented Jun 6, 2021

Aaron1011 commented Jun 6, 2021

bors commented Jun 6, 2021

bors commented Jun 6, 2021

bors commented Jun 6, 2021

klensy commented Jun 7, 2021

Mark-Simulacrum commented Jun 8, 2021

Aaron1011 commented Jun 27, 2021

	diag.help("See <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance");
	diag.help("see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance");

Add variance-related information to lifetime error messages #85343

Add variance-related information to lifetime error messages #85343

Conversation

Aaron1011 commented May 15, 2021 • edited Loading

Aaron1011 commented May 15, 2021

rust-timer commented May 15, 2021

bors commented May 15, 2021

This comment has been minimized.

bors commented May 15, 2021

rust-timer commented May 15, 2021

rust-timer commented May 16, 2021

Aaron1011 commented May 17, 2021

rust-timer commented May 17, 2021

bors commented May 17, 2021

bors commented May 17, 2021

rust-timer commented May 17, 2021

rust-timer commented May 17, 2021

estebank May 20, 2021

Choose a reason for hiding this comment

estebank May 20, 2021

Choose a reason for hiding this comment

Aaron1011 May 30, 2021

Choose a reason for hiding this comment

estebank May 20, 2021

Choose a reason for hiding this comment

Aaron1011 May 30, 2021

Choose a reason for hiding this comment

Aaron1011 commented May 30, 2021

bors commented Jun 4, 2021

estebank commented Jun 5, 2021

bors commented Jun 6, 2021

This comment has been minimized.

bors commented Jun 6, 2021

Aaron1011 commented Jun 6, 2021

bors commented Jun 6, 2021

bors commented Jun 6, 2021

bors commented Jun 6, 2021

klensy commented Jun 7, 2021

Mark-Simulacrum commented Jun 8, 2021

Aaron1011 commented Jun 27, 2021

Aaron1011 commented May 15, 2021 •

edited

Loading