Skip to content

NLL: Add union justifications to conflicting borrows. #57102

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 3 commits into from
Jan 13, 2019
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
8 changes: 2 additions & 6 deletions src/librustc_borrowck/borrowck/check_loans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,12 +557,8 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
if new_loan.loan_path.has_fork(&old_loan.loan_path) && common.is_some() {
let nl = self.bccx.loan_path_to_string(&common.unwrap());
let ol = nl.clone();
let new_loan_msg = format!(" (via `{}`)",
self.bccx.loan_path_to_string(
&new_loan.loan_path));
let old_loan_msg = format!(" (via `{}`)",
self.bccx.loan_path_to_string(
&old_loan.loan_path));
let new_loan_msg = self.bccx.loan_path_to_string(&new_loan.loan_path);
let old_loan_msg = self.bccx.loan_path_to_string(&old_loan.loan_path);
(nl, ol, new_loan_msg, old_loan_msg)
} else {
(self.bccx.loan_path_to_string(&new_loan.loan_path),
Expand Down
126 changes: 116 additions & 10 deletions src/librustc_mir/borrow_check/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,10 +329,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
"closure"
};

let desc_place = self.describe_place(place).unwrap_or_else(|| "_".to_owned());
let tcx = self.infcx.tcx;

let first_borrow_desc;
let (desc_place, msg_place, msg_borrow, union_type_name) =
self.describe_place_for_conflicting_borrow(place, &issued_borrow.borrowed_place);

let explanation = self.explain_why_borrow_contains_point(context, issued_borrow, None);
let second_borrow_desc = if explanation.is_explained() {
Expand All @@ -342,6 +340,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
};

// FIXME: supply non-"" `opt_via` when appropriate
let tcx = self.infcx.tcx;
let first_borrow_desc;
let mut err = match (
gen_borrow_kind,
"immutable",
Expand All @@ -355,12 +355,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
tcx.cannot_reborrow_already_borrowed(
span,
&desc_place,
"",
&msg_place,
lft,
issued_span,
"it",
rgt,
"",
&msg_borrow,
None,
Origin::Mir,
)
Expand All @@ -370,12 +370,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
tcx.cannot_reborrow_already_borrowed(
span,
&desc_place,
"",
&msg_place,
lft,
issued_span,
"it",
rgt,
"",
&msg_borrow,
None,
Origin::Mir,
)
Expand All @@ -386,9 +386,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
tcx.cannot_mutably_borrow_multiply(
span,
&desc_place,
"",
&msg_place,
issued_span,
"",
&msg_borrow,
None,
Origin::Mir,
)
Expand Down Expand Up @@ -512,12 +512,118 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
);
}

if union_type_name != "" {
err.note(&format!(
"`{}` is a field of the union `{}`, so it overlaps the field `{}`",
msg_place, union_type_name, msg_borrow,
));
}

explanation
.add_explanation_to_diagnostic(self.infcx.tcx, self.mir, &mut err, first_borrow_desc);

err.buffer(&mut self.errors_buffer);
}

/// 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", "", "")
///
/// 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,
first_borrowed_place: &Place<'tcx>,
second_borrowed_place: &Place<'tcx>,
) -> (String, 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(|_| {
// 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());

// Also compute the name of the union type, eg. `Foo` so we
// can add a helpful note with it.
let ty = base.ty(self.mir, self.infcx.tcx).to_ty(self.infcx.tcx);

return Some((desc_base, desc_first, desc_second, ty.to_string()));
},
_ => current = base,
}
}
None
})
.unwrap_or_else(|| {
// 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(), "".to_string())
})
}

