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

Update closure capture error logging for disjoint captures for disjoint captures #84358

Merged
merged 6 commits into from
May 2, 2021
Merged

Update closure capture error logging for disjoint captures for disjoint captures #84358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
08c4fbc
Closure capture borrow diagnostics for disjoint captures
ChrisPardy Mar 17, 2021
25beade
Update compiler/rustc_mir/src/borrow_check/diagnostics/explain_borrow.rs
ChrisPardy Apr 29, 2021
20a1315
Update compiler/rustc_mir/src/borrow_check/diagnostics/explain_borrow.rs
ChrisPardy Apr 29, 2021
e612f7a
Update compiler/rustc_mir/src/borrow_check/diagnostics/explain_borrow.rs
ChrisPardy Apr 29, 2021
d19c468
add docstrings and add issue to FIXMEs
ChrisPardy Apr 29, 2021
404cc33
fix nll test stderr
ChrisPardy May 2, 2021
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
11 changes: 11 additions & 0 deletions compiler/rustc_middle/src/mir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,15 @@ impl BorrowKind {
BorrowKind::Mut { allow_two_phase_borrow } => allow_two_phase_borrow,
}
}

pub fn describe_mutability(&self) -> String {
match *self {
BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Unique => {
"immutable".to_string()
}
BorrowKind::Mut { .. } => "mutable".to_string(),
}
}
}

///////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -2369,6 +2378,7 @@ impl<'tcx> Debug for Rvalue<'tcx> {
};
let mut struct_fmt = fmt.debug_struct(&name);

// FIXME(project-rfc-2229#48): This should be a list of capture names/places
if let Some(upvars) = tcx.upvars_mentioned(def_id) {
for (&var_id, place) in iter::zip(upvars.keys(), places) {
let var_name = tcx.hir().name(var_id);
Expand All @@ -2388,6 +2398,7 @@ impl<'tcx> Debug for Rvalue<'tcx> {
let name = format!("[generator@{:?}]", tcx.hir().span(hir_id));
let mut struct_fmt = fmt.debug_struct(&name);

// FIXME(project-rfc-2229#48): This should be a list of capture names/places
if let Some(upvars) = tcx.upvars_mentioned(def_id) {
for (&var_id, place) in iter::zip(upvars.keys(), places) {
let var_name = tcx.hir().name(var_id);
Expand Down
20 changes: 20 additions & 0 deletions compiler/rustc_middle/src/ty/closure.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ pub struct CapturedPlace<'tcx> {
}

impl CapturedPlace<'tcx> {
pub fn to_string(&self, tcx: TyCtxt<'tcx>) -> String {
place_to_string_for_capture(tcx, &self.place)
}

/// Returns the hir-id of the root variable for the captured place.
/// e.g., if `a.b.c` was captured, would return the hir-id for `a`.
pub fn get_root_variable(&self) -> hir::HirId {
Expand All @@ -168,6 +172,22 @@ impl CapturedPlace<'tcx> {
}
}

/// Return span pointing to use that resulted in selecting the captured path
pub fn get_path_span(&self, tcx: TyCtxt<'tcx>) -> Span {
if let Some(path_expr_id) = self.info.path_expr_id {
tcx.hir().span(path_expr_id)
} else if let Some(capture_kind_expr_id) = self.info.capture_kind_expr_id {
tcx.hir().span(capture_kind_expr_id)
} else {
// Fallback on upvars mentioned if neither path or capture expr id is captured

// Safe to unwrap since we know this place is captured by the closure, therefore the closure must have upvars.
tcx.upvars_mentioned(self.get_closure_local_def_id()).unwrap()
[&self.get_root_variable()]
.span
}
}

/// Return span pointing to use that resulted in selecting the current capture kind
pub fn get_capture_kind_span(&self, tcx: TyCtxt<'tcx>) -> Span {
if let Some(capture_kind_expr_id) = self.info.capture_kind_expr_id {
Expand Down
48 changes: 33 additions & 15 deletions compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
);
err.span_label(span, format!("use of possibly-uninitialized {}", item_msg));

use_spans.var_span_label(
use_spans.var_span_label_path_only(
&mut err,
format!("{} occurs due to use{}", desired_action.as_noun(), use_spans.describe()),
);
Expand Down Expand Up @@ -255,6 +255,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
partially_str,
move_spans.describe()
),
"moved",
);
}
}
Expand Down Expand Up @@ -304,7 +305,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}

