Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add variance-related information to lifetime error messages #85343

Merged
merged 1 commit into from
Jun 6, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented May 15, 2021

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
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

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

bors commented May 15, 2021

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

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 15, 2021

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

@rust-timer
Copy link
Collaborator

Queued 76be1e69fe41762f3f733a494530f4ea13eaaa20 with parent eac3c7c, future comparison URL.

@rust-timer
Copy link
Collaborator

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

@rustbot 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
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

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

bors commented May 17, 2021

⌛ Trying commit 1f59727a4560671bb299267a0beb00b7978ae31b with merge a5e251503d529369b35269793fdeed56b46e8561...

@bors
Copy link
Contributor

bors commented May 17, 2021

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

@rust-timer
Copy link
Collaborator

Queued a5e251503d529369b35269793fdeed56b46e8561 with parent fe72845, future comparison URL.

@rust-timer
Copy link
Collaborator

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 17, 2021
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 11 to 14
= 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Aaron1011 changed the title [WIP] Add variance-related information to lifetime error messages Add variance-related information to lifetime error messages May 30, 2021
@Aaron1011
Copy link
Member Author

This PR is now ready for review.

r? @estebank

@bors
Copy link
Contributor

bors commented Jun 4, 2021

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

@estebank
Copy link
Contributor

estebank commented Jun 5, 2021

@bors r+

@bors 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
@bors
Copy link
Contributor

bors commented Jun 6, 2021

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

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 6, 2021

💔 Test failed - checks-actions

@bors 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
@Aaron1011
Copy link
Member Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jun 6, 2021

📌 Commit fad2242 has been approved by estebank

@bors 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
Copy link
Contributor

bors commented Jun 6, 2021

⌛ Testing commit fad2242 with merge 35fff69...

@bors
Copy link
Contributor

bors commented Jun 6, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 6, 2021
@bors bors merged commit 35fff69 into rust-lang:master Jun 6, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 6, 2021
@Mark-Simulacrum
Copy link
Member

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?

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 11, 2021
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`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 11, 2021
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``
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 11, 2021
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```
JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Jun 14, 2021
JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Jun 14, 2021
@Aaron1011
Copy link
Member Author

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 22, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants