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

Fix borrow checker unsoundness with unions #47689

Merged
merged 1 commit into from Feb 25, 2018

Conversation

Projects
None yet
7 participants
@davidtwco
Member

davidtwco commented Jan 23, 2018

Fixes #45157. After discussion with @nikomatsakis on Gitter, this PR only adds a test since the original issue was resolved elsewhere.

r? @nikomatsakis

@davidtwco davidtwco changed the title from Fix borrow checker unsoundness with unions to WIP: Fix borrow checker unsoundness with unions Jan 23, 2018

@davidtwco

This comment has been minimized.

Member

davidtwco commented Jan 23, 2018

Renamed this to be "WIP" since I've seen it fail some compile-fail errors that I'll need to look into. See PR description, only adding a test now.

let mut u = U { s: Default::default() };
let mref = &mut u.s.a; //~ ERROR cannot assign twice to immutable variable `mref` [E0384]
let err = &u.z.c; //~ ERROR cannot assign twice to immutable variable `err` [E0384]

This comment has been minimized.

@matthewjasper

matthewjasper Jan 24, 2018

Contributor

The code as is should compile with nll. There needs to be a second use of mref to keep the borrow active for the second assignment.

debug!("place_element_conflict: DISJOINT-LOCAL");
Overlap::Disjoint
},
}

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 24, 2018

Contributor

I think this is not quite right. Local variables should always be disjoint from one another. For example, borrowing x while y is borrowed should never be an error, no matter the type of x.

@davidtwco

This comment has been minimized.

Member

davidtwco commented Jan 24, 2018

Going to modify this PR to add a test to handle the case @nikomatsakis mentioned in the issue since whichever problem this issue originally referred to is now being handled.

@davidtwco davidtwco changed the title from WIP: Fix borrow checker unsoundness with unions to Fix borrow checker unsoundness with unions Jan 24, 2018

| ^^^^^^ 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

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 24, 2018

Contributor

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

cc @pnkfelix -- agreed?

This comment has been minimized.

@pnkfelix

pnkfelix Jan 26, 2018

Member

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?

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 26, 2018

Contributor

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"

This comment has been minimized.

@davidtwco

davidtwco Jan 26, 2018

Member

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

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:

(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?

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 26, 2018

Contributor

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

This comment has been minimized.

@davidtwco

davidtwco Jan 26, 2018

Member

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 26, 2018

Contributor

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

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 26, 2018

Contributor

updated your comment =)

This comment has been minimized.

@davidtwco

davidtwco Jan 26, 2018

Member

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.

@kennytm

This comment has been minimized.

Member

kennytm commented Jan 31, 2018

Ping from triage @nikomatsakis!

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Feb 5, 2018

@nikomatsakis ping from triage!

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 6, 2018

So I did a bit of digging here, IIRC, and found that the strange errors arose from two-phase borrows. I did not however get far enough to decide if this was a distinct bug or what.

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Feb 12, 2018

@nikomatsakis and @rust-lang/compiler, can we get a review on this?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 13, 2018

I was still hoping that @pnkfelix would weigh in. But I still hope to carve out some time to dig in to the two-phase borrow logic and try to understand just why it's triggering in the way that it is and how we can improve it.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Feb 16, 2018

@nikomatsakis oh, sorry, I didn't know you were still waiting on further input from me

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 20, 2018

@pnkfelix so I think the problem was that whenever we saw a Gen bit for an activation, we treated that as an activation that needs checking. I think that should only be required for loans that are not already activated. I made that change in the most recent commit. Can you review it?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 21, 2018

hmm, in gitter just now, @pnkfelix and I were saying we had some concern about the possibility of an activation being overlooked due to an activation from a previous loop

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 22, 2018

So it seems like what we need is some way to know when one activation is dominated by another (in which case it can be ignored). Because activations are "unioned" on control-flow join, we don't get that for free. Annoying. We could make a "must be activated" dataflow, but it feels like overkill to me.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 22, 2018

That said, this seems orthogonal from the original PR. Why don't we do this -- I'll peel off my last two commits and we'll open up a separate issue to discuss the extra errors resulting from two-phase borrows. Then we can land this PR, which fixes a legit problem.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 22, 2018

@bors r+ rollup

@bors

This comment has been minimized.

Contributor

bors commented Feb 22, 2018

📌 Commit e6376a1 has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 22, 2018

I opened #48418

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 24, 2018

Rollup merge of rust-lang#47689 - davidtwco:issue-45157, r=nikomatsakis
Fix borrow checker unsoundness with unions

Fixes rust-lang#45157. After discussion with @nikomatsakis on Gitter, this PR only adds a test since the original issue was resolved elsewhere.

r? @nikomatsakis

bors added a commit that referenced this pull request Feb 24, 2018

Auto merge of #48520 - Manishearth:rollup, r=Manishearth
Rollup of 15 pull requests

- Successful merges: #47689, #48110, #48197, #48296, #48386, #48392, #48404, #48415, #48441, #48448, #48452, #48481, #48490, #48499, #48503
- Failed merges:

bors added a commit that referenced this pull request Feb 25, 2018

Auto merge of #48520 - Manishearth:rollup, r=Manishearth
Rollup of 15 pull requests

- Successful merges: #47689, #48110, #48197, #48296, #48386, #48392, #48404, #48415, #48441, #48448, #48452, #48481, #48490, #48499, #48503
- Failed merges:

@bors bors merged commit e6376a1 into rust-lang:master Feb 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidtwco davidtwco deleted the davidtwco:issue-45157 branch Feb 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment