Reborrow &mut on every move

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -229,23 +229,19 @@ impl<'diag, 'tcx> MirBorrowckCtxt<'_, 'diag, 'tcx> {
                     is_loop_move = true;
                 }
 
-                let mut has_suggest_reborrow = false;
                 if !seen_spans.contains(&move_span) {
                     self.suggest_ref_or_clone(
                         mpi,
                         &mut err,
                         move_spans,
                         moved_place.as_ref(),
-                        &mut has_suggest_reborrow,
                         closure,
                     );
 
                     let msg_opt = CapturedMessageOpt {
                         is_partial_move,
                         is_loop_message,
                         is_move_msg,
-                        is_loop_move,
-                        has_suggest_reborrow,
                         maybe_reinitialized_locations_is_empty: maybe_reinitialized_locations
                             .is_empty(),
                     };
@@ -356,7 +352,6 @@ impl<'diag, 'tcx> MirBorrowckCtxt<'_, 'diag, 'tcx> {
         err: &mut Diag<'_>,
         move_spans: UseSpans<'tcx>,
         moved_place: PlaceRef<'tcx>,
-        has_suggest_reborrow: &mut bool,
         moved_or_invoked_closure: bool,
     ) {
         let move_span = match move_spans {
@@ -477,20 +472,6 @@ impl<'diag, 'tcx> MirBorrowckCtxt<'_, 'diag, 'tcx> {
                         None
                     };
 
-                    // If the moved value is a mut reference, it is used in a
-                    // generic function and it's type is a generic param, it can be
-                    // reborrowed to avoid moving.
-                    // for example:
-                    // struct Y(u32);
-                    // x's type is '& mut Y' and it is used in `fn generic<T>(x: T) {}`.
-                    if let ty::Ref(_, _, hir::Mutability::Mut) = ty.kind()
-                        && arg_param.is_some()
-                    {
-                        *has_suggest_reborrow = true;
-                        self.suggest_reborrow(err, expr.span, moved_place);
-                        return;
-                    }
-
                     // If the moved place is used generically by the callee and a reference to it
                     // would still satisfy any bounds on its type, suggest borrowing.
                     if let Some(&param) = arg_param
@@ -632,25 +613,6 @@ impl<'diag, 'tcx> MirBorrowckCtxt<'_, 'diag, 'tcx> {
         }
     }
 
-    pub(crate) fn suggest_reborrow(
-        &self,
-        err: &mut Diag<'_>,
-        span: Span,
-        moved_place: PlaceRef<'tcx>,
-    ) {
-        err.span_suggestion_verbose(
-            span.shrink_to_lo(),
-            format!(
-                "consider creating a fresh reborrow of {} here",
-                self.describe_place(moved_place)
-                    .map(|n| format!("`{n}`"))
-                    .unwrap_or_else(|| "the mutable reference".to_string()),
-            ),
-            "&mut *",
-            Applicability::MachineApplicable,
-        );
-    }
-
     /// If a place is used after being moved as an argument to a function, the function is generic
     /// in that argument, and a reference to the argument's type would still satisfy the function's
     /// bounds, suggest borrowing. This covers, e.g., borrowing an `impl Fn()` argument being passed

compiler/rustc_borrowck/src/diagnostics/mod.rs

@@ -34,7 +34,6 @@ use tracing::debug;
 
 use super::MirBorrowckCtxt;
 use super::borrow_set::BorrowData;
-use crate::LocalMutationIsAllowed;
 use crate::constraints::OutlivesConstraint;
 use crate::nll::ConstraintDescription;
 use crate::session_diagnostics::{
@@ -993,8 +992,6 @@ struct CapturedMessageOpt {
     is_partial_move: bool,
     is_loop_message: bool,
     is_move_msg: bool,
-    is_loop_move: bool,
-    has_suggest_reborrow: bool,
     maybe_reinitialized_locations_is_empty: bool,
 }
 
@@ -1235,8 +1232,6 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
             is_partial_move: is_partial,
             is_loop_message,
             is_move_msg,
-            is_loop_move,
-            has_suggest_reborrow,
             maybe_reinitialized_locations_is_empty,
         } = msg_opt;
         let mut suggested_cloning = false;