use_spans.var_span_label(
use_spans.var_span_label_path_only(
&mut err,
format!("{} occurs due to use{}", desired_action.as_noun(), use_spans.describe()),
);
Expand Down Expand Up @@ -434,13 +435,16 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
err.span_label(borrow_span, format!("borrow of {} occurs here", borrow_msg));
err.span_label(span, format!("move out of {} occurs here", value_msg));

borrow_spans.var_span_label(
borrow_spans.var_span_label_path_only(
&mut err,
format!("borrow occurs due to use{}", borrow_spans.describe()),
);

move_spans
.var_span_label(&mut err, format!("move occurs due to use{}", move_spans.describe()));
move_spans.var_span_label(
&mut err,
format!("move occurs due to use{}", move_spans.describe()),
"moved",
);

self.explain_why_borrow_contains_point(location, borrow, None)
.add_explanation_to_diagnostic(
Expand Down Expand Up @@ -468,18 +472,24 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let use_spans = self.move_spans(place.as_ref(), location);
let span = use_spans.var_or_use();

// If the attempted use is in a closure then we do not care about the path span of the place we are currently trying to use
// we call `var_span_label` on `borrow_spans` to annotate if the existing borrow was in a closure
let mut err = self.cannot_use_when_mutably_borrowed(
span,
&self.describe_any_place(place.as_ref()),
borrow_span,
&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_any_place(place.as_ref());
format!("borrow occurs due to use of {}{}", desc_place, borrow_spans.describe())
});
borrow_spans.var_span_label(
&mut err,
{
let place = &borrow.borrowed_place;
let desc_place = self.describe_any_place(place.as_ref());
format!("borrow occurs due to use of {}{}", desc_place, borrow_spans.describe())
},
"mutable",
);

self.explain_why_borrow_contains_point(location, borrow, None)
.add_explanation_to_diagnostic(
Expand Down Expand Up @@ -591,6 +601,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
desc_place,
borrow_spans.describe(),
),
"immutable",
);

return err;
Expand Down Expand Up @@ -667,7 +678,8 @@ 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(),),
gen_borrow_kind.describe_mutability(),
);
} else {
let borrow_place = &issued_borrow.borrowed_place;
Expand All @@ -679,6 +691,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
borrow_place_desc,
issued_spans.describe(),
),
issued_borrow.kind.describe_mutability(),
);

borrow_spans.var_span_label(
Expand All @@ -688,6 +701,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
desc_place,
borrow_spans.describe(),
),
gen_borrow_kind.describe_mutability(),
);
}

Expand Down Expand Up @@ -847,7 +861,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
self.prefixes(borrow.borrowed_place.as_ref(), PrefixSet::All).last().unwrap();

let borrow_spans = self.retrieve_borrow_spans(borrow);
let borrow_span = borrow_spans.var_or_use();
let borrow_span = borrow_spans.var_or_use_path_span();

assert!(root_place.projection.is_empty());
let proper_span = self.body.local_decls[root_place.local].source_info.span;
Expand Down Expand Up @@ -987,7 +1001,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
location, name, borrow, drop_span, borrow_spans
);

let borrow_span = borrow_spans.var_or_use();
let borrow_span = borrow_spans.var_or_use_path_span();
if let BorrowExplanation::MustBeValidFor {
category,
span,
Expand Down Expand Up @@ -1575,6 +1589,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
loan_spans.var_span_label(
&mut err,
format!("borrow occurs due to use{}", loan_spans.describe()),
loan.kind.describe_mutability(),
);

err.buffer(&mut self.errors_buffer);
Expand All @@ -1585,8 +1600,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {

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()));
loan_spans.var_span_label(
&mut err,
format!("borrow occurs due to use{}", loan_spans.describe()),
loan.kind.describe_mutability(),
);