/// Reports StorageDeadOrDrop of `place` conflicts with `borrow`.
///
/// This means that some data referenced by `borrow` needs to live
Expand Down
43 changes: 32 additions & 11 deletions src/librustc_mir/util/borrowck_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,15 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
old_load_end_span: Option<Span>,
o: Origin,
) -> DiagnosticBuilder<'cx> {
let via = |msg: &str|
if msg.is_empty() { msg.to_string() } else { format!(" (via `{}`)", msg) };
let mut err = struct_span_err!(
self,
new_loan_span,
E0499,
"cannot borrow `{}`{} as mutable more than once at a time{OGN}",
desc,
opt_via,
via(opt_via),
OGN = o
);
if old_loan_span == new_loan_span {
Expand All @@ -164,11 +166,11 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
} else {
err.span_label(
old_loan_span,
format!("first mutable borrow occurs here{}", old_opt_via),
format!("first mutable borrow occurs here{}", via(old_opt_via)),
);
err.span_label(
new_loan_span,
format!("second mutable borrow occurs here{}", opt_via),
format!("second mutable borrow occurs here{}", via(opt_via)),
);
if let Some(old_load_end_span) = old_load_end_span {
err.span_label(old_load_end_span, "first borrow ends here");
Expand Down Expand Up @@ -292,27 +294,46 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
old_load_end_span: Option<Span>,
o: Origin,
) -> DiagnosticBuilder<'cx> {
let via = |msg: &str|
if msg.is_empty() { msg.to_string() } else { format!(" (via `{}`)", msg) };
let mut err = struct_span_err!(
self,
span,
E0502,
"cannot borrow `{}`{} as {} because {} is also borrowed as {}{}{OGN}",
"cannot borrow `{}`{} as {} because {} is also borrowed \
as {}{}{OGN}",
desc_new,
msg_new,
via(msg_new),
kind_new,
noun_old,
kind_old,
msg_old,
via(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 msg_new == "" {
// If `msg_new` is empty, then this isn't a borrow of a union field.
err.span_label(span, format!("{} borrow occurs here", kind_new));
err.span_label(old_span, format!("{} borrow occurs here", kind_old));
} else {
// If `msg_new` isn't empty, then this a borrow of a union field.
err.span_label(
span,
format!(
"{} borrow of `{}` -- which overlaps with `{}` -- occurs here",
kind_new, msg_new, msg_old,
)
);
err.span_label(
old_span,
format!("{} borrow occurs here{}", kind_old, via(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)
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/borrowck/borrowck-borrow-from-owned-ptr.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ error[E0502]: cannot borrow `foo` (via `foo.bar2`) as immutable because `foo` is
LL | let bar1 = &mut foo.bar1;
| -------- mutable borrow occurs here (via `foo.bar1`)
LL | let _foo1 = &foo.bar2; //~ ERROR cannot borrow
| ^^^^^^^^ immutable borrow occurs here (via `foo.bar2`)
| ^^^^^^^^ immutable borrow of `foo.bar2` -- which overlaps with `foo.bar1` -- occurs here
LL | *bar1;
LL | }
| - mutable borrow ends here
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/borrowck/borrowck-box-insensitivity.ast.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ error[E0502]: cannot borrow `a` (via `a.y`) as immutable because `a` is also bor
LL | let _x = &mut a.x;
| --- mutable borrow occurs here (via `a.x`)
LL | let _y = &a.y; //[ast]~ ERROR cannot borrow
| ^^^ immutable borrow occurs here (via `a.y`)
| ^^^ immutable borrow of `a.y` -- which overlaps with `a.x` -- occurs here
...
LL | }
| - mutable borrow ends here
Expand All @@ -72,7 +72,7 @@ error[E0502]: cannot borrow `a` (via `a.y`) as mutable because `a` is also borro
LL | let _x = &a.x;
| --- immutable borrow occurs here (via `a.x`)
LL | let _y = &mut a.y; //[ast]~ ERROR cannot borrow
| ^^^ mutable borrow occurs here (via `a.y`)
| ^^^ mutable borrow of `a.y` -- which overlaps with `a.x` -- occurs here
...
LL | }
| - immutable borrow ends here
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/borrowck/borrowck-box-insensitivity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,14 @@ fn borrow_after_mut_borrow() {
let mut a: Box<_> = box A { x: box 0, y: 1 };
let _x = &mut a.x;
let _y = &a.y; //[ast]~ ERROR cannot borrow
//[ast]~^ immutable borrow occurs here (via `a.y`)
//[ast]~^ immutable borrow of `a.y` -- which overlaps with `a.x` -- occurs here
use_mut(_x);
}
fn mut_borrow_after_borrow() {
let mut a: Box<_> = box A { x: box 0, y: 1 };
let _x = &a.x;
let _y = &mut a.y; //[ast]~ ERROR cannot borrow
//[ast]~^ mutable borrow occurs here (via `a.y`)
//[ast]~^ mutable borrow of `a.y` -- which overlaps with `a.x` -- occurs here
use_imm(_x);
}
fn copy_after_move_nested() {
Expand Down
Loading