@@ -1438,32 +1433,6 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
                             }
                             _ => {}
                         }
-                        // If the moved place was a `&mut` ref, then we can
-                        // suggest to reborrow it where it was moved, so it
-                        // will still be valid by the time we get to the usage.
-                        if let ty::Ref(_, _, hir::Mutability::Mut) =
-                            moved_place.ty(self.body, self.infcx.tcx).ty.kind()
-                        {
-                            // The `&mut *place` reborrow suggestion is `MachineApplicable`, so
-                            // only offer it where `*place` can be borrowed mutably: a value
-                            // captured by an `Fn` closure (held via `&self`) cannot, and the
-                            // suggestion would otherwise fail to compile with E0596.
-                            let reborrow_place = self.infcx.tcx.mk_place_deref(moved_place);
-                            let reborrow_is_valid = self
-                                .is_mutable(reborrow_place.as_ref(), LocalMutationIsAllowed::No)
-                                .is_ok();
-                            // Suggest `reborrow` in other place for following situations:
-                            // 1. If we are in a loop this will be suggested later.
-                            // 2. If the moved value is a mut reference, it is used in a
-                            // generic function and the corresponding arg's type is generic param.
-                            if !is_loop_move && !has_suggest_reborrow && reborrow_is_valid {
-                                self.suggest_reborrow(
-                                    err,
-                                    move_span.shrink_to_lo(),
-                                    moved_place.as_ref(),
-                                );
-                            }
-                        }
                     } else {
                         match desugaring {
                             Some((CallDesugaringKind::Await, _)) => {

compiler/rustc_borrowck/src/diagnostics/move_errors.rs

@@ -552,8 +552,6 @@ impl<'diag, 'tcx> MirBorrowckCtxt<'_, 'diag, 'tcx> {
             is_partial_move: false,
             is_loop_message: false,
             is_move_msg: false,
-            is_loop_move: false,
-            has_suggest_reborrow: false,
             maybe_reinitialized_locations_is_empty: true,
         };
         let suggested_cloning = if let Some(use_spans) = use_spans {

compiler/rustc_hir_typeck/src/coercion.rs

@@ -505,25 +505,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
             }
         };
 
-        if coerced_a == a && mt_a.mutbl.is_not() && autoderef.step_count() == 1 {
-            // As a special case, if we would produce `&'a *x`, that's
-            // a total no-op. We end up with the type `&'a T` just as
-            // we started with. In that case, just skip it altogether.
-            //
-            // Unfortunately, this can actually effect capture analysis
-            // which in turn means this effects borrow checking. This can
-            // also effect diagnostics.
-            // FIXME(BoxyUwU): we should always emit reborrow coercions
-            //
-            // Note that for `&mut`, we DO want to reborrow --
-            // otherwise, this would be a move, which might be an
-            // error. For example `foo(self.x)` where `self` and
-            // `self.x` both have `&mut `type would be a move of
-            // `self.x`, but we auto-coerce it to `foo(&mut *self.x)`,
-            // which is a borrow.
-            assert!(mutbl_b.is_not()); // can only coerce &T -> &U
-            return success(vec![], coerced_a, obligations);
-        }
+        let autoderef_step_count = autoderef.step_count();
 
         let InferOk { value: mut adjustments, obligations: o } =
             self.adjust_steps_as_infer_ok(&autoderef);
@@ -536,10 +518,55 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
             coerced_a
         );
 