self.explain_why_borrow_contains_point(location, loan, None).add_explanation_to_diagnostic(
self.infcx.tcx,
Expand Down
71 changes: 54 additions & 17 deletions compiler/rustc_mir/src/borrow_check/diagnostics/explain_borrow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use super::{find_use, RegionName, UseSpans};

#[derive(Debug)]
pub(in crate::borrow_check) enum BorrowExplanation {
UsedLater(LaterUseKind, Span),
UsedLaterInLoop(LaterUseKind, Span),
UsedLater(LaterUseKind, Span, Option<Span>),
UsedLaterInLoop(LaterUseKind, Span, Option<Span>),
UsedLaterWhenDropped {
drop_loc: Location,
dropped_local: Local,
Expand Down Expand Up @@ -67,22 +67,39 @@ impl BorrowExplanation {
borrow_span: Option<Span>,
) {
match *self {
BorrowExplanation::UsedLater(later_use_kind, var_or_use_span) => {
BorrowExplanation::UsedLater(later_use_kind, var_or_use_span, path_span) => {
let message = match later_use_kind {
LaterUseKind::TraitCapture => "captured here by trait object",
LaterUseKind::ClosureCapture => "captured here by closure",
LaterUseKind::Call => "used by call",
LaterUseKind::FakeLetRead => "stored here",
LaterUseKind::Other => "used here",
};
if !borrow_span.map_or(false, |sp| sp.overlaps(var_or_use_span)) {
err.span_label(
var_or_use_span,
format!("{}borrow later {}", borrow_desc, message),
);
// We can use `var_or_use_span` if either `path_span` is not present, or both spans are the same
if path_span.map(|path_span| path_span == var_or_use_span).unwrap_or(true) {
if borrow_span.map(|sp| !sp.overlaps(var_or_use_span)).unwrap_or(true) {
err.span_label(
var_or_use_span,
format!("{}borrow later {}", borrow_desc, message),
);
}
} else {
// path_span must be `Some` as otherwise the if condition is true
let path_span = path_span.unwrap();
// path_span is only present in the case of closure capture
assert!(matches!(later_use_kind, LaterUseKind::ClosureCapture));
if !borrow_span.map_or(false, |sp| sp.overlaps(var_or_use_span)) {
let path_label = "used here by closure";
let capture_kind_label = message;
err.span_label(
var_or_use_span,
format!("{}borrow later {}", borrow_desc, capture_kind_label),
);
err.span_label(path_span, path_label);
}
}
}
BorrowExplanation::UsedLaterInLoop(later_use_kind, var_or_use_span) => {
BorrowExplanation::UsedLaterInLoop(later_use_kind, var_or_use_span, path_span) => {
let message = match later_use_kind {
LaterUseKind::TraitCapture => {
"borrow captured here by trait object, in later iteration of loop"
Expand All @@ -94,7 +111,24 @@ impl BorrowExplanation {
LaterUseKind::FakeLetRead => "borrow later stored here",
LaterUseKind::Other => "borrow used here, in later iteration of loop",
};
err.span_label(var_or_use_span, format!("{}{}", borrow_desc, message));
// We can use `var_or_use_span` if either `path_span` is not present, or both spans are the same
if path_span.map(|path_span| path_span == var_or_use_span).unwrap_or(true) {
err.span_label(var_or_use_span, format!("{}{}", borrow_desc, message));
} else {
// path_span must be `Some` as otherwise the if condition is true
let path_span = path_span.unwrap();
// path_span is only present in the case of closure capture
assert!(matches!(later_use_kind, LaterUseKind::ClosureCapture));
if borrow_span.map(|sp| !sp.overlaps(var_or_use_span)).unwrap_or(true) {
let path_label = "used here by closure";
let capture_kind_label = message;
err.span_label(
var_or_use_span,
format!("{}borrow later {}", borrow_desc, capture_kind_label),
);
err.span_label(path_span, path_label);
}
}
}
BorrowExplanation::UsedLaterWhenDropped {
drop_loc,
Expand Down Expand Up @@ -311,13 +345,13 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let borrow_location = location;
if self.is_use_in_later_iteration_of_loop(borrow_location, location) {
let later_use = self.later_use_kind(borrow, spans, location);
BorrowExplanation::UsedLaterInLoop(later_use.0, later_use.1)
BorrowExplanation::UsedLaterInLoop(later_use.0, later_use.1, later_use.2)
} else {
// Check if the location represents a `FakeRead`, and adapt the error
// message to the `FakeReadCause` it is from: in particular,
// the ones inserted in optimized `let var = <expr>` patterns.
let later_use = self.later_use_kind(borrow, spans, location);
BorrowExplanation::UsedLater(later_use.0, later_use.1)
BorrowExplanation::UsedLater(later_use.0, later_use.1, later_use.2)
}
}

Expand Down Expand Up @@ -498,16 +532,19 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}

/// Determine how the borrow was later used.
/// First span returned points to the location of the conflicting use
/// Second span if `Some` is returned in the case of closures and points
/// to the use of the path
fn later_use_kind(
&self,
borrow: &BorrowData<'tcx>,
use_spans: UseSpans<'tcx>,
location: Location,
) -> (LaterUseKind, Span) {
) -> (LaterUseKind, Span, Option<Span>) {
match use_spans {
UseSpans::ClosureUse { var_span, .. } => {
UseSpans::ClosureUse { capture_kind_span, path_span, .. } => {
// Used in a closure.
(LaterUseKind::ClosureCapture, var_span)
(LaterUseKind::ClosureCapture, capture_kind_span, Some(path_span))
}
UseSpans::PatUse(span)
| UseSpans::OtherUse(span)
Expand Down Expand Up @@ -542,15 +579,15 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}
};
return (LaterUseKind::Call, function_span);
return (LaterUseKind::Call, function_span, None);
} else {
LaterUseKind::Other
}
} else {
LaterUseKind::Other
};

(kind, span)
(kind, span, None)
}
}
}
Expand Down
Loading