Rollup merge of #87069 - sexxi-goose:copy_ref_always, r=nikomatsakis

ExprUseVisitor: Treat ByValue use of Copy types as ImmBorrow

r? ``@nikomatsakis``

Author: GuillaumeGomez

compiler/rustc_typeck/src/check/upvar.rs

@@ -1528,20 +1528,11 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
         &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={:?}, diag_expr_id={:?}, mode={:?})",
-            place_with_id, diag_expr_id, mode
+            "adjust_upvar_borrow_kind_for_consume(place_with_id={:?}, diag_expr_id={:?})",
+            place_with_id, diag_expr_id
         );
-
-        // Copy type being used as ByValue are equivalent to ImmBorrow and don't require any
-        // escalation.
-        match mode {
-            euv::ConsumeMode::Copy => return,
-            euv::ConsumeMode::Move => {}
-        };
-
         let tcx = self.fcx.tcx;
         let upvar_id = if let PlaceBase::Upvar(upvar_id) = place_with_id.place.base {
             upvar_id
@@ -1716,22 +1707,14 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
         }
     }
 
-    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
-        );
+    fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) {
+        debug!("consume(place_with_id={:?}, diag_expr_id={:?})", place_with_id, diag_expr_id);
 
         if !self.capture_information.contains_key(&place_with_id.place) {
             self.init_capture_info_for_place(&place_with_id, diag_expr_id);
         }
 
-        self.adjust_upvar_borrow_kind_for_consume(&place_with_id, diag_expr_id, mode);
+        self.adjust_upvar_borrow_kind_for_consume(&place_with_id, diag_expr_id);
     }
 
     fn borrow(

compiler/rustc_typeck/src/expr_use_visitor.rs

@@ -2,8 +2,6 @@
 //! normal visitor, which just walks the entire body in one shot, the
 //! `ExprUseVisitor` determines how expressions are being used.
 
-pub use self::ConsumeMode::*;
-
 // Export these here so that Clippy can use them.
 pub use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, Projection};
 
@@ -28,19 +26,20 @@ use crate::mem_categorization as mc;
 /// This trait defines the callbacks you can expect to receive when
 /// employing the ExprUseVisitor.
 pub trait Delegate<'tcx> {
-    // The value found at `place` is either copied or moved, depending
+    // The value found at `place` is moved, depending
     // on `mode`. Where `diag_expr_id` is the id used for diagnostics for `place`.
     //
+    // Use of a `Copy` type in a ByValue context is considered a use
+    // by `ImmBorrow` and `borrow` is called instead. This is because
+    // a shared borrow is the "minimum access" that would be needed
+    // to perform a copy.
+    //
+    //
     // 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,
-    );
+    fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId);
 
     // The value found at `place` is being borrowed with kind `bk`.
     // `diag_expr_id` is the id used for diagnostics (see `consume` for more details).
@@ -60,7 +59,7 @@ pub trait Delegate<'tcx> {
 }
 
 #[derive(Copy, Clone, PartialEq, Debug)]
-pub enum ConsumeMode {
+enum ConsumeMode {
     Copy, // reference to x where x has a type that copies
     Move, // reference to x where x has a type that moves
 }
@@ -141,10 +140,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, '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, diag_expr_id, mode);
+        delegate_consume(&self.mc, self.delegate, place_with_id, diag_expr_id)
     }
 
     fn consume_exprs(&mut self, exprs: &[hir::Expr<'_>]) {
@@ -653,9 +649,8 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
                             delegate.borrow(place, discr_place.hir_id, bk);
                         }
                         ty::BindByValue(..) => {
-                            let mode = copy_or_move(mc, &place);
                             debug!("walk_pat binding consuming pat");
-                            delegate.consume(place, discr_place.hir_id, mode);
+                            delegate_consume(mc, *delegate, place, discr_place.hir_id);
                         }
                     }
                 }
