-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Refactorings to borrowck region diagnostic reporting #67241
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #66650) made this pull request unmergeable. Please resolve the merge conflicts. |
9ce51ec
to
9f88192
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
9f88192
to
cd940ea
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ok, so this is looking like it will be one of those PRs that later get split up into smaller PRs... I'm going to leave some notes to self here:
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
That last commit was quite satisfying! Look at all of those arguments that were being passed around solely to generate diagnostics! |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bf0c279
to
65be906
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
65be906
to
e5614d6
Compare
@matthewjasper @eddyb I've rebased and consolidated my commits. The first one I've split out into #67404 The next two contain the meat so far. The remaining todos:
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@matthewjasper @eddyb This is ready, but still blocked on #67404 |
📌 Commit 4dc7793 has been approved by |
🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened |
…=matthewjasper Refactorings to borrowck region diagnostic reporting This PR is a followup to rust-lang#66886 and rust-lang#67404 EDIT: updated In this PR: Clean up how errors are collected from NLL: introduce `borrow_check::diagnostics::RegionErrors` to collect errors. This is later passed to `MirBorrowckCtx::report_region_errors` after the `MirBorrowckCtx` is created. This will allow us to refactor away some of the extra context structs floating around (but we don't do it in this PR). `borrow_check::region_infer` is now mostly free of diagnostic generating code. The signatures of most of the functions in `region_infer` lose somewhere between 4 and 7 arguments :) Left for future (feedback appreciated): - Merge `ErrorRegionCtx` with `MirBorrowckCtx`, as suggested by @matthewjasper in rust-lang#66886 (comment) - Maybe move the contents of `borrow_check::nll` into `borrow_check` and remove the `nll` submodule altogether. - Find a way to make `borrow_check::diagnostics::region_errors` less of a strange appendage to `RegionInferenceContext`. I'm not really sure how to do this yet. Ideas welcome.
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
4dc7793
to
05e3d20
Compare
I squashed and rebased... I have not rerun tests because it takes forever on my laptop, but ./x.py check passes... |
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
@bors r+ |
📌 Commit 05e3d20 has been approved by |
Refactorings to borrowck region diagnostic reporting This PR is a followup to #66886 and #67404 EDIT: updated In this PR: Clean up how errors are collected from NLL: introduce `borrow_check::diagnostics::RegionErrors` to collect errors. This is later passed to `MirBorrowckCtx::report_region_errors` after the `MirBorrowckCtx` is created. This will allow us to refactor away some of the extra context structs floating around (but we don't do it in this PR). `borrow_check::region_infer` is now mostly free of diagnostic generating code. The signatures of most of the functions in `region_infer` lose somewhere between 4 and 7 arguments :) Left for future (feedback appreciated): - Merge `ErrorRegionCtx` with `MirBorrowckCtx`, as suggested by @matthewjasper in #66886 (comment) - Maybe move the contents of `borrow_check::nll` into `borrow_check` and remove the `nll` submodule altogether. - Find a way to make `borrow_check::diagnostics::region_errors` less of a strange appendage to `RegionInferenceContext`. I'm not really sure how to do this yet. Ideas welcome.
☀️ Test successful - checks-azure |
Get rid of ErrorReportingCtx [5/N] We can now use `MirBorrowckCtxt` instead :) ``` 6 files changed, 122 insertions(+), 243 deletions(-) ``` This is a followup to (and thus blocked on) #67241. r? @matthewjasper cc @eddyb I while try to do one more to get rid of the weird usage of `RegionInferenceCtx` in `borrow_check::diagnostics::{region_errors, region_naming}`. I think those uses can possibly also be refactored to use `MirBorrowckCtxt`...
This PR is a followup to #66886 and #67404
EDIT: updated
In this PR: Clean up how errors are collected from NLL: introduce
borrow_check::diagnostics::RegionErrors
to collect errors. This is later passed toMirBorrowckCtx::report_region_errors
after theMirBorrowckCtx
is created. This will allow us to refactor away some of the extra context structs floating around (but we don't do it in this PR).borrow_check::region_infer
is now mostly free of diagnostic generating code. The signatures of most of the functions inregion_infer
lose somewhere between 4 and 7 arguments :)Left for future (feedback appreciated):
ErrorRegionCtx
withMirBorrowckCtx
, as suggested by @matthewjasper in Remove the borrow check::nll submodule #66886 (comment)borrow_check::nll
intoborrow_check
and remove thenll
submodule altogether.borrow_check::diagnostics::region_errors
less of a strange appendage toRegionInferenceContext
. I'm not really sure how to do this yet. Ideas welcome.