Auto merge of #75933 - Aaron1011:feature/closure-move-err, r=oli-obk

Point to a move-related span when pointing to closure upvars

Fixes #75904

When emitting move/borrow errors, we may point into a closure to
indicate why an upvar is used in the closure. However, we use the
'upvar span', which is just an arbitrary usage of the upvar. If the
upvar is used in multiple places (e.g. a borrow and a move), we may end
up pointing to the borrow. If the overall error is a move error, this
can be confusing.

This PR tracks the span that caused an upvar to become captured by-value
instead of by-ref (assuming that it's not a `move` closure). We use this
span instead of the 'upvar' span when we need to point to an upvar usage
during borrow checking.

src/librustc_middle/ty/mod.rs

@@ -721,7 +721,13 @@ pub enum UpvarCapture<'tcx> {
     /// Upvar is captured by value. This is always true when the
     /// closure is labeled `move`, but can also be true in other cases
     /// depending on inference.
-    ByValue,
+    ///
+    /// If the upvar was inferred to be captured by value (e.g. `move`
+    /// was not used), then the `Span` points to a usage that
+    /// required it. There may be more than one such usage
+    /// (e.g. `|| { a; a; }`), in which case we pick an
+    /// arbitrary one.
+    ByValue(Option<Span>),
 
     /// Upvar is captured by reference.
     ByRef(UpvarBorrow<'tcx>),

src/librustc_mir/borrow_check/diagnostics/mod.rs

@@ -938,19 +938,38 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             "closure_span: def_id={:?} target_place={:?} places={:?}",
             def_id, target_place, places
         );
-        let hir_id = self.infcx.tcx.hir().local_def_id_to_hir_id(def_id.as_local()?);
+        let local_did = def_id.as_local()?;
+        let hir_id = self.infcx.tcx.hir().local_def_id_to_hir_id(local_did);
         let expr = &self.infcx.tcx.hir().expect_expr(hir_id).kind;
         debug!("closure_span: hir_id={:?} expr={:?}", hir_id, expr);
         if let hir::ExprKind::Closure(.., body_id, args_span, _) = expr {
-            for (upvar, place) in self.infcx.tcx.upvars_mentioned(def_id)?.values().zip(places) {
+            for ((upvar_hir_id, upvar), place) in
+                self.infcx.tcx.upvars_mentioned(def_id)?.iter().zip(places)
+            {
                 match place {
                     Operand::Copy(place) | Operand::Move(place)
                         if target_place == place.as_ref() =>
                     {
                         debug!("closure_span: found captured local {:?}", place);
                         let body = self.infcx.tcx.hir().body(*body_id);
                         let generator_kind = body.generator_kind();
-                        return Some((*args_span, generator_kind, upvar.span));
+                        let upvar_id = ty::UpvarId {
+                            var_path: ty::UpvarPath { hir_id: *upvar_hir_id },
+                            closure_expr_id: local_did,
+                        };
+
+                        // If we have a more specific span available, point to that.
+                        // We do this even though this span might be part of a borrow error
+                        // message rather than a move error message. Our goal is to point
+                        // to a span that shows why the upvar is used in the closure,
+                        // so a move-related span is as good as any (and potentially better,
+                        // if the overall error is due to a move of the upvar).
+                        let usage_span =
+                            match self.infcx.tcx.typeck(local_did).upvar_capture(upvar_id) {
+                                ty::UpvarCapture::ByValue(Some(span)) => span,
+                                _ => upvar.span,
+                            };
+                        return Some((*args_span, generator_kind, usage_span));
                     }
                     _ => {}
                 }

src/librustc_mir/borrow_check/mod.rs

@@ -163,7 +163,7 @@ fn do_mir_borrowck<'a, 'tcx>(
             let var_hir_id = upvar_id.var_path.hir_id;
             let capture = tables.upvar_capture(*upvar_id);
             let by_ref = match capture {
-                ty::UpvarCapture::ByValue => false,
+                ty::UpvarCapture::ByValue(_) => false,
                 ty::UpvarCapture::ByRef(..) => true,
             };
             let mut upvar = Upvar {

src/librustc_mir_build/build/mod.rs

@@ -876,7 +876,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     let mut projs = closure_env_projs.clone();
                     projs.push(ProjectionElem::Field(Field::new(i), ty));
                     match capture {
-                        ty::UpvarCapture::ByValue => {}
+                        ty::UpvarCapture::ByValue(_) => {}
                         ty::UpvarCapture::ByRef(..) => {
                             projs.push(ProjectionElem::Deref);
                         }

src/librustc_mir_build/thir/cx/expr.rs

@@ -959,7 +959,7 @@ fn convert_var<'tcx>(
             // ...but the upvar might be an `&T` or `&mut T` capture, at which
             // point we need an implicit deref
             match cx.typeck_results().upvar_capture(upvar_id) {
-                ty::UpvarCapture::ByValue => field_kind,
+                ty::UpvarCapture::ByValue(_) => field_kind,
                 ty::UpvarCapture::ByRef(borrow) => ExprKind::Deref {
                     arg: Expr {
                         temp_lifetime,
@@ -1074,7 +1074,7 @@ fn capture_upvar<'tcx>(
         kind: convert_var(cx, closure_expr, var_hir_id),
     };
     match upvar_capture {
-        ty::UpvarCapture::ByValue => captured_var.to_ref(),
+        ty::UpvarCapture::ByValue(_) => captured_var.to_ref(),
         ty::UpvarCapture::ByRef(upvar_borrow) => {
             let borrow_kind = match upvar_borrow.kind {
                 ty::BorrowKind::ImmBorrow => BorrowKind::Shared,

src/librustc_passes/liveness.rs

@@ -945,7 +945,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
                         let var = self.variable(var_hir_id, upvar.span);
                         self.acc(self.s.exit_ln, var, ACC_READ | ACC_USE);
                     }
-                    ty::UpvarCapture::ByValue => {}
+                    ty::UpvarCapture::ByValue(_) => {}
                 }
             }
         }
@@ -1610,7 +1610,7 @@ impl<'tcx> Liveness<'_, 'tcx> {
                 closure_expr_id: self.ir.body_owner,
             };
             match self.typeck_results.upvar_capture(upvar_id) {
-                ty::UpvarCapture::ByValue => {}
+                ty::UpvarCapture::ByValue(_) => {}
                 ty::UpvarCapture::ByRef(..) => continue,
             };
             if self.used_on_entry(entry_ln, var) {

src/librustc_typeck/check/regionck.rs

@@ -786,7 +786,7 @@ impl<'a, 'tcx> RegionCtxt<'a, 'tcx> {
                     return;
                 }
             }
-            ty::UpvarCapture::ByValue => {}
+            ty::UpvarCapture::ByValue(_) => {}
         }
         let fn_hir_id = self.tcx.hir().local_def_id_to_hir_id(upvar_id.closure_expr_id);
         let ty = self.resolve_node_type(fn_hir_id);

src/librustc_typeck/check/upvar.rs

@@ -42,6 +42,7 @@ use rustc_infer::infer::UpvarRegion;
 use rustc_middle::hir::place::{PlaceBase, PlaceWithHirId};
 use rustc_middle::ty::{self, Ty, TyCtxt, UpvarSubsts};
 use rustc_span::{Span, Symbol};
+use std::collections::hash_map::Entry;
 
 impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     pub fn closure_analyze(&self, body: &'tcx hir::Body<'tcx>) {
@@ -124,7 +125,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 closure_captures.insert(var_hir_id, upvar_id);
 
                 let capture_kind = match capture_clause {
-                    hir::CaptureBy::Value => ty::UpvarCapture::ByValue,
+                    hir::CaptureBy::Value => ty::UpvarCapture::ByValue(None),
                     hir::CaptureBy::Ref => {
                         let origin = UpvarRegion(upvar_id, span);
                         let upvar_region = self.next_region_var(origin);
@@ -237,7 +238,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     debug!("var_id={:?} upvar_ty={:?} capture={:?}", var_hir_id, upvar_ty, capture);
 
                     match capture {
-                        ty::UpvarCapture::ByValue => upvar_ty,
+                        ty::UpvarCapture::ByValue(_) => upvar_ty,
                         ty::UpvarCapture::ByRef(borrow) => tcx.mk_ref(
                             borrow.region,
                             ty::TypeAndMut { ty: upvar_ty, mutbl: borrow.kind.to_mutbl_lossy() },
@@ -300,15 +301,43 @@ 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);
+
         // To move out of an upvar, this must be a FnOnce closure
         self.adjust_closure_kind(
             upvar_id.closure_expr_id,
             ty::ClosureKind::FnOnce,
-            tcx.hir().span(place_with_id.hir_id),
+            usage_span,
             var_name(tcx, upvar_id.var_path.hir_id),
         );
 
-        self.adjust_upvar_captures.insert(upvar_id, ty::UpvarCapture::ByValue);
+        // 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);
+        match self.adjust_upvar_captures.entry(upvar_id) {
+            Entry::Occupied(mut e) => {
+                match e.get() {
+                    // We always overwrite `ByRef`, since we require
+                    // that the upvar be available by value.
+                    //
+                    // If we had a previous by-value usage without a specific
+                    // span, use ours instead. Otherwise, keep the first span
+                    // we encountered, since there isn't an obviously better one.
+                    ty::UpvarCapture::ByRef(_) | ty::UpvarCapture::ByValue(None) => {
+                        e.insert(new_capture);
+                    }
+                    _ => {}
+                }
+            }
+            Entry::Vacant(e) => {
+                e.insert(new_capture);
+            }
+        }
     }
 
     /// Indicates that `place_with_id` is being directly mutated (e.g., assigned
@@ -404,7 +433,7 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
         );
 
         match upvar_capture {
-            ty::UpvarCapture::ByValue => {
+            ty::UpvarCapture::ByValue(_) => {
                 // Upvar is already by-value, the strongest criteria.
             }
             ty::UpvarCapture::ByRef(mut upvar_borrow) => {

src/librustc_typeck/check/writeback.rs

@@ -331,7 +331,7 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
     fn visit_upvar_capture_map(&mut self) {
         for (upvar_id, upvar_capture) in self.fcx.typeck_results.borrow().upvar_capture_map.iter() {
             let new_upvar_capture = match *upvar_capture {
-                ty::UpvarCapture::ByValue => ty::UpvarCapture::ByValue,
+                ty::UpvarCapture::ByValue(span) => ty::UpvarCapture::ByValue(span),
                 ty::UpvarCapture::ByRef(ref upvar_borrow) => {
                     ty::UpvarCapture::ByRef(ty::UpvarBorrow {
                         kind: upvar_borrow.kind,

src/librustc_typeck/expr_use_visitor.rs

@@ -559,7 +559,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
                     var_id,
                 ));
                 match upvar_capture {
-                    ty::UpvarCapture::ByValue => {
+                    ty::UpvarCapture::ByValue(_) => {
                         let mode = copy_or_move(&self.mc, &captured_place);
                         self.delegate.consume(&captured_place, mode);
                     }

src/test/ui/moves/issue-75904-move-closure-loop.rs

@@ -0,0 +1,15 @@
+// Regression test for issue #75904
+// Tests that we point at an expression
+// that required the upvar to be moved, rather than just borrowed.
+
+struct NotCopy;
+
+fn main() {
+    let mut a = NotCopy;
+    loop {
+        || { //~ ERROR use of moved value
+            &mut a;
+            a;
+        };
+    }
+}

src/test/ui/moves/issue-75904-move-closure-loop.stderr

@@ -0,0 +1,15 @@
+error[E0382]: use of moved value: `a`
+  --> $DIR/issue-75904-move-closure-loop.rs:10:9
+   |
+LL |     let mut a = NotCopy;
+   |         ----- move occurs because `a` has type `NotCopy`, which does not implement the `Copy` trait
+LL |     loop {
+LL |         || {
+   |         ^^ value moved into closure here, in previous iteration of loop
+LL |             &mut a;
+LL |             a;
+   |             - use occurs due to use in closure
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0382`.