Skip to content

Fix duplicate display of error E0502 #45603

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

Merged
merged 5 commits into from
Oct 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions src/librustc_borrowck/borrowck/check_loans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

@AlexEne AlexEne Oct 31, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@AlexEne AlexEne Nov 1, 2017

Choose a reason for hiding this comment

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

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"

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 = self.report_error_if_loan_conflicts_with_restriction(
old_loan, new_loan, old_loan, new_loan).err();
let err_new_old = self.report_error_if_loan_conflicts_with_restriction(
new_loan, old_loan, old_loan, new_loan).err();

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,
}

false
}

pub fn report_error_if_loan_conflicts_with_restriction(&self,
loan1: &Loan<'tcx>,
loan2: &Loan<'tcx>,
old_loan: &Loan<'tcx>,
new_loan: &Loan<'tcx>)
-> bool {
-> Result<(), DiagnosticBuilder<'a>> {
//! Checks whether the restrictions introduced by `loan1` would
//! prohibit `loan2`. Returns false if an error is reported.

Expand All @@ -416,7 +427,7 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
loan2);

if compatible_borrow_kinds(loan1.kind, loan2.kind) {
return true;
return Ok(());
}

let loan2_base_path = owned_ptr_base_path_rc(&loan2.loan_path);
Expand Down Expand Up @@ -520,11 +531,10 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
_ => { }
}

err.emit();
return false;
return Err(err);
}

true
Ok(())
}

fn consume_common(&self,
Expand Down
16 changes: 16 additions & 0 deletions src/test/ui/issue-42106.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn do_something<T>(collection: &mut Vec<T>) {
let _a = &collection;
collection.swap(1, 2);
}

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/issue-42106.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
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);
| ^^^^^^^^^^ mutable borrow occurs here
14 | }
| - immutable borrow ends here

error: aborting due to 2 previous errors