Fix duplicate display of error E0502 #45603
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
kennytm
added
the
S-waiting-on-review
| old_loan, new_loan, old_loan, new_loan) && | ||
| self.report_error_if_loan_conflicts_with_restriction( | ||
| new_loan, old_loan, old_loan, new_loan) | ||
| let err_old_new = match self.report_error_if_loan_conflicts_with_restriction( |
There was a problem hiding this comment.
These matches can be changed to call Result::err instead.
| err_new.emit(); | ||
| } else { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This block of code can be made slightly more idiomatic in the following way:
match (err_old_new, err_new_old) {
(Some(mut err), None) | (None, Some(mut err)) => err.emit(),
(Some(mut err_old), Some(mut err_new)) => {
err_old.emit();
err_new.cancel();
}
(None, None) => return true,
}This way the compiler will always check that all paths have been taken care of, and you can handle cases that are mutually exclusive but that require the same code only once.
| old_loan, new_loan, old_loan, new_loan) && | ||
| self.report_error_if_loan_conflicts_with_restriction( | ||
| new_loan, old_loan, old_loan, new_loan) | ||
| let err_old_new = Result::err(self.report_error_if_loan_conflicts_with_restriction( |
There was a problem hiding this comment.
Instead of Result::err you can use the .err() method on the result produced by the method.
src/test/compile-fail/issue-42106.rs
Outdated
|
|
||
| fn do_something<T>(collection: &mut Vec<T>) { | ||
| let _a = &collection; | ||
| collection.swap(1, 2); //~ ERROR E0502 |
There was a problem hiding this comment.
Could this be a ui test instead?
|
@bors r+ |
|
📌 Commit cf10bcf has been approved by |
Fix duplicate display of error E0502 Ref. Repeated "mutable/immutable borrow" error messages #42106. This PR modifies the return type of [`report_error_if_loan_conflicts_with_restriction`](https://github.com/rust-lang/rust/blob/0f0f5db465de96b6c12e71f0c7d3e475f618b104/src/librustc_borrowck/borrowck/check_loans.rs#L398-L403) so the result can be checked in [`report_error_if_loans_conflict`](https://github.com/rust-lang/rust/blob/0f0f5db465de96b6c12e71f0c7d3e475f618b104/src/librustc_borrowck/borrowck/check_loans.rs#L377-L396). This is done to prevent displaying a duplicate of the error message E0502 which is referenced in #42106. The output of compiling: ```rust fn do_something<T>(collection: &mut Vec<T>) { let _a = &collection; collection.swap(1, 2); } fn main() {} ``` is now ```bash $ rustc src/test/compile-fail/issue-42106.rs error[E0502]: cannot borrow `*collection` as mutable because `collection` is also borrowed as immutable --> src/test/compile-fail/issue-42106.rs:13:5 | 12 | let _a = &collection; | ---------- immutable borrow occurs here 13 | collection.swap(1, 2); | ^^^^^^^^^^ mutable borrow occurs here 14 | } | - immutable borrow ends here error: aborting due to 2 previous errors ``` r? @estebank
kennytm
added
S-waiting-on-bors
|
☀️ Test successful - status-appveyor, status-travis |
| @@ -395,18 +395,29 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { | |||
| assert!(self.bccx.region_scope_tree.scopes_intersect(old_loan.kill_scope, | |||
| new_loan.kill_scope)); | |||
|
|
|||
| self.report_error_if_loan_conflicts_with_restriction( | |||
There was a problem hiding this comment.
I am a bit confused on why this actually fixes the error. Maybe this is not the right place to discuss since this was approved already, but shouldn't the old version of report_error_if_loan_conflicts_with_restriction return false if an error is emitted, and thus short-circuiting this && statement (so it is not called the second time)?
Maybe I am missing something here.
p.s. Also why does the the test mention "error: aborting due to 2 previous errors" ? Shouldn't there be only one error?
There was a problem hiding this comment.
Maybe this is not the right place to discuss since this was approved already, but shouldn't the old version of report_error_if_loan_conflicts_with_restriction return false if an error is emitted, and thus short-circuiting this if statement (so it is not called the second time)?
You're right, I believe that was the original intent. I am not sure why short-circuiting wasn't happening.
p.s. Also why does the the test mention "error: aborting due to 2 previous errors" ? Shouldn't there be only one error?
I think that is because of the cancelled diagnostic, I didn't want to block this PR on that as it is a separate issue. Haven't checked wether there's an issue already, but if not I'll create one.
There was a problem hiding this comment.
The short-circuiting actually does happen as expected on the current compiler master branch, I am on 9c87aca. I just built it, ran the test from this PR and it passed (without any other changes from this PR). I even manually double-checked the output just to make sure it was executed. btw, tested only --stage 1.
Also the count is still wrong, even with the original && I still get "... 2 previous errors"
Only bump error count when we are sure that the diagnostic is not a repetition This ensures that if we emit the same diagnostic twice, the error count will match the real number of errors shown to the user. Fixes #42106 This is a followup of #45603 as stated in #42106 (comment). This program, for example: ```rust fn do_something<T>(collection: &mut Vec<T>) { let _a = &collection; collection.swap(1, 2); } fn main() {} ``` without this patch, produces: ``` error[E0502]: cannot borrow `*collection` as mutable because `collection` is also borrowed as immutable --> $DIR/issue-42106.rs:13:5 | 12 | let _a = &collection; | ---------- immutable borrow occurs here 13 | collection.swap(1, 2); //~ ERROR also borrowed as immutable | ^^^^^^^^^^ mutable borrow occurs here 14 | } | - immutable borrow ends here error: aborting due to 2 previous errors ``` The number of errors do not match the diagnostics reported. This PR fixes this problem. The output is now in this case: ``` error[E0502]: cannot borrow `*collection` as mutable because `collection` is also borrowed as immutable --> $DIR/issue-42106.rs:13:5 | 12 | let _a = &collection; | ---------- immutable borrow occurs here 13 | collection.swap(1, 2); //~ ERROR also borrowed as immutable | ^^^^^^^^^^ mutable borrow occurs here 14 | } | - immutable borrow ends here error: aborting due to previous error ``` Also, some other tests outputs have been adapted because their count didn't really match the number of diagnostics reported. As an aside, an outdated comment has been removed (`Handler::cancel` will only call to the `Diagnostic::cancel` method and will not decrease the count of errors). All tests are passing with this PR (`x.py test` is successful).
Ref. Repeated "mutable/immutable borrow" error messages #42106.
This PR modifies the return type of
report_error_if_loan_conflicts_with_restrictionso the result can be checked inreport_error_if_loans_conflict. This is done to prevent displaying a duplicate of the error message E0502 which is referenced in #42106.The output of compiling:
is now
r? @estebank