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

librustc: Some borrowck error message fixes #17434

Merged
merged 4 commits into from
Oct 2, 2014
Merged

librustc: Some borrowck error message fixes #17434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8577be
Output a note when lifetimes cannot be elided from functions
ftxqxd Sep 13, 2014
36b8502
Add an explanatory note when calling a closure via `&`
ftxqxd Sep 13, 2014
35ff2de
Clarify some borrowck errors
ftxqxd Sep 15, 2014
02c6ebd
Change the `use of moved value` error to be more accurate
ftxqxd Sep 13, 2014
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
84 changes: 57 additions & 27 deletions src/librustc/middle/borrowck/check_loans.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,50 +400,82 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
for restr_path in loan1.restricted_paths.iter() {
if *restr_path != loan2_base_path { continue; }

let old_pronoun = if new_loan.loan_path == old_loan.loan_path {
// If new_loan is something like `x.a`, and old_loan is something like `x.b`, we would
// normally generate a rather confusing message (in this case, for multiple mutable
// borrows):
//
// error: cannot borrow `x.b` as mutable more than once at a time
// note: previous borrow of `x.a` occurs here; the mutable borrow prevents
// subsequent moves, borrows, or modification of `x.a` until the borrow ends
//
// What we want to do instead is get the 'common ancestor' of the two borrow paths and
// use that for most of the message instead, giving is something like this:
//
// error: cannot borrow `x` as mutable more than once at a time
// note: previous borrow of `x` occurs here (through borrowing `x.a`); the mutable
// borrow prevents subsequent moves, borrows, or modification of `x` until the
// borrow ends

let common = new_loan.loan_path.common(&*old_loan.loan_path);
let (nl, ol, new_loan_msg, old_loan_msg) =
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!(" (here through borrowing `{}`)",
self.bccx.loan_path_to_string(
&*new_loan.loan_path));
let old_loan_msg = format!(" (through borrowing `{}`)",
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),
self.bccx.loan_path_to_string(&*old_loan.loan_path),
String::new(), String::new())
};

let ol_pronoun = if new_loan.loan_path == old_loan.loan_path {
"it".to_string()
} else {
format!("`{}`",
self.bccx.loan_path_to_string(&*old_loan.loan_path))
format!("`{}`", ol)
};

match (new_loan.kind, old_loan.kind) {
(ty::MutBorrow, ty::MutBorrow) => {
self.bccx.span_err(
new_loan.span,
format!("cannot borrow `{}` as mutable \
format!("cannot borrow `{}`{} as mutable \
more than once at a time",
self.bccx.loan_path_to_string(
&*new_loan.loan_path)).as_slice());
nl, new_loan_msg).as_slice())
}

(ty::UniqueImmBorrow, _) => {
self.bccx.span_err(
new_loan.span,
format!("closure requires unique access to `{}` \
but {} is already borrowed",
self.bccx.loan_path_to_string(&*new_loan.loan_path),
old_pronoun).as_slice());
but {} is already borrowed{}",
nl, ol_pronoun, old_loan_msg).as_slice());
}

(_, ty::UniqueImmBorrow) => {
self.bccx.span_err(
new_loan.span,
format!("cannot borrow `{}` as {} because \
format!("cannot borrow `{}`{} as {} because \
previous closure requires unique access",
self.bccx.loan_path_to_string(&*new_loan.loan_path),
new_loan.kind.to_user_str()).as_slice());
nl, new_loan_msg, new_loan.kind.to_user_str()).as_slice());
}

(_, _) => {
self.bccx.span_err(
new_loan.span,
format!("cannot borrow `{}` as {} because \
{} is also borrowed as {}",
self.bccx.loan_path_to_string(&*new_loan.loan_path),
format!("cannot borrow `{}`{} as {} because \
{} is also borrowed as {}{}",
nl,
new_loan_msg,
new_loan.kind.to_user_str(),
old_pronoun,
old_loan.kind.to_user_str()).as_slice());
ol_pronoun,
old_loan.kind.to_user_str(),
old_loan_msg).as_slice());
}
}

