squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Provide diagnostic suggestion in ExprUseVisitor Delegate

Browse source 
The [Delegate
trait](981346fc07/compiler/rustc_typeck/src/expr_use_visitor.rs (L28-L38))
currently use `PlaceWithHirId` which is composed of Hir `Place` and the
corresponding expression id.

Even though this is an accurate way of expressing how a Place is used,
it can cause confusion during diagnostics.

Eg:

```
let arr : [String; 5];

let [a, ...]     =   arr;
 ^^^ E1 ^^^      =  ^^E2^^
 ```

 Here `arr` is moved because of the binding created E1. However, when we
 point to E1 in diagnostics with the message `arr` was moved, it can be
 confusing.  Rather we would like to report E2 to the user.

 Closes: https://github.com/rust-lang/project-rfc-2229/issues/20
This commit is contained in:
Dhruv Jauhar 2020-11-01 02:15:22 -05:00
parent c4fe25d861
commit 1f53754236
7 changed files with 124 additions and 70 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

83
compiler/rustc_typeck/src/check/upvar.rs
View file

@ -279,11 +279,12 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
fn adjust_upvar_borrow_kind_for_consume(
&mut self,
place_with_id: &PlaceWithHirId<'tcx>,
diag_expr_id: hir::HirId,
mode: euv::ConsumeMode,
) {
debug!(
"adjust_upvar_borrow_kind_for_consume(place_with_id={:?}, mode={:?})",
place_with_id, mode
"adjust_upvar_borrow_kind_for_consume(place_with_id={:?}, diag_expr_id={:?}, mode={:?})",
place_with_id, diag_expr_id, mode
);
// we only care about moves
@ -303,7 +304,7 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
debug!("adjust_upvar_borrow_kind_for_consume: upvar={:?}", upvar_id);
let usage_span = tcx.hir().span(place_with_id.hir_id);
let usage_span = tcx.hir().span(diag_expr_id);
// To move out of an upvar, this must be a FnOnce closure
self.adjust_closure_kind(
@ -313,14 +314,7 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
var_name(tcx, upvar_id.var_path.hir_id),
);
// In a case like `let pat = upvar`, don't use the span
// of the pattern, as this just looks confusing.
let by_value_span = match tcx.hir().get(place_with_id.hir_id) {
hir::Node::Pat(_) => None,
_ => Some(usage_span),
};
let new_capture = ty::UpvarCapture::ByValue(by_value_span);
let new_capture = ty::UpvarCapture::ByValue(Some(usage_span));
match self.adjust_upvar_captures.entry(upvar_id) {
Entry::Occupied(mut e) => {
match e.get() {
@ -345,8 +339,15 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
/// Indicates that `place_with_id` is being directly mutated (e.g., assigned
/// to). If the place is based on a by-ref upvar, this implies that
/// the upvar must be borrowed using an `&mut` borrow.
fn adjust_upvar_borrow_kind_for_mut(&mut self, place_with_id: &PlaceWithHirId<'tcx>) {
debug!("adjust_upvar_borrow_kind_for_mut(place_with_id={:?})", place_with_id);
fn adjust_upvar_borrow_kind_for_mut(
&mut self,
place_with_id: &PlaceWithHirId<'tcx>,
diag_expr_id: hir::HirId,
) {
debug!(
"adjust_upvar_borrow_kind_for_mut(place_with_id={:?}, diag_expr_id={:?})",
place_with_id, diag_expr_id
);
if let PlaceBase::Upvar(upvar_id) = place_with_id.place.base {
let mut borrow_kind = ty::MutBorrow;
@ -362,16 +363,19 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
_ => (),
}
}
self.adjust_upvar_deref(
upvar_id,
self.fcx.tcx.hir().span(place_with_id.hir_id),
borrow_kind,
);
self.adjust_upvar_deref(upvar_id, self.fcx.tcx.hir().span(diag_expr_id), borrow_kind);
}
}
fn adjust_upvar_borrow_kind_for_unique(&mut self, place_with_id: &PlaceWithHirId<'tcx>) {
debug!("adjust_upvar_borrow_kind_for_unique(place_with_id={:?})", place_with_id);
fn adjust_upvar_borrow_kind_for_unique(
&mut self,
place_with_id: &PlaceWithHirId<'tcx>,
diag_expr_id: hir::HirId,
) {
debug!(
"adjust_upvar_borrow_kind_for_unique(place_with_id={:?}, diag_expr_id={:?})",
place_with_id, diag_expr_id
);
if let PlaceBase::Upvar(upvar_id) = place_with_id.place.base {
if place_with_id.place.deref_tys().any(ty::TyS::is_unsafe_ptr) {
@ -381,7 +385,7 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
// for a borrowed pointer to be unique, its base must be unique
self.adjust_upvar_deref(
upvar_id,
self.fcx.tcx.hir().span(place_with_id.hir_id),
self.fcx.tcx.hir().span(diag_expr_id),
ty::UniqueImmBorrow,
);
}
@ -500,29 +504,44 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
}
impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, mode: euv::ConsumeMode) {
debug!("consume(place_with_id={:?},mode={:?})", place_with_id, mode);
self.adjust_upvar_borrow_kind_for_consume(place_with_id, mode);
fn consume(
&mut self,
place_with_id: &PlaceWithHirId<'tcx>,
diag_expr_id: hir::HirId,
mode: euv::ConsumeMode,
) {
debug!(
"consume(place_with_id={:?}, diag_expr_id={:?}, mode={:?})",
place_with_id, diag_expr_id, mode
);
self.adjust_upvar_borrow_kind_for_consume(&place_with_id, diag_expr_id, mode);
}
fn borrow(&mut self, place_with_id: &PlaceWithHirId<'tcx>, bk: ty::BorrowKind) {
debug!("borrow(place_with_id={:?}, bk={:?})", place_with_id, bk);
fn borrow(
&mut self,
place_with_id: &PlaceWithHirId<'tcx>,
diag_expr_id: hir::HirId,
bk: ty::BorrowKind,
) {
debug!(
"borrow(place_with_id={:?}, diag_expr_id={:?}, bk={:?})",
place_with_id, diag_expr_id, bk
);
match bk {
ty::ImmBorrow => {}
ty::UniqueImmBorrow => {
self.adjust_upvar_borrow_kind_for_unique(place_with_id);
self.adjust_upvar_borrow_kind_for_unique(&place_with_id, diag_expr_id);
}
ty::MutBorrow => {
self.adjust_upvar_borrow_kind_for_mut(place_with_id);
self.adjust_upvar_borrow_kind_for_mut(&place_with_id, diag_expr_id);
}
}
}
fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>) {
debug!("mutate(assignee_place={:?})", assignee_place);
self.adjust_upvar_borrow_kind_for_mut(assignee_place);
fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) {
debug!("mutate(assignee_place={:?}, diag_expr_id={:?})", assignee_place, diag_expr_id);
self.adjust_upvar_borrow_kind_for_mut(assignee_place, diag_expr_id);
}
}

77
compiler/rustc_typeck/src/expr_use_visitor.rs
View file

@ -27,14 +27,38 @@ use rustc_span::Span;
/// employing the ExprUseVisitor.
pub trait Delegate<'tcx> {
// The value found at `place` is either copied or moved, depending
// on mode.
fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, mode: ConsumeMode);
// on mode. Where `diag_expr_id` is the id used for diagnostics for `place`.
//
// The reson for a separate expr_id for diagonostics is to support cases
// like `let pat = upvar`, in such scenarios reporting the pattern (lhs)
// looks confusing. Instead we prefer to report the discriminant (rhs)
fn consume(
&mut self,
place_with_id: &PlaceWithHirId<'tcx>,
diag_expr_id: hir::HirId,
mode: ConsumeMode,
);
// The value found at `place` is being borrowed with kind `bk`.
fn borrow(&mut self, place_with_id: &PlaceWithHirId<'tcx>, bk: ty::BorrowKind);
// `diag_expr_id` is the id used for diagnostics for `place`.
//
// The reson for a separate expr_id for diagonostics is to support cases
// like `let pat = upvar`, in such scenarios reporting the pattern (lhs)
// looks confusing. Instead we prefer to report the discriminant (rhs)
fn borrow(
&mut self,
place_with_id: &PlaceWithHirId<'tcx>,
diag_expr_id: hir::HirId,
bk: ty::BorrowKind,
);
// The path at `place_with_id` is being assigned to.
fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>);
// The path at `assignee_place` is being assigned to.
// `diag_expr_id` is the id used for diagnostics for `place`.
//
// The reson for a separate expr_id for diagonostics is to support cases
// like `let pat = upvar`, in such scenarios reporting the pattern (lhs)
// looks confusing. Instead we prefer to report the discriminant (rhs)
fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId);
}
#[derive(Copy, Clone, PartialEq, Debug)]
@ -116,11 +140,11 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
self.mc.tcx()
}
fn delegate_consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>) {
fn delegate_consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) {
debug!("delegate_consume(place_with_id={:?})", place_with_id);
let mode = copy_or_move(&self.mc, place_with_id);
self.delegate.consume(place_with_id, mode);
self.delegate.consume(place_with_id, diag_expr_id, mode);
}
fn consume_exprs(&mut self, exprs: &[hir::Expr<'_>]) {
@ -133,13 +157,13 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
debug!("consume_expr(expr={:?})", expr);
let place_with_id = return_if_err!(self.mc.cat_expr(expr));
self.delegate_consume(&place_with_id);
self.delegate_consume(&place_with_id, place_with_id.hir_id);
self.walk_expr(expr);
}
fn mutate_expr(&mut self, expr: &hir::Expr<'_>) {
let place_with_id = return_if_err!(self.mc.cat_expr(expr));
self.delegate.mutate(&place_with_id);
self.delegate.mutate(&place_with_id, place_with_id.hir_id);
self.walk_expr(expr);
}
@ -147,7 +171,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
debug!("borrow_expr(expr={:?}, bk={:?})", expr, bk);
let place_with_id = return_if_err!(self.mc.cat_expr(expr));
self.delegate.borrow(&place_with_id, bk);
self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk);
self.walk_expr(expr)
}
@ -404,7 +428,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
with_field.ty(self.tcx(), substs),
ProjectionKind::Field(f_index as u32, VariantIdx::new(0)),
);
self.delegate_consume(&field_place);
self.delegate_consume(&field_place, field_place.hir_id);
}
}
}
@ -436,7 +460,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
adjustment::Adjust::NeverToAny | adjustment::Adjust::Pointer(_) => {
// Creating a closure/fn-pointer or unsizing consumes
// the input and stores it into the resulting rvalue.
self.delegate_consume(&place_with_id);
self.delegate_consume(&place_with_id, place_with_id.hir_id);
}
adjustment::Adjust::Deref(None) => {}
@ -448,7 +472,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
// this is an autoref of `x`.
adjustment::Adjust::Deref(Some(ref deref)) => {
let bk = ty::BorrowKind::from_mutbl(deref.mutbl);
self.delegate.borrow(&place_with_id, bk);
self.delegate.borrow(&place_with_id, place_with_id.hir_id, bk);
}
adjustment::Adjust::Borrow(ref autoref) => {
@ -476,13 +500,17 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
match *autoref {
adjustment::AutoBorrow::Ref(_, m) => {
self.delegate.borrow(base_place, ty::BorrowKind::from_mutbl(m.into()));
self.delegate.borrow(
base_place,
base_place.hir_id,
ty::BorrowKind::from_mutbl(m.into()),
);
}
adjustment::AutoBorrow::RawPtr(m) => {
debug!("walk_autoref: expr.hir_id={} base_place={:?}", expr.hir_id, base_place);
self.delegate.borrow(base_place, ty::BorrowKind::from_mutbl(m));
self.delegate.borrow(base_place, base_place.hir_id, ty::BorrowKind::from_mutbl(m));
}
}
}
@ -525,19 +553,22 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
// binding being produced.
let def = Res::Local(canonical_id);
if let Ok(ref binding_place) = mc.cat_res(pat.hir_id, pat.span, pat_ty, def) {
delegate.mutate(binding_place);
delegate.mutate(binding_place, binding_place.hir_id);
}
// It is also a borrow or copy/move of the value being matched.
// In a cases of pattern like `let pat = upvar`, don't use the span
// of the pattern, as this just looks confusing, instead use the span
// of the discriminant.
match bm {
ty::BindByReference(m) => {
let bk = ty::BorrowKind::from_mutbl(m);
delegate.borrow(place, bk);
delegate.borrow(place, discr_place.hir_id, bk);
}
ty::BindByValue(..) => {
let mode = copy_or_move(mc, place);
let mode = copy_or_move(mc, &place);
debug!("walk_pat binding consuming pat");
delegate.consume(place, mode);
delegate.consume(place, discr_place.hir_id, mode);
}
}
}
@ -564,10 +595,14 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
match upvar_capture {
ty::UpvarCapture::ByValue(_) => {
let mode = copy_or_move(&self.mc, &captured_place);
self.delegate.consume(&captured_place, mode);
self.delegate.consume(&captured_place, captured_place.hir_id, mode);
}
ty::UpvarCapture::ByRef(upvar_borrow) => {
self.delegate.borrow(&captured_place, upvar_borrow.kind);
self.delegate.borrow(
&captured_place,
captured_place.hir_id,
upvar_borrow.kind,
);
}
}
}

