Auto merge of #78662 - sexxi-goose:add_expr_id_to_delegate, r=nikomatsakis

Provide diagnostic suggestion in ExprUseVisitor Delegate The [Delegate trait](https://github.com/rust-lang/rust/blob/981346fc07dd5ef414c5b1b21999f7604cece006/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: rust-lang/project-rfc-2229#20 r? `@ghost`

Author: bors

compiler/rustc_typeck/src/check/upvar.rs

@@ -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);
  }
 }
 

compiler/rustc_typeck/src/expr_use_visitor.rs

@@ -27,14 +27,31 @@ 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 parameter `diag_expr_id` indicates the HIR id that ought to be used for
+ // diagnostics. Around pattern matching such as `let pat = expr`, the diagnostic
+ // id will be the id of the expression `expr` but the place itself will have
+ // the id of the binding in the pattern `pat`.
+ 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);
-
- // The path at `place_with_id` is being assigned to.
- fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>);
+ // `diag_expr_id` is the id used for diagnostics (see `consume` for more details).
+ fn borrow(
+ &mut self,
+ place_with_id: &PlaceWithHirId<'tcx>,
+ diag_expr_id: hir::HirId,
+ bk: ty::BorrowKind,
+ );
+
+ // The path at `assignee_place` is being assigned to.
+ // `diag_expr_id` is the id used for diagnostics (see `consume` for more details).
+ fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId);
 }
 
 #[derive(Copy, Clone, PartialEq, Debug)]
@@ -116,11 +133,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,21 +150,21 @@ 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);
  }
 
  fn borrow_expr(&mut self, expr: &hir::Expr<'_>, bk: ty::BorrowKind) {
  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 +421,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 +453,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 +465,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 +493,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 +546,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 +588,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,
+ );
  }
  }
  }

src/test/ui/borrowck/borrowck-closures-slice-patterns.stderr

@@ -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

src/tools/clippy/clippy_lints/src/escape.rs

@@ -115,7 +115,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 {
@@ -135,15 +135,15 @@ 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);
  }
  }
  }
 
- 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) {

src/tools/clippy/clippy_lints/src/loops.rs

@@ -1957,28 +1957,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))
  }
  }
  }