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

Jump to bottom

borrowck: prefer "value" over "_" in diagnostics #70389

Merged
merged 2 commits into from
Mar 26, 2020
Merged

borrowck: prefer "value" over "_" in diagnostics #70389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c70aa34
borrowck: prefer "value" over "`_`".
Centril Mar 25, 2020
632c0af
borrowck diagnostics: address review comments.
Centril Mar 26, 2020
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
89 changes: 31 additions & 58 deletions src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,25 +256,17 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
"report_move_out_while_borrowed: location={:?} place={:?} span={:?} borrow={:?}",
location, place, span, borrow
);
let value_msg = match self.describe_place(place.as_ref()) {
Some(name) => format!("`{}`", name),
None => "value".to_owned(),
};
let borrow_msg = match self.describe_place(borrow.borrowed_place.as_ref()) {
Some(name) => format!("`{}`", name),
None => "value".to_owned(),
};
let value_msg = self.describe_any_place(place.as_ref());
let borrow_msg = self.describe_any_place(borrow.borrowed_place.as_ref());

let borrow_spans = self.retrieve_borrow_spans(borrow);
let borrow_span = borrow_spans.args_or_use();

let move_spans = self.move_spans(place.as_ref(), location);
let span = move_spans.args_or_use();

let mut err = self.cannot_move_when_borrowed(
span,
&self.describe_place(place.as_ref()).unwrap_or_else(|| "_".to_owned()),
);
let mut err =
self.cannot_move_when_borrowed(span, &self.describe_any_place(place.as_ref()));
err.span_label(borrow_span, format!("borrow of {} occurs here", borrow_msg));
err.span_label(span, format!("move out of {} occurs here", value_msg));

