-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Refactorings to borrowck region diagnostic reporting #67241
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
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.