Expand All @@ -452,8 +484,7 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
self.bccx.span_note(
span,
format!("borrow occurs due to use of `{}` in closure",
self.bccx.loan_path_to_string(
&*new_loan.loan_path)).as_slice());
nl).as_slice());
}
_ => { }
}
Expand All @@ -463,30 +494,29 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
format!("the mutable borrow prevents subsequent \
moves, borrows, or modification of `{0}` \
until the borrow ends",
self.bccx.loan_path_to_string(
&*old_loan.loan_path))
ol)
}

ty::ImmBorrow => {
format!("the immutable borrow prevents subsequent \
moves or mutable borrows of `{0}` \
until the borrow ends",
self.bccx.loan_path_to_string(&*old_loan.loan_path))
ol)
}

ty::UniqueImmBorrow => {
format!("the unique capture prevents subsequent \
moves or borrows of `{0}` \
until the borrow ends",
self.bccx.loan_path_to_string(&*old_loan.loan_path))
ol)
}
};

let borrow_summary = match old_loan.cause {
euv::ClosureCapture(_) => {
format!("previous borrow of `{}` occurs here due to \
format!("previous borrow of `{}` occurs here{} due to \
use in closure",
self.bccx.loan_path_to_string(&*old_loan.loan_path))
ol, old_loan_msg)
}

euv::OverloadedOperator(..) |
Expand All @@ -496,8 +526,8 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
euv::ForLoop(..) |
euv::RefBinding(..) |
euv::MatchDiscriminant(..) => {
format!("previous borrow of `{}` occurs here",
self.bccx.loan_path_to_string(&*old_loan.loan_path))
format!("previous borrow of `{}` occurs here{}",
ol, old_loan_msg)
}
};

Expand Down
114 changes: 102 additions & 12 deletions src/librustc/middle/borrowck/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,51 @@ impl LoanPath {
LpExtend(ref base, _, _) => base.kill_scope(tcx),
}
}

fn has_fork(&self, other: &LoanPath) -> bool {
match (self, other) {
(&LpExtend(ref base, _, LpInterior(id)), &LpExtend(ref base2, _, LpInterior(id2))) =>
if id == id2 {
base.has_fork(&**base2)
} else {
true
},
(&LpExtend(ref base, _, LpDeref(_)), _) => base.has_fork(other),
(_, &LpExtend(ref base, _, LpDeref(_))) => self.has_fork(&**base),
_ => false,
}
}

fn depth(&self) -> uint {
match *self {
LpExtend(ref base, _, LpDeref(_)) => base.depth(),
LpExtend(ref base, _, LpInterior(_)) => base.depth() + 1,
_ => 0,
}
}

fn common(&self, other: &LoanPath) -> Option<LoanPath> {
match (self, other) {
(&LpExtend(ref base, a, LpInterior(id)), &LpExtend(ref base2, _, LpInterior(id2))) =>
if id == id2 {
base.common(&**base2).map(|x| {
let xd = x.depth();
if base.depth() == xd && base2.depth() == xd {
LpExtend(Rc::new(x), a, LpInterior(id))
} else {
x
}
})
} else {
base.common(&**base2)
},
(&LpExtend(ref base, _, LpDeref(_)), _) => base.common(other),
(_, &LpExtend(ref other, _, LpDeref(_))) => self.common(&**other),
(&LpVar(id), &LpVar(id2)) => if id == id2 { Some(LpVar(id)) } else { None },
(&LpUpvar(id), &LpUpvar(id2)) => if id == id2 { Some(LpUpvar(id)) } else { None },
_ => None,
}
}
}

pub fn opt_loan_path(cmt: &mc::cmt) -> Option<Rc<LoanPath>> {
Expand Down Expand Up @@ -416,24 +461,58 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
MovedInCapture => "capture",
};