Expand Down Expand Up @@ -314,16 +306,15 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {

let mut err = self.cannot_use_when_mutably_borrowed(
span,
&self.describe_place(place.as_ref()).unwrap_or_else(|| "_".to_owned()),
&self.describe_any_place(place.as_ref()),
borrow_span,
&self.describe_place(borrow.borrowed_place.as_ref()).unwrap_or_else(|| "_".to_owned()),
&self.describe_any_place(borrow.borrowed_place.as_ref()),
);

borrow_spans.var_span_label(&mut err, {
let place = &borrow.borrowed_place;
let desc_place = self.describe_place(place.as_ref()).unwrap_or_else(|| "_".to_owned());

format!("borrow occurs due to use of `{}`{}", desc_place, borrow_spans.describe())
let desc_place = self.describe_any_place(place.as_ref());
format!("borrow occurs due to use of {}{}", desc_place, borrow_spans.describe())
});

self.explain_why_borrow_contains_point(location, borrow, None)
Expand Down Expand Up @@ -433,7 +424,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
borrow_spans.var_span_label(
&mut err,
format!(
"borrow occurs due to use of `{}`{}",
"borrow occurs due to use of {}{}",
desc_place,
borrow_spans.describe(),
),
Expand Down Expand Up @@ -511,16 +502,15 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
if issued_spans == borrow_spans {
borrow_spans.var_span_label(
&mut err,
format!("borrows occur due to use of `{}`{}", desc_place, borrow_spans.describe()),
format!("borrows occur due to use of {}{}", desc_place, borrow_spans.describe()),
);
} else {
let borrow_place = &issued_borrow.borrowed_place;
let borrow_place_desc =
self.describe_place(borrow_place.as_ref()).unwrap_or_else(|| "_".to_owned());
let borrow_place_desc = self.describe_any_place(borrow_place.as_ref());
issued_spans.var_span_label(
&mut err,
format!(
"first borrow occurs due to use of `{}`{}",
"first borrow occurs due to use of {}{}",
borrow_place_desc,
issued_spans.describe(),
),
Expand All @@ -529,7 +519,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
borrow_spans.var_span_label(
&mut err,
format!(
"second borrow occurs due to use of `{}`{}",
"second borrow occurs due to use of {}{}",
desc_place,
borrow_spans.describe(),
),
Expand All @@ -538,7 +528,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {

if union_type_name != "" {
err.note(&format!(
"`{}` is a field of the union `{}`, so it overlaps the field `{}`",
"{} is a field of the union `{}`, so it overlaps the field {}",
msg_place, union_type_name, msg_borrow,
));
}
Expand Down Expand Up @@ -606,7 +596,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let ty = Place::ty_from(place_base, place_projection, *self.body, self.infcx.tcx).ty;
ty.ty_adt_def().filter(|adt| adt.is_union()).map(|_| ty)
};
let describe_place = |place| self.describe_place(place).unwrap_or_else(|| "_".to_owned());

// 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
Expand Down Expand Up @@ -645,30 +634,25 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
.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 Place { local, projection } = second_borrowed_place;
let Place { local, ref projection } = *second_borrowed_place;

let mut cursor = &projection[..];
while let [proj_base @ .., elem] = cursor {
cursor = proj_base;

if let ProjectionElem::Field(field, _) = elem {
if let Some(union_ty) = union_ty(*local, proj_base) {
if let Some(union_ty) = union_ty(local, proj_base) {
if field != target_field
&& *local == target_base.local
&& local == target_base.local
&& proj_base == target_base.projection
{
// FIXME when we avoid clone reuse describe_place closure
let describe_base_place = self
.describe_place(PlaceRef {
local: *local,
projection: proj_base,
})
.unwrap_or_else(|| "_".to_owned());

return Some((
describe_base_place,
describe_place(first_borrowed_place.as_ref()),
describe_place(second_borrowed_place.as_ref()),
self.describe_any_place(PlaceRef {
local,
projection: proj_base,
}),
self.describe_any_place(first_borrowed_place.as_ref()),
self.describe_any_place(second_borrowed_place.as_ref()),
union_ty.to_string(),
));
}
Expand All @@ -681,7 +665,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// If we didn't find a field access into a union, or both places match, then
// only return the description of the first place.
(
describe_place(first_borrowed_place.as_ref()),
self.describe_any_place(first_borrowed_place.as_ref()),
"".to_string(),
"".to_string(),
"".to_string(),
Expand Down Expand Up @@ -1404,12 +1388,13 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let loan_spans = self.retrieve_borrow_spans(loan);
let loan_span = loan_spans.args_or_use();

let descr_place = self.describe_any_place(place.as_ref());
if loan.kind == BorrowKind::Shallow {
if let Some(section) = self.classify_immutable_section(&loan.assigned_place) {
let mut err = self.cannot_mutate_in_immutable_section(
span,
loan_span,
&self.describe_place(place.as_ref()).unwrap_or_else(|| "_".to_owned()),
&descr_place,
section,
"assign",
);
Expand All @@ -1424,11 +1409,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}

let mut err = self.cannot_assign_to_borrowed(
span,
loan_span,
&self.describe_place(place.as_ref()).unwrap_or_else(|| "_".to_owned()),
);
let mut err = self.cannot_assign_to_borrowed(span, loan_span, &descr_place);

loan_spans
.var_span_label(&mut err, format!("borrow occurs due to use{}", loan_spans.describe()));
Expand Down Expand Up @@ -1482,27 +1463,19 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
})
| Some(LocalDecl { local_info: LocalInfo::StaticRef { .. }, .. })
| Some(LocalDecl { local_info: LocalInfo::Other, .. })
| None => (self.describe_place(place.as_ref()), assigned_span),
Some(decl) => (self.describe_place(err_place.as_ref()), decl.source_info.span),
| None => (self.describe_any_place(place.as_ref()), assigned_span),
Some(decl) => (self.describe_any_place(err_place.as_ref()), decl.source_info.span),
};

let mut err = self.cannot_reassign_immutable(
span,
place_description.as_ref().map(AsRef::as_ref).unwrap_or("_"),
from_arg,
);
let mut err = self.cannot_reassign_immutable(span, &place_description, from_arg);
let msg = if from_arg {
"cannot assign to immutable argument"
} else {
"cannot assign twice to immutable variable"
};
if span != assigned_span {
if !from_arg {
let value_msg = match place_description {
Some(name) => format!("`{}`", name),
None => "value".to_owned(),
};
err.span_label(assigned_span, format!("first assignment to {}", value_msg));
err.span_label(assigned_span, format!("first assignment to {}", place_description));
}
}
if let Some(decl) = local_decl {
Expand Down
19 changes: 17 additions & 2 deletions src/librustc_mir/borrow_check/diagnostics/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,23 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}

/// End-user visible description of `place` if one can be found. If the
/// place is a temporary for instance, None will be returned.
/// End-user visible description of `place` if one can be found.
/// If the place is a temporary for instance, `"value"` will be returned.
pub(super) fn describe_any_place(&self, place_ref: PlaceRef<'tcx>) -> String {
match self.describe_place(place_ref) {
Some(mut descr) => {
// Surround descr with `backticks`.
descr.reserve(2);
descr.insert_str(0, "`");
descr.push_str("`");
descr
}
None => "value".to_string(),
}
}

/// End-user visible description of `place` if one can be found.
/// If the place is a temporary for instance, None will be returned.
pub(super) fn describe_place(&self, place_ref: PlaceRef<'tcx>) -> Option<String> {
self.describe_place_with_options(place_ref, IncludingDowncast(false))
}
Expand Down
24 changes: 11 additions & 13 deletions src/librustc_mir/borrow_check/diagnostics/move_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,14 +272,14 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
span: Span,
) -> DiagnosticBuilder<'a> {
let description = if place.projection.len() == 1 {
format!("static item `{}`", self.describe_place(place.as_ref()).unwrap())
format!("static item {}", self.describe_any_place(place.as_ref()))
} else {
let base_static = PlaceRef { local: place.local, projection: &[ProjectionElem::Deref] };

format!(
"`{:?}` as `{:?}` is a static item",
self.describe_place(place.as_ref()).unwrap(),
self.describe_place(base_static).unwrap(),
"{} as {} is a static item",
self.describe_any_place(place.as_ref()),
self.describe_any_place(base_static),
)
};

Expand Down Expand Up @@ -349,16 +349,14 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
let upvar_name = upvar.name;
let upvar_span = self.infcx.tcx.hir().span(upvar_hir_id);

let place_name = self.describe_place(move_place.as_ref()).unwrap();
let place_name = self.describe_any_place(move_place.as_ref());

let place_description = if self
.is_upvar_field_projection(move_place.as_ref())
.is_some()
{
format!("`{}`, a {}", place_name, capture_description)
} else {
format!("`{}`, as `{}` is a {}", place_name, upvar_name, capture_description,)
};
let place_description =
if self.is_upvar_field_projection(move_place.as_ref()).is_some() {
format!("{}, a {}", place_name, capture_description)
} else {
format!("{}, as `{}` is a {}", place_name, upvar_name, capture_description)
};

debug!(
"report: closure_kind_ty={:?} closure_kind={:?} place_description={:?}",
Expand Down
5 changes: 2 additions & 3 deletions src/librustc_mir/borrow_check/diagnostics/mutability_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
borrow_spans.var_span_label(
&mut err,
format!(
"mutable borrow occurs due to use of `{}` in closure",
// always Some() if the message is printed.
self.describe_place(access_place.as_ref()).unwrap_or_default(),
"mutable borrow occurs due to use of {} in closure",
self.describe_any_place(access_place.as_ref()),
),
);
borrow_span
Expand Down
Loading