-        // Now apply the autoref
-        let mutbl = AutoBorrowMutability::new(mutbl_b, self.allow_two_phase);
-        adjustments
-            .push(Adjustment { kind: Adjust::Borrow(AutoBorrow::Ref(mutbl)), target: coerced_a });
+        // As a special case, if we would produce `&'a *x`, that's
+        // a total no-op. We end up with the type `&'a T` just as
+        // we started with. In that case, just skip it altogether.
+
+        let can_skip_noop_reborrow = if mutbl_b.is_not() {
+            if self.tcx.is_closure_like(self.body_def_id.to_def_id()) {
+                // FIXME: Inside a closure, reborrows can affect capture inference.
+                // So we have to keep the existing elision behavior stable.
+                // Ideally we wouldn't have to do this, as it makes lifetime
+                // annotations affect codegen.
+                // See `tests/ui/coercion/structural_identity_dependent_reborrows.rs`
+                coerced_a == a && autoderef_step_count == 1
+            } else {
+                if matches!(
+                    adjustments.last(),
+                    Some(Adjustment { kind: Adjust::Deref(DerefAdjustKind::Builtin), .. })
+                ) {
+                    // Last adjustment is a builtin deref adjustment.
+                    // Check if that deref was applied to a shared reference
+
+                    let ty_before_deref = if adjustments.len() >= 2 {
+                        *adjustments[adjustments.len() - 2].target.kind()
+                    } else {
+                        ty::Ref(r_a, mt_a.ty, mt_a.mutbl)
+                    };
+
+                    matches!(ty_before_deref, ty::Ref(_, _, ty::Mutability::Not))
+                } else {
+                    // Last adjustment is not a builtin deref adjustment,
+                    // so this can't be a shared ref reborrow.
+                    false
+                }
+            }
+        } else {
+            // FIXME: Ideally we would also skip `&mut` -> `&mut` reborrow,
+            // and handle that in MIR.
+            false
+        };
+
+        if can_skip_noop_reborrow {
+            adjustments.pop();
+        } else {
+            // Now apply the autoref
+            let mutbl = AutoBorrowMutability::new(mutbl_b, self.allow_two_phase);
+            adjustments.push(Adjustment {
+                kind: Adjust::Borrow(AutoBorrow::Ref(mutbl)),
+                target: coerced_a,
+            });
+        }
 
         debug!("coerce_to_ref: succeeded coerced_a={:?} adjustments={:?}", coerced_a, adjustments);
 
@@ -630,6 +657,16 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
 
         // Handle reborrows before selecting `Source: CoerceUnsized<Target>`.
         let reborrow = match (source.kind(), target.kind()) {
+            // Don't emit a no-op shared -> shared reborrow.
+            (&ty::Ref(_, _, ty::Mutability::Not), &ty::Ref(_, _, ty::Mutability::Not))
+                // FIXME: Inside a closure, reborrows can affect capture inference.
+                // So we have to emit them anyway.
+                // Ideally we wouldn't have to do this, it makes lifetime
+                // annotations affect codegen
+                if !self.tcx.is_closure_like(self.body_def_id.to_def_id()) =>
+            {
+                None
+            }
             (&ty::Ref(_, ty_a, mutbl_a), &ty::Ref(_, _, mutbl_b)) => {
                 coerce_mutbls(mutbl_a, mutbl_b)?;
 

compiler/rustc_middle/src/mir/syntax.rs

@@ -1479,9 +1479,10 @@ pub enum Rvalue<'tcx> {
     /// Future work may add support for multiple lifetimes and changing memory layout as part of
     /// CoerceShared. These may be end up implemented as multiple MIR operations.
     ///
-    /// This is produced by the [`ExprKind::Reborrow`].
+    /// This is produced by [`ExprKind::CoerceShared`],
+    /// and by `consume_by_copy_reborrow_or_move()` in `rustc_mir_build`.
     ///
-    /// [`ExprKind::Reborrow`]: crate::thir::ExprKind::Reborrow
+    /// [`ExprKind::CoerceShared`]: crate::thir::ExprKind::CoerceShared
     Reborrow(Ty<'tcx>, Mutability, Place<'tcx>),
 }
 

compiler/rustc_middle/src/thir.rs

@@ -554,16 +554,15 @@ pub enum ExprKind<'tcx> {
     Yield {
         value: ExprId,
     },
-    /// Use of an ADT that implements the Reborrow (for Mut) or CoerceShared traits (for Not). This
+    /// Use of an ADT that implements the CoerceShared trait. This
     /// expression is produced by the [`Adjust::GenericReborrow`] in places where normally the ADT
     /// would be moved or assigned over. Instead, this produces an [`Rvalue::Reborrow`] which
     /// produces a bitwise copy of the source ADT and disables the source for the copy's lifetime.
     ///
     /// [`Adjust::GenericReborrow`]: crate::ty::adjustment::Adjust::GenericReborrow
     /// [`Rvalue::Reborrow`]: mir::Rvalue::Reborrow
-    Reborrow {
+    CoerceShared {
         source: ExprId,
-        mutability: Mutability,
         target: Ty<'tcx>,
     },
 }

compiler/rustc_middle/src/thir/visit.rs

@@ -187,9 +187,7 @@ pub fn walk_expr<'thir, 'tcx: 'thir, V: Visitor<'thir, 'tcx>>(
         }
         ThreadLocalRef(_) => {}
         Yield { value } => visitor.visit_expr(&visitor.thir()[value]),