match the_move.kind {
let (ol, moved_lp_msg) = match the_move.kind {
move_data::Declared => {
self.tcx.sess.span_err(
use_span,
format!("{} of possibly uninitialized variable: `{}`",
verb,
self.loan_path_to_string(lp)).as_slice());
(self.loan_path_to_string(moved_lp),
String::new())
}
_ => {
let partially = if lp == moved_lp {""} else {"partially "};
// If moved_lp is something like `x.a`, and lp is something like `x.b`, we would
// normally generate a rather confusing message:
//
// error: use of moved value: `x.b`
// note: `x.a` moved here...
//
// What we want to do instead is get the 'common ancestor' of the two moves and
// use that for most of the message instead, giving is something like this:
//
// error: use of moved value: `x`
// note: `x` moved here (through moving `x.a`)...

let common = moved_lp.common(lp);
let has_common = common.is_some();
let has_fork = moved_lp.has_fork(lp);
let (nl, ol, moved_lp_msg) =
if has_fork && has_common {
let nl = self.loan_path_to_string(&common.unwrap());
let ol = nl.clone();
let moved_lp_msg = format!(" (through moving `{}`)",
self.loan_path_to_string(moved_lp));
(nl, ol, moved_lp_msg)
} else {
(self.loan_path_to_string(lp),
self.loan_path_to_string(moved_lp),
String::new())
};

let partial = moved_lp.depth() > lp.depth();
let msg = if !has_fork && partial { "partially " }
else if has_fork && !has_common { "collaterally "}
else { "" };
self.tcx.sess.span_err(
use_span,
format!("{} of {}moved value: `{}`",
verb,
partially,
self.loan_path_to_string(lp)).as_slice());
msg,
nl).as_slice());
(ol, moved_lp_msg)
}
}
};

match the_move.kind {
move_data::Declared => {}
Expand All @@ -456,19 +535,21 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
"moved by default (use `copy` to override)");
self.tcx.sess.span_note(
expr_span,
format!("`{}` moved here because it has type `{}`, which is {}",
self.loan_path_to_string(moved_lp),
format!("`{}` moved here{} because it has type `{}`, which is {}",
ol,
moved_lp_msg,
expr_ty.user_string(self.tcx),
suggestion).as_slice());
}

move_data::MovePat => {
let pat_ty = ty::node_id_to_type(self.tcx, the_move.id);
self.tcx.sess.span_note(self.tcx.map.span(the_move.id),
format!("`{}` moved here because it has type `{}`, \
format!("`{}` moved here{} because it has type `{}`, \
which is moved by default (use `ref` to \
override)",
self.loan_path_to_string(moved_lp),
ol,
moved_lp_msg,
pat_ty.user_string(self.tcx)).as_slice());
}

Expand All @@ -491,9 +572,10 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
capture that instead to override)");
self.tcx.sess.span_note(
expr_span,
format!("`{}` moved into closure environment here because it \
format!("`{}` moved into closure environment here{} because it \
has type `{}`, which is {}",
self.loan_path_to_string(moved_lp),
ol,
moved_lp_msg,
expr_ty.user_string(self.tcx),
suggestion).as_slice());
}
Expand Down Expand Up @@ -602,6 +684,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
span: Span,
kind: AliasableViolationKind,
cause: mc::AliasableReason) {
let mut is_closure = false;
let prefix = match kind {
MutabilityViolation => {
"cannot assign to data"
Expand All @@ -625,6 +708,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
}

BorrowViolation(euv::ClosureInvocation) => {
is_closure = true;
"closure invocation"
}

Expand All @@ -649,14 +733,20 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
mc::AliasableManaged => {
self.tcx.sess.span_err(
span,
format!("{} in a `@` pointer", prefix).as_slice());
format!("{} in a `Gc` pointer", prefix).as_slice());
}
mc::AliasableBorrowed => {
self.tcx.sess.span_err(
span,
format!("{} in a `&` reference", prefix).as_slice());
}
}

if is_closure {
self.tcx.sess.span_note(
span,
"closures behind references must be called via `&mut`");
}
}

pub fn note_and_explain_bckerr(&self, err: BckError) {
Expand Down
Loading