From b5b8b9329b3a118879bad6c476612d49f89461a3 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 25 Aug 2020 23:58:58 -0400 Subject: [PATCH] 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 | 8 +++- .../borrow_check/diagnostics/mod.rs | 25 ++++++++++-- src/librustc_mir/borrow_check/mod.rs | 2 +- src/librustc_mir_build/build/mod.rs | 2 +- src/librustc_mir_build/thir/cx/expr.rs | 4 +- src/librustc_passes/liveness.rs | 4 +- src/librustc_typeck/check/regionck.rs | 2 +- src/librustc_typeck/check/upvar.rs | 39 ++++++++++++++++--- src/librustc_typeck/check/writeback.rs | 2 +- src/librustc_typeck/expr_use_visitor.rs | 2 +- .../ui/moves/issue-75904-move-closure-loop.rs | 15 +++++++ .../issue-75904-move-closure-loop.stderr | 15 +++++++ 12 files changed, 102 insertions(+), 18 deletions(-) create mode 100644 src/test/ui/moves/issue-75904-move-closure-loop.rs create mode 100644 src/test/ui/moves/issue-75904-move-closure-loop.stderr diff --git a/src/librustc_middle/ty/mod.rs b/src/librustc_middle/ty/mod.rs index a961d02f7a2b1..a4e54c5e31bf5 100644 --- a/src/librustc_middle/ty/mod.rs +++ b/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), /// Upvar is captured by reference. ByRef(UpvarBorrow<'tcx>), diff --git a/src/librustc_mir/borrow_check/diagnostics/mod.rs b/src/librustc_mir/borrow_check/diagnostics/mod.rs index e73a78811d490..dfaa75d9f23f8 100644 --- a/src/librustc_mir/borrow_check/diagnostics/mod.rs +++ b/src/librustc_mir/borrow_check/diagnostics/mod.rs @@ -938,11 +938,14 @@ 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() => @@ -950,7 +953,23 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { 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)); } _ => {} } diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index a61af5c3f0523..fae2af6f2bbdb 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/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 { diff --git a/src/librustc_mir_build/build/mod.rs b/src/librustc_mir_build/build/mod.rs index 71026f5096df6..249cce0ba1994 100644 --- a/src/librustc_mir_build/build/mod.rs +++ b/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); } diff --git a/src/librustc_mir_build/thir/cx/expr.rs b/src/librustc_mir_build/thir/cx/expr.rs index c51c3bcf56288..066e46fec1456 100644 --- a/src/librustc_mir_build/thir/cx/expr.rs +++ b/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, diff --git a/src/librustc_passes/liveness.rs b/src/librustc_passes/liveness.rs index de21f0b5e09b3..55525586479d9 100644 --- a/src/librustc_passes/liveness.rs +++ b/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) { diff --git a/src/librustc_typeck/check/regionck.rs b/src/librustc_typeck/check/regionck.rs index 221e5f72dc977..484961dbdb8f2 100644 --- a/src/librustc_typeck/check/regionck.rs +++ b/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); diff --git a/src/librustc_typeck/check/upvar.rs b/src/librustc_typeck/check/upvar.rs index 030c0ab668a80..9bb84c0786847 100644 --- a/src/librustc_typeck/check/upvar.rs +++ b/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) => { diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 50e2d6a94bb71..67f67e64dd47b 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/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, diff --git a/src/librustc_typeck/expr_use_visitor.rs b/src/librustc_typeck/expr_use_visitor.rs index d1b386c9d4d47..e774f2d095d9c 100644 --- a/src/librustc_typeck/expr_use_visitor.rs +++ b/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); } diff --git a/src/test/ui/moves/issue-75904-move-closure-loop.rs b/src/test/ui/moves/issue-75904-move-closure-loop.rs new file mode 100644 index 0000000000000..6641a0376c6ae --- /dev/null +++ b/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; + }; + } +} diff --git a/src/test/ui/moves/issue-75904-move-closure-loop.stderr b/src/test/ui/moves/issue-75904-move-closure-loop.stderr new file mode 100644 index 0000000000000..5e427a1fcdc7d --- /dev/null +++ b/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`.