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.

Sign up for GitHub

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

Fix duplicate display of error E0502 #45603

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5086657
Fix duplicate display of error E0502
Oct 28, 2017
975ff7b
Add test for fix duplicate display of E0502
Oct 29, 2017
014e610
Refactor matches to use Result::err
Oct 29, 2017
37f2382
Refactor if block to use idiomatic matches
Oct 29, 2017
cf10bcf
Move issue-42106 test from compile-fail to ui
Oct 29, 2017
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
32 changes: 23 additions & 9 deletions src/librustc_borrowck/borrowck/check_loans.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,18 +395,33 @@ 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 •
edited
Loading

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 •
edited
Loading

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 = Result::err(self.report_error_if_loan_conflicts_with_restriction(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of Result::err you can use the .err() method on the result produced by the method.

old_loan, new_loan, old_loan, new_loan
));
let err_new_old = Result::err(self.report_error_if_loan_conflicts_with_restriction(
new_loan, old_loan, old_loan, new_loan
));

if let Some(mut err_old) = err_old_new {
err_old.emit();
if let Some(mut err_new) = err_new_old {
err_new.cancel();
}
} else if let Some(mut err_new) = err_new_old {
err_new.emit();
} else {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


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 +431,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 +535,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/compile-fail/issue-42106.rs
View file
Open in desktop
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); //~ ERROR E0502
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a ui test instead?

}

fn main() {}