@@ -773,8 +768,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
 
                     match capture_info.capture_kind {
                         ty::UpvarCapture::ByValue(_) => {
-                            let mode = copy_or_move(&self.mc, &place_with_id);
-                            self.delegate.consume(&place_with_id, place_with_id.hir_id, mode);
+                            self.delegate_consume(&place_with_id, place_with_id.hir_id);
                         }
                         ty::UpvarCapture::ByRef(upvar_borrow) => {
                             self.delegate.borrow(
@@ -798,8 +792,28 @@ fn copy_or_move<'a, 'tcx>(
         place_with_id.place.ty(),
         mc.tcx().hir().span(place_with_id.hir_id),
     ) {
-        Move
+        ConsumeMode::Move
     } else {
-        Copy
+        ConsumeMode::Copy
+    }
+}
+
+// - If a place is used in a `ByValue` context then move it if it's not a `Copy` type.
+// - If the place that is a `Copy` type consider it a `ImmBorrow`.
+fn delegate_consume<'a, 'tcx>(
+    mc: &mc::MemCategorizationContext<'a, 'tcx>,
+    delegate: &mut (dyn Delegate<'tcx> + 'a),
+    place_with_id: &PlaceWithHirId<'tcx>,
+    diag_expr_id: hir::HirId,
+) {
+    debug!("delegate_consume(place_with_id={:?})", place_with_id);
+
+    let mode = copy_or_move(&mc, place_with_id);
+
+    match mode {
+        ConsumeMode::Move => delegate.consume(place_with_id, diag_expr_id),
+        ConsumeMode::Copy => {
+            delegate.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow)
+        }
     }
 }

src/test/ui/closures/2229_closure_analysis/move_closure.rs

@@ -195,6 +195,21 @@ fn box_mut_2() {
     //~| NOTE: Min Capture p_foo[Deref,Deref,(0, 0)] -> UniqueImmBorrow
 }
 
+// Test that move closures can take ownership of Copy type
+fn returned_closure_owns_copy_type_data() -> impl Fn() -> i32 {
+    let x = 10;
+
+    let c = #[rustc_capture_analysis] move || x;
+    //~^ ERROR: attributes on expressions are experimental
+    //~| NOTE: see issue #15701 <https://github.com/rust-lang/rust/issues/15701>
+    //~| First Pass analysis includes:
+    //~| NOTE: Capturing x[] -> ImmBorrow
+    //~| Min Capture analysis includes:
+    //~| NOTE: Min Capture x[] -> ByValue
+
+    c
+}
+
 fn main() {
     simple_move_closure();
     simple_ref();

src/test/ui/closures/2229_closure_analysis/move_closure.stderr

@@ -88,6 +88,39 @@ LL |     let c = #[rustc_capture_analysis] move || p_foo.x += 10;
    = note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information
    = help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable
 
+error[E0658]: attributes on expressions are experimental
+  --> $DIR/move_closure.rs:202:13
+   |
+LL |     let c = #[rustc_capture_analysis] move || x;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information
+   = help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable
+
+error: First Pass analysis includes:
+  --> $DIR/move_closure.rs:202:39
+   |
+LL |     let c = #[rustc_capture_analysis] move || x;
+   |                                       ^^^^^^^^^
+   |
+note: Capturing x[] -> ImmBorrow
+  --> $DIR/move_closure.rs:202:47
+   |
+LL |     let c = #[rustc_capture_analysis] move || x;
+   |                                               ^
+
+error: Min Capture analysis includes:
+  --> $DIR/move_closure.rs:202:39
+   |
+LL |     let c = #[rustc_capture_analysis] move || x;
+   |                                       ^^^^^^^^^
+   |
+note: Min Capture x[] -> ByValue
+  --> $DIR/move_closure.rs:202:47
+   |
+LL |     let c = #[rustc_capture_analysis] move || x;
+   |                                               ^
+
 error: First Pass analysis includes:
   --> $DIR/move_closure.rs:15:5
    |
@@ -424,6 +457,6 @@ note: Min Capture p_foo[Deref,Deref,(0, 0)] -> UniqueImmBorrow
 LL |     let c = #[rustc_capture_analysis] move || p_foo.x += 10;
    |                                               ^^^^^^^
 
-error: aborting due to 30 previous errors
+error: aborting due to 33 previous errors
 
 For more information about this error, try `rustc --explain E0658`.

src/test/ui/closures/2229_closure_analysis/run_pass/move_closure.rs

@@ -3,6 +3,8 @@
 
 // Test that move closures compile properly with `capture_disjoint_fields` enabled.
 
+#![allow(unused)]
+
 fn simple_ref() {
     let mut s = 10;
     let ref_s = &mut s;
@@ -92,6 +94,15 @@ fn data_moved_but_not_fn_once() {
     c();
 }
 
+// Test that move closures can take ownership of Copy type
+fn returned_closure_owns_copy_type_data() -> impl Fn() -> i32 {
+    let x = 10;
+
+    let c = move || x;
+
+    c
+}
+
 fn main() {
     simple_ref();
     struct_contains_ref_to_another_struct();
@@ -100,4 +111,6 @@ fn main() {
 
     disjoint_via_ref();
     data_moved_but_not_fn_once();
+
+    returned_closure_owns_copy_type_data();
 }

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

@@ -11,7 +11,7 @@ use rustc_span::source_map::Span;
 use rustc_span::symbol::kw;
 use rustc_target::abi::LayoutOf;
 use rustc_target::spec::abi::Abi;
-use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
+use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 
 #[derive(Copy, Clone)]
 pub struct BoxedLocal {
@@ -133,13 +133,10 @@ 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>, _: HirId, mode: ConsumeMode) {
+    fn consume(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) {
         if cmt.place.projections.is_empty() {
             if let PlaceBase::Local(lid) = cmt.place.base {
-                if let ConsumeMode::Move = mode {
-                    // moved out or in. clearly can't be localized
-                    self.set.remove(&lid);
-                }
+                self.set.remove(&lid);
                 let map = &self.cx.tcx.hir();
                 if let Some(Node::Binding(_)) = map.find(cmt.hir_id) {
                     if self.set.contains(&lid) {

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

@@ -7,7 +7,7 @@ use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
 use rustc_middle::{mir::FakeReadCause, ty};
 use rustc_span::source_map::Span;
-use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
+use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 
 pub(super) fn check(cx: &LateContext<'_>, arg: &Expr<'_>, body: &Expr<'_>) {
     if let Some(higher::Range {
@@ -82,7 +82,7 @@ struct MutatePairDelegate<'a, 'tcx> {
 }
 
 impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> {
-    fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: ConsumeMode) {}
+    fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
 
     fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) {
         if let ty::BorrowKind::MutBorrow = bk {

src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs

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

src/tools/clippy/clippy_utils/src/usage.rs

@@ -10,7 +10,7 @@ use rustc_lint::LateContext;
 use rustc_middle::hir::map::Map;
 use rustc_middle::mir::FakeReadCause;
 use rustc_middle::ty;
-use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
+use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 
 /// Returns a set of mutated local variable IDs, or `None` if mutations could not be determined.
 pub fn mutated_variables<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) -> Option<HirIdSet> {
@@ -67,7 +67,7 @@ impl<'tcx> MutVarsDelegate {
 }
 
 impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
-    fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: ConsumeMode) {}
+    fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
 
     fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, bk: ty::BorrowKind) {
         if let ty::BorrowKind::MutBorrow = bk {