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 outlives suggestions for some lifetime errors #58281

Merged
merged 5 commits into from
Nov 19, 2019

Conversation

mark-i-m
Copy link
Member

@mark-i-m mark-i-m commented Feb 8, 2019

This PR implements suggestion diagnostics for some lifetime mismatch errors. When the borrow checker finds that some lifetime 'a doesn't outlive some other lifetime 'b that it should outlive, then in addition to the current lifetime error, we also emit a suggestion for how to fix the problem by adding a bound:

  • If a and b are normal named regions, suggest to add the bound 'a: 'b
  • If b is static, suggest to replace a with static
  • If b also needs to outlive a, they must be the same, so suggest unifying them

We start with a simpler implementation that avoids diagnostic regression or implementation complexity:

  • We only makes suggestions for lifetimes the user can already name (eg not closure regions or elided regions)
  • For now, we only emit a help note, not an actually suggestion because it is significantly easier.

Finally, there is one hack: it seems that implicit regions in async fn are given the name '_ incorrectly. To avoid suggesting '_: 'x, we simply filter out such lifetimes by name.

For more info, see this internals thread:

https://internals.rust-lang.org/t/mechanical-suggestions-for-some-borrow-checker-errors/9049/3

TL;DR Make suggestions to add a where 'a: 'b constraint for some lifetime errors. Details are in the paper linked from the internals thread above.

r? @estebank

TODO

  • Clean up code
  • Only make idiomatic suggestions
    • don't suggest naming &'a self
    • rather than 'a: 'static, suggest replacing 'a with 'static
    • rather than 'a: 'b, 'b: 'a, suggest replacing 'a with 'b or vice versa
  • Performance (maybe need a perf run when this is closer to the finish line?)
    • perf run was clean...
    • EDIT: perf run seems to only check non-error performance... How do we check that error performance didn't regress?
  • Needs ui tests
  • Integrate the help message into the main lifetime error

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2019
@mark-i-m
Copy link
Member Author

mark-i-m commented Feb 8, 2019

cc @lorisdanto

@mark-i-m mark-i-m force-pushed the synthesis branch 2 times, most recently from 5e3e44a to 3614f29 Compare February 8, 2019 03:26
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@mark-i-m
Copy link
Member Author

mark-i-m commented Feb 12, 2019 via email

@mark-i-m mark-i-m force-pushed the synthesis branch 2 times, most recently from 6f864bc to da8e5de Compare February 12, 2019 18:28
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@mark-i-m
Copy link
Member Author

So for now, I'm just going to simplify this:

  • Instead of generating a suggestion, we will just emit a help note. (i.e. not machine applicable).
  • We will use the 'N notation of the other lifetime errors, rather than generating a new name.

My thought is that this will be a lot easier to implement and still useful.

@estebank What do you think?

Also do you know how I can check if a lifetime is 'self or not? This is needed to prevent suggestions that name the self lifetime.

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 3, 2019

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

@mark-i-m
Copy link
Member Author

mark-i-m commented Oct 10, 2019

@estebank Ok, I think this is finally ready for merging.

I added a hacky (but effective) workaround: we just check if the printed name would be equal to '_ and filter out those suggestions (commit 4908618 ). This really only affects async tests with nll compare mode.

The problem still seems to be that somewhere an anon region is being synthesized improperly, but that shows up in other diagnostics already with nll compare mode and async.

@wirelessringo
Copy link

Ping from triage. @estebank any updates on this? Thanks.

@JohnCSimon
Copy link
Member

Pinging again from triage.
@estebank any updates on this?
CC: @mark-i-m
Thanks.

@bors
Copy link
Contributor

bors commented Oct 26, 2019

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

@mark-i-m
Copy link
Member Author

rebased

@JohnCSimon
Copy link
Member

Ping from triage
@estebank can you please review his PR?
cc: @mark-i-m

Thanks

@JohnCSimon
Copy link
Member

Pinging again from triage...
@estebank can you please review his PR?
cc: @mark-i-m

Thanks

@mark-i-m
Copy link
Member Author

Hi @estebank! Thanks for your help so far. Will you have a chance to look at this soon? It has been waiting for review for over a month now, and I am afraid it will bitrot again. Is there anything I can do to expedite the process?

@JohnCSimon
Copy link
Member

Raised this issue on Discord triage

} else {
debug!("Region {:?} is NOT suggestable", name);
false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be just name.name().with(|name| name != "'_").

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I mostly did that for debugging statements

@estebank
Copy link
Contributor

Let's merge as is, but ideally we'd point at the appropriate place for the lifetime restrictions.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 18, 2019

📌 Commit cba0761 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 Nov 18, 2019
@estebank
Copy link
Contributor

estebank commented Nov 18, 2019

Apologies for the long delay. It is a relatively big PR and there are a couple of things I'd like to improve on it, not to mention the general lack of free time on my side :).

Now that the new beta has been cut, I feel more comfortable with approving and further improving on it directly on master. @mark-i-m would you be open to helping me doing that? The good news is that the follow up PRs should be much smaller in size because the bulk of the work is done in this one.

@mark-i-m
Copy link
Member Author

@estebank Thanks for your time, I realize that this is a large pr. The next couple of weeks are a bit hectic for me, but I'm absolutely interested in improving.

@bors
Copy link
Contributor

bors commented Nov 18, 2019

⌛ Testing commit cba0761 with merge 0ccee30...

bors added a commit that referenced this pull request Nov 18, 2019
Add outlives suggestions for some lifetime errors

This PR implements suggestion diagnostics for some lifetime mismatch errors. When the borrow checker finds that some lifetime 'a doesn't outlive some other lifetime 'b that it should outlive, then in addition to the current lifetime error, we also emit a suggestion for how to fix the problem by adding a bound:

- If a and b are normal named regions, suggest to add the bound `'a: 'b`
- If b is static, suggest to replace a with static
- If b also needs to outlive a, they must be the same, so suggest unifying  them

We start with a simpler implementation that avoids diagnostic regression or implementation complexity:
- We only makes suggestions for lifetimes the user can already name (eg not closure regions or elided regions)
- For now, we only emit a help note, not an actually suggestion because it is significantly easier.

Finally, there is one hack: it seems that implicit regions in async fn are given the name '_ incorrectly. To avoid suggesting '_: 'x, we simply filter out such lifetimes by name.

For more info, see this internals thread:

https://internals.rust-lang.org/t/mechanical-suggestions-for-some-borrow-checker-errors/9049/3

TL;DR Make suggestions to add a `where 'a: 'b` constraint for some lifetime errors. Details are in the paper linked from the internals thread above.

r? @estebank

TODO
- [x] Clean up code
- [x] Only make idiomatic suggestions
     - [x] don't suggest naming `&'a self`
     - [x] rather than `'a: 'static`, suggest replacing `'a` with `'static`
     - [x] rather than `'a: 'b, 'b: 'a`, suggest replacing `'a` with `'b` or vice versa
- [x] Performance (maybe need a perf run when this is closer to the finish line?)
     - perf run was clean...
     - EDIT: perf run seems to only check non-error performance... How do we check that error performance didn't regress?
- [x] Needs ui tests
- [x] Integrate the `help` message into the main lifetime `error`
@bors
Copy link
Contributor

bors commented Nov 19, 2019

☀️ Test successful - checks-azure
Approved by: estebank
Pushing 0ccee30 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2019
@bors bors merged commit cba0761 into rust-lang:master Nov 19, 2019
@mark-i-m mark-i-m deleted the synthesis branch December 11, 2019 21:35
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.