Skip to content

Commit

Permalink
Make conflicting borrow description more robust.
Browse files Browse the repository at this point in the history
This commit improves the logic for place descriptions in conflicting
borrow errors so that borrows of union fields have better messages even
when the unions are embedded in other unions or structs.
  • Loading branch information
davidtwco committed Jan 4, 2019
1 parent 69bded2 commit 388dffe
Show file tree
Hide file tree
Showing 3 changed files with 187 additions and 31 deletions.
122 changes: 91 additions & 31 deletions src/librustc_mir/borrow_check/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,16 +329,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
"closure"
};

let (desc_place, msg_place, msg_borrow) = if issued_borrow.borrowed_place == *place {
let desc_place = self.describe_place(place).unwrap_or_else(|| "_".to_owned());
(desc_place, "".to_string(), "".to_string())
} else {
let (desc_place, msg_place) = self.describe_place_for_conflicting_borrow(place);
let (_, msg_borrow) = self.describe_place_for_conflicting_borrow(
&issued_borrow.borrowed_place
);
(desc_place, msg_place, msg_borrow)
};
let (desc_place, msg_place, msg_borrow) = self.describe_place_for_conflicting_borrow(
place, &issued_borrow.borrowed_place,
);
let via = |msg: String| if msg.is_empty() { msg } else { format!(" (via `{}`)", msg) };
let msg_place = via(msg_place);
let msg_borrow = via(msg_borrow);

let explanation = self.explain_why_borrow_contains_point(context, issued_borrow, None);
let second_borrow_desc = if explanation.is_explained() {
Expand Down Expand Up @@ -526,33 +522,97 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
err.buffer(&mut self.errors_buffer);
}