2
src/test/ui/borrowck/borrowck-closures-slice-patterns.stderr
View file

@ -75,7 +75,7 @@ LL | fn arr_box_by_move(x: Box<[String; 3]>) {
LL | let f = || {
| -- value moved into closure here
LL | let [y, z @ ..] = *x;
| - variable moved due to use in closure
| -- variable moved due to use in closure
LL | };
LL | &x;
| ^^ value borrowed here after move

6
src/tools/clippy/clippy_lints/src/escape.rs
View file

@ -116,7 +116,7 @@ fn is_argument(map: rustc_middle::hir::map::Map<'_>, id: HirId) -> bool {
}
impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
fn consume(&mut self, cmt: &PlaceWithHirId<'tcx>, mode: ConsumeMode) {
fn consume(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, mode: ConsumeMode) {
if cmt.place.projections.is_empty() {
if let PlaceBase::Local(lid) = cmt.place.base {
if let ConsumeMode::Move = mode {
@ -136,7 +136,7 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
}
}
fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: ty::BorrowKind) {
fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
if cmt.place.projections.is_empty() {
if let PlaceBase::Local(lid) = cmt.place.base {
self.set.remove(&lid);
@ -144,7 +144,7 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
}
}
fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>) {
fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) {
if cmt.place.projections.is_empty() {
let map = &self.cx.tcx.hir();
if is_argument(*map, cmt.hir_id) {

14
src/tools/clippy/clippy_lints/src/loops.rs
View file

@ -1694,28 +1694,28 @@ struct MutatePairDelegate<'a, 'tcx> {
}
impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> {
fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: ConsumeMode) {}
fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: ConsumeMode) {}
fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, bk: ty::BorrowKind) {
fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) {
if let ty::BorrowKind::MutBorrow = bk {
if let PlaceBase::Local(id) = cmt.place.base {
if Some(id) == self.hir_id_low {
self.span_low = Some(self.cx.tcx.hir().span(cmt.hir_id))
self.span_low = Some(self.cx.tcx.hir().span(diag_expr_id))
}
if Some(id) == self.hir_id_high {
self.span_high = Some(self.cx.tcx.hir().span(cmt.hir_id))
self.span_high = Some(self.cx.tcx.hir().span(diag_expr_id))
}
}
}
}
fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>) {
fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) {
if let PlaceBase::Local(id) = cmt.place.base {
if Some(id) == self.hir_id_low {
self.span_low = Some(self.cx.tcx.hir().span(cmt.hir_id))
self.span_low = Some(self.cx.tcx.hir().span(diag_expr_id))
}
if Some(id) == self.hir_id_high {
self.span_high = Some(self.cx.tcx.hir().span(cmt.hir_id))
self.span_high = Some(self.cx.tcx.hir().span(diag_expr_id))
}
}
}

6
src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs
View file

@ -325,13 +325,13 @@ impl MovedVariablesCtxt {
}
impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt {
fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, mode: euv::ConsumeMode) {
fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _: HirId, mode: euv::ConsumeMode) {
if let euv::ConsumeMode::Move = mode {
self.move_common(cmt);
}
}
fn borrow(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: ty::BorrowKind) {}
fn borrow(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {}
fn mutate(&mut self, _: &euv::PlaceWithHirId<'tcx>) {}
fn mutate(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId) {}
}

6
src/tools/clippy/clippy_lints/src/utils/usage.rs
View file

@ -68,15 +68,15 @@ impl<'tcx> MutVarsDelegate {
}
impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: ConsumeMode) {}
fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: ConsumeMode) {}
fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, bk: ty::BorrowKind) {
fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, bk: ty::BorrowKind) {
if let ty::BorrowKind::MutBorrow = bk {
self.update(&cmt)
}
}
fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>) {
fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) {
self.update(&cmt)
}
}