-        Reborrow { source, mutability: _, target: _ } => {
-            visitor.visit_expr(&visitor.thir()[source])
-        }
+        CoerceShared { source, target: _ } => visitor.visit_expr(&visitor.thir()[source]),
     }
 }
 

compiler/rustc_middle/src/ty/adjustment.rs

@@ -107,9 +107,10 @@ pub enum Adjust {
     /// Take a user-type T implementing the Reborrow trait (for Mut) or the CoerceShared trait (for
     /// Not) and reborrow as `T` or `CoreceShared<U>`.
     ///
-    /// This produces an [`ExprKind::Reborrow`].
+    /// Thu Not form produces an [`ExprKind::CoerceShared`]; the Mut one produces no THIR,
+    /// and exists only for closure capture inference.
     ///
-    /// [`ExprKind::Reborrow`]: crate::thir::ExprKind::Reborrow
+    /// [`ExprKind::CoerceShared`]: crate::thir::ExprKind::CoerceShared
     GenericReborrow(hir::Mutability),
 }
 

compiler/rustc_mir_build/src/builder/expr/as_place.rs

@@ -587,7 +587,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             // A reborrow is an rvalue. If a place is needed for it, materialize
             // the rvalue in a temporary instead of treating the reborrow
             // expression itself as an assignable place.
-            | ExprKind::Reborrow { .. }
+            | ExprKind::CoerceShared { .. }
             | ExprKind::WrapUnsafeBinder { .. } => {
                 // these are not places, so we need to make a temporary.
                 debug_assert!(!matches!(Category::of(&expr.kind), Some(Category::Place)));

compiler/rustc_mir_build/src/builder/expr/as_rvalue.rs

@@ -293,7 +293,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             // by reference captures use as_operand
                             Some(Category::Place) => {
                                 let place = unpack!(block = this.as_place(block, upvar));
-                                this.consume_by_copy_or_move(place)
+                                this.consume_by_copy_reborrow_or_move(place, block, upvar_expr.span)
                             }
                             _ => {
                                 // Turn mutable borrow captures into unique
@@ -434,9 +434,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 );
                 block.and(Rvalue::Use(operand, WithRetag::Yes))
             }
-            ExprKind::Reborrow { source, mutability, target } => {
-                let temp = unpack!(block = this.as_temp(block, scope, source, mutability));
-                block.and(Rvalue::Reborrow(target, mutability, temp.into()))
+            ExprKind::CoerceShared { source, target } => {
+                let temp = unpack!(block = this.as_temp(block, scope, source, Mutability::Not));
+                block.and(Rvalue::Reborrow(target, Mutability::Not, temp.into()))
             }
         }
     }

compiler/rustc_mir_build/src/builder/expr/category.rs

@@ -72,7 +72,7 @@ impl Category {
             // A reborrow expression produces a value represented in MIR as
             // `Rvalue::Reborrow`. Its source may be a place, but the reborrow
             // expression itself does not denote an assignable place.
-            | ExprKind::Reborrow { .. }
+            | ExprKind::CoerceShared { .. }
             | ExprKind::ThreadLocalRef(_)
             | ExprKind::WrapUnsafeBinder { .. } => Some(Category::Rvalue(RvalueFunc::AsRvalue)),