/// Returns a description of a place and an associated message for the purposes of conflicting
/// borrow diagnostics.
/// Returns the description of the root place for a conflicting borrow and the full
/// descriptions of the places that caused the conflict.
///
/// In the simplest case, where there are no unions involved, if a mutable borrow of `x` is
/// attempted while a shared borrow is live, then this function will return:
///
/// ("x", "", "")
///
/// If the borrow is of the field `b` of a union `u`, then the return value will be
/// `("u", " (via \`u.b\`)")`. Otherwise, for some variable `a`, the return value will be
/// `("a", "")`.
/// In the simple union case, if a mutable borrow of a union field `x.z` is attempted while
/// a shared borrow of another field `x.y`, then this function will return:
///
/// ("x", "x.z", "x.y")
///
/// In the more complex union case, where the union is a field of a struct, then if a mutable
/// borrow of a union field in a struct `x.u.z` is attempted while a shared borrow of
/// another field `x.u.y`, then this function will return:
///
/// ("x.u", "x.u.z", "x.u.y")
///
/// This is used when creating error messages like below:
///
/// > cannot borrow `a.u` (via `a.u.z.c`) as immutable because it is also borrowed as
/// > mutable (via `a.u.s.b`) [E0502]
pub(super) fn describe_place_for_conflicting_borrow(
&self,
place: &Place<'tcx>,
) -> (String, String) {
place.base_local()
.filter(|local| {
// Filter out non-unions.
self.mir.local_decls[*local].ty
.ty_adt_def()
.map(|adt| adt.is_union())
.unwrap_or(false)
first_borrowed_place: &Place<'tcx>,
second_borrowed_place: &Place<'tcx>,
) -> (String, String, String) {
// Define a small closure that we can use to check if the type of a place
// is a union.
let is_union = |place: &Place<'tcx>| -> bool {
place.ty(self.mir, self.infcx.tcx)
.to_ty(self.infcx.tcx)
.ty_adt_def()
.map(|adt| adt.is_union())
.unwrap_or(false)
};

// Start with an empty tuple, so we can use the functions on `Option` to reduce some
// code duplication (particularly around returning an empty description in the failure
// case).
Some(())
.filter(|_| {
// If we have a conflicting borrow of the same place, then we don't want to add
// an extraneous "via x.y" to our diagnostics, so filter out this case.
first_borrowed_place != second_borrowed_place
})
.and_then(|local| {
let desc_base = self.describe_place(&Place::Local(local))
.unwrap_or_else(|| "_".to_owned());
let desc_original = self.describe_place(place)
.unwrap_or_else(|| "_".to_owned());
return Some((desc_base, format!(" (via `{}`)", desc_original)));
.and_then(|_| {
// We're going to want to traverse the first borrowed place to see if we can find
// field access to a union. If we find that, then we will keep the place of the
// union being accessed and the field that was being accessed so we can check the
// second borrowed place for the same union and a access to a different field.
let mut current = first_borrowed_place;
while let Place::Projection(box PlaceProjection { base, elem }) = current {
match elem {
ProjectionElem::Field(field, _) if is_union(base) => {
return Some((base, field));
},
_ => current = base,
}
}
None
})
.and_then(|(target_base, target_field)| {
// With the place of a union and a field access into it, we traverse the second
// borrowed place and look for a access to a different field of the same union.
let mut current = second_borrowed_place;
while let Place::Projection(box PlaceProjection { base, elem }) = current {
match elem {
ProjectionElem::Field(field, _) if {
is_union(base) && field != target_field && base == target_base
} => {
let desc_base = self.describe_place(base)
.unwrap_or_else(|| "_".to_owned());
let desc_first = self.describe_place(first_borrowed_place)
.unwrap_or_else(|| "_".to_owned());
let desc_second = self.describe_place(second_borrowed_place)
.unwrap_or_else(|| "_".to_owned());
return Some((desc_base, desc_first, desc_second));
},
_ => current = base,
}
}
None
})
.unwrap_or_else(|| {
(self.describe_place(place).unwrap_or_else(|| "_".to_owned()), "".to_string())
// If we didn't find a field access into a union, or both places match, then
// only return the description of the first place.
let desc_place = self.describe_place(first_borrowed_place)
.unwrap_or_else(|| "_".to_owned());
(desc_place, "".to_string(), "".to_string())
})
}

Expand Down
69 changes: 69 additions & 0 deletions src/test/ui/nll/issue-57100.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
#![allow(unused)]
#![feature(nll)]

// ignore-tidy-linelength

// This tests the error messages for borrows of union fields when the unions are embedded in other
// structs or unions.

#[derive(Clone, Copy, Default)]
struct Leaf {
l1_u8: u8,
l2_u8: u8,
}

#[derive(Clone, Copy)]
union First {
f1_leaf: Leaf,
f2_leaf: Leaf,
f3_union: Second,
}

#[derive(Clone, Copy)]
union Second {
s1_leaf: Leaf,
s2_leaf: Leaf,
}

struct Root {
r1_u8: u8,
r2_union: First,
}

// Borrow a different field of the nested union.
fn nested_union() {
unsafe {
let mut r = Root {
r1_u8: 3,
r2_union: First { f3_union: Second { s2_leaf: Leaf { l1_u8: 8, l2_u8: 4 } } }
};

let mref = &mut r.r2_union.f3_union.s1_leaf.l1_u8;
// ^^^^^^^
*mref = 22;
let nref = &r.r2_union.f3_union.s2_leaf.l1_u8;
// ^^^^^^^
//~^^ ERROR cannot borrow `r.r2_union.f3_union` (via `r.r2_union.f3_union.s2_leaf.l1_u8`) as immutable because it is also borrowed as mutable (via `r.r2_union.f3_union.s1_leaf.l1_u8`) [E0502]
println!("{} {}", mref, nref)
}
}

// Borrow a different field of the first union.
fn first_union() {
unsafe {
let mut r = Root {
r1_u8: 3,
r2_union: First { f3_union: Second { s2_leaf: Leaf { l1_u8: 8, l2_u8: 4 } } }
};

let mref = &mut r.r2_union.f2_leaf.l1_u8;
// ^^^^^^^
*mref = 22;
let nref = &r.r2_union.f1_leaf.l1_u8;
// ^^^^^^^
//~^^ ERROR cannot borrow `r.r2_union` (via `r.r2_union.f1_leaf.l1_u8`) as immutable because it is also borrowed as mutable (via `r.r2_union.f2_leaf.l1_u8`) [E0502]
println!("{} {}", mref, nref)
}
}

fn main() {}
27 changes: 27 additions & 0 deletions src/test/ui/nll/issue-57100.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
error[E0502]: cannot borrow `r.r2_union.f3_union` (via `r.r2_union.f3_union.s2_leaf.l1_u8`) as immutable because it is also borrowed as mutable (via `r.r2_union.f3_union.s1_leaf.l1_u8`)
--> $DIR/issue-57100.rs:44:20
|
LL | let mref = &mut r.r2_union.f3_union.s1_leaf.l1_u8;
| -------------------------------------- mutable borrow occurs here (via `r.r2_union.f3_union.s1_leaf.l1_u8`)
...
LL | let nref = &r.r2_union.f3_union.s2_leaf.l1_u8;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ immutable borrow occurs here (via `r.r2_union.f3_union.s2_leaf.l1_u8`)
...
LL | println!("{} {}", mref, nref)
| ---- mutable borrow later used here

error[E0502]: cannot borrow `r.r2_union` (via `r.r2_union.f1_leaf.l1_u8`) as immutable because it is also borrowed as mutable (via `r.r2_union.f2_leaf.l1_u8`)
--> $DIR/issue-57100.rs:62:20
|
LL | let mref = &mut r.r2_union.f2_leaf.l1_u8;
| ----------------------------- mutable borrow occurs here (via `r.r2_union.f2_leaf.l1_u8`)
...
LL | let nref = &r.r2_union.f1_leaf.l1_u8;
| ^^^^^^^^^^^^^^^^^^^^^^^^^ immutable borrow occurs here (via `r.r2_union.f1_leaf.l1_u8`)
...
LL | println!("{} {}", mref, nref)
| ---- mutable borrow later used here

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0502`.

0 comments on commit 388dffe

Please sign in to comment.