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