Skip to content
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

Fix borrow checker unsoundness with unions #47689

Merged
merged 1 commit into from
Feb 25, 2018
Merged

Fix borrow checker unsoundness with unions #47689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 43 additions & 0 deletions src/test/ui/issue-45157.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// 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.

#![allow(unused)]
#![feature(nll)]

#[derive(Clone, Copy, Default)]
struct S {
a: u8,
b: u8,
}
#[derive(Clone, Copy, Default)]
struct Z {
c: u8,
d: u8,
}

union U {
s: S,
z: Z,
}

fn main() {
unsafe {
let mut u = U { s: Default::default() };

let mref = &mut u.s.a;
*mref = 22;

let nref = &u.z.c;
//~^ ERROR cannot borrow `u.z.c` as immutable because it is also borrowed as mutable [E0502]
println!("{} {}", mref, nref)
//~^ ERROR cannot borrow `u.s.a` as mutable because it is also borrowed as immutable [E0502]
}
}

20 changes: 20 additions & 0 deletions src/test/ui/issue-45157.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
error[E0502]: cannot borrow `u.z.c` as immutable because it is also borrowed as mutable
--> $DIR/issue-45157.rs:37:20
|
34 | let mref = &mut u.s.a;
| ---------- mutable borrow occurs here
...
37 | let nref = &u.z.c;
| ^^^^^^ immutable borrow occurs here

error[E0502]: cannot borrow `u.s.a` as mutable because it is also borrowed as immutable
--> $DIR/issue-45157.rs:39:27
Copy link
Contributor

Choose a reason for hiding this comment

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

This second error is a bit surprising. I don't quite understand what it is saying, it looks a bit fishy.

cc @pnkfelix -- agreed?

Copy link
Member

Choose a reason for hiding this comment

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

Are you saying that there should be no error here at all? Or just that the error should be focused on prefixes of the field projections it is currently highlighting?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think there should be an error at all, and certainly not with these spans. Usually something like this:

let mut a = 2;
let mref = &mut a;
let nref = &a;
println("{}{}", mref, nref);

gives only one error, right?

i.e., one borrow comes first, and it "wins"

Copy link
Contributor

Choose a reason for hiding this comment

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

Example: https://play.rust-lang.org/?gist=695ae34722e06a4060c38df82a45c92d&version=nightly

Copy link
Member Author

Choose a reason for hiding this comment

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

So, looking into this a little bit, here is what I've figured out:

rust/src/librustc_mir/util/borrowck_errors.rs

Lines 225 to 247 in fd0f292

fn cannot_reborrow_already_borrowed(&self,
span: Span,
desc_new: &str,
msg_new: &str,
kind_new: &str,
old_span: Span,
noun_old: &str,
kind_old: &str,
msg_old: &str,
old_load_end_span: Option<Span>,
o: Origin)
-> DiagnosticBuilder
{
let mut err = struct_span_err!(self, span, E0502,
"cannot borrow `{}`{} as {} because {} is also borrowed as {}{}{OGN}",
desc_new, msg_new, kind_new, noun_old, kind_old, msg_old, OGN=o);
err.span_label(span, format!("{} borrow occurs here{}", kind_new, msg_new));
err.span_label(old_span, format!("{} borrow occurs here{}", kind_old, msg_old));
if let Some(old_load_end_span) = old_load_end_span {
err.span_label(old_load_end_span, format!("{} borrow ends here", kind_old));
}
self.cancel_if_wrong_origin(err, o)
}

The error is reported in the above function. That is called by the following function:

rust/src/librustc_mir/borrow_check/error_reporting.rs

Lines 259 to 272 in fdecb05

(BorrowKind::Shared, lft, _, BorrowKind::Mut, _, rgt) |
(BorrowKind::Mut, _, lft, BorrowKind::Shared, rgt, _) => self.tcx
.cannot_reborrow_already_borrowed(
span,
&desc_place,
"",
lft,
issued_span,
"it",
rgt,
"",
end_issued_loan_span,
Origin::Mir,
),

This is quite similar to the work done for #47607, in that PR, I added a set that contains the place/span of any errors reported so that they aren't reported again. In this case, the span on line 37 on the below error (that we want) would be in this set:

error[E0502]: cannot borrow `u.z.c` as immutable because it is also borrowed as mutable
  --> src/main.rs:37:20
   |
34 |         let mref = &mut u.s.a; //~ ERROR cannot assign twice to immutable variable `mref` [E0384]
   |                    ---------- mutable borrow occurs here
...
37 |         let nref = &u.z.c; //~ ERROR cannot assign twice to immutable variable `err` [E0384]
   |                    ^^^^^^ immutable borrow occurs here

I'm not entirely sure what the unintended side effects of the following might be, but we could check if the issued_span is in the set (in the above example, that is the error on line 34, but in the second unintended error, that would refer to the same location as above on line 37) and if it is, skip this error. It would essentially silence errors that overlap where the second borrow location of the first error is the first borrow location in subsequent errors. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidtwco do you think you could gist the output from -Zdump-mir=nll for this function?

Copy link
Member Author

@davidtwco davidtwco Jan 26, 2018 •
edited by nikomatsakis
Loading

Choose a reason for hiding this comment

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

@nikomatsakis is this what you're looking for?

https://gist.github.com/nikomatsakis/b0ac3440933b3ae1d4dc3db02d738111

Copy link
Contributor

Choose a reason for hiding this comment

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

yep but I was hoping for a gist :) kind of hard to digest inline...

Copy link
Contributor

Choose a reason for hiding this comment

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

updated your comment =)

Copy link
Member Author

@davidtwco davidtwco Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Here is a gist with some logging added in access_place and check_access_for_conflict that should show the variable values. Lines 13730 to 13759 for the first error and lines 13912 to 13959 for the second error.

|
37 | let nref = &u.z.c;
| ------ immutable borrow occurs here
38 | //~^ ERROR cannot borrow `u.z.c` as immutable because it is also borrowed as mutable [E0502]
39 | println!("{} {}", mref, nref)
| ^^^^ mutable borrow occurs here

error: aborting due to 2 previous errors