Fix deref field pattern suggestions and improve error messages

Author: chenyukang

compiler/rustc_borrowck/src/diagnostics/move_errors.rs

@@ -94,6 +94,12 @@ enum GroupedMoveError<'tcx> {
     },
 }
 
+struct PatternBindingInfo {
+    pat_span: Span,
+    binding_spans: Vec<Span>,
+    has_mutable_by_value_binding: bool,
+}
+
 impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
     pub(crate) fn report_move_errors(&mut self) {
         let grouped_errors = self.group_move_errors();
@@ -678,8 +684,11 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
     fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span: Span) {
         match error {
             GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => {
-                self.add_borrow_suggestions(err, span, !binds_to.is_empty());
+                binds_to.sort();
+                binds_to.dedup();
+
                 if binds_to.is_empty() {
+                    self.add_borrow_suggestions(err, span, false);
                     let place_ty = move_from.ty(self.body, self.infcx.tcx).ty;
                     let place_desc = match self.describe_place(move_from.as_ref()) {
                         Some(desc) => format!("`{desc}`"),
@@ -697,16 +706,30 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
                         span,
                     });
                 } else {
-                    binds_to.sort();
-                    binds_to.dedup();
-
-                    self.add_move_error_details(err, &binds_to, &[]);
+                    let binding_info = self.pattern_binding_info(&binds_to);
+                    let suggest_pattern_binding = binding_info.as_ref().is_some_and(|info| {
+                        self.should_suggest_pattern_binding_instead(span, info)
+                    });
+                    let desugar_spans = if suggest_pattern_binding {
+                        self.add_move_error_suggestions(err, &binds_to)
+                    } else {
+                        if self.should_suggest_borrow_instead(span, binding_info.as_ref()) {
+                            self.add_borrow_suggestions(err, span, true);
+                        }
+                        None
+                    };
+                    self.add_move_error_details(
+                        err,
+                        &binds_to,
+                        desugar_spans.as_deref().unwrap_or_default(),
+                    );
                 }
             }
             GroupedMoveError::MovesFromValue { mut binds_to, .. } => {
                 binds_to.sort();
                 binds_to.dedup();
-                let desugar_spans = self.add_move_error_suggestions(err, &binds_to);
+                let desugar_spans =
+                    self.add_move_error_suggestions(err, &binds_to).unwrap_or_default();
                 self.add_move_error_details(err, &binds_to, &desugar_spans);
             }
             // No binding. Nothing to suggest.
@@ -861,7 +884,102 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         }
     }
 
-    fn add_move_error_suggestions(&self, err: &mut Diag<'_>, binds_to: &[Local]) -> Vec<Span> {
+    fn should_suggest_pattern_binding_instead(
+        &self,
+        span: Span,
+        binding_info: &PatternBindingInfo,
+    ) -> bool {
+        let Some(expr) = self.find_expr(span) else {
+            return false;
+        };
+
+        let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
+        let projection_qualifies = match expr.kind {
+            hir::ExprKind::Field(base, ..) => {
+                !typeck_results.node_type_opt(base.hir_id).is_some_and(|base_ty| {
+                    binding_info.has_mutable_by_value_binding
+                        && matches!(base_ty.kind(), ty::Ref(_, _, hir::Mutability::Not))
+                })
+            }
+            hir::ExprKind::Index(base, ..) => typeck_results
+                .node_type_opt(base.hir_id)
+                .is_some_and(|base_ty| match base_ty.kind() {
+                    ty::Ref(_, _, hir::Mutability::Not) | ty::RawPtr(..) => false,
+                    ty::Ref(_, _, hir::Mutability::Mut) => {
+                        binding_info.has_mutable_by_value_binding
+                    }
+                    _ => true,
+                }),
+            _ => false,
+        };
+        if !projection_qualifies {
+            return false;
+        }
+
+        let is_single_binding = binding_info.binding_spans.len() == 1
+            && binding_info.binding_spans[0] == binding_info.pat_span;
+        !is_single_binding
+    }
+
+    fn should_suggest_borrow_instead(
+        &self,
+        span: Span,
+        binding_info: Option<&PatternBindingInfo>,
+    ) -> bool {
+        if !binding_info.is_some_and(|info| info.has_mutable_by_value_binding) {
+            return true;
+        }
+
+        let Some(expr) = self.find_expr(span) else {
+            return true;
+        };
+
+        let Some(base) = (match expr.kind {
+            hir::ExprKind::Field(base, _) | hir::ExprKind::Index(base, ..) => Some(base),
+            _ => None,
+        }) else {
+            return true;
+        };
+
+        !self
+            .infcx
+            .tcx
+            .typeck(self.mir_def_id())
+            .node_type_opt(base.hir_id)
+            .is_some_and(|base_ty| matches!(base_ty.kind(), ty::Ref(_, _, hir::Mutability::Not)))
+    }
+
+    fn pattern_binding_info(&self, binds_to: &[Local]) -> Option<PatternBindingInfo> {
+        let mut pat_span = None;
+        let mut binding_spans = Vec::new();
+        let mut has_mutable_by_value_binding = false;
+        for local in binds_to {
+            let bind_to = &self.body.local_decls[*local];
+            if let LocalInfo::User(BindingForm::Var(VarBindingForm {
+                pat_span: pat_sp,
+                binding_mode,
+                ..
+            })) = *bind_to.local_info()
+            {
+                pat_span = Some(pat_sp);
+                binding_spans.push(bind_to.source_info.span);
+                has_mutable_by_value_binding |=
+                    matches!(binding_mode, hir::BindingMode(hir::ByRef::No, hir::Mutability::Mut));
+            }
+        }
+
+        Some(PatternBindingInfo {
+            pat_span: pat_span?,
+            binding_spans,
+            has_mutable_by_value_binding,
+        })
+    }
+
+    fn add_move_error_suggestions(
+        &self,
+        err: &mut Diag<'_>,
+        binds_to: &[Local],
+    ) -> Option<Vec<Span>> {
         /// A HIR visitor to associate each binding with a `&` or `&mut` that could be removed to
         /// make it bind by reference instead (if possible)
         struct BindingFinder<'tcx> {
@@ -964,27 +1082,20 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
                 self.has_adjustments = parent_has_adjustments;
             }
         }
-        let mut pat_span = None;
-        let mut binding_spans = Vec::new();
-        for local in binds_to {
-            let bind_to = &self.body.local_decls[*local];
-            if let LocalInfo::User(BindingForm::Var(VarBindingForm { pat_span: pat_sp, .. })) =
-                *bind_to.local_info()
-            {
-                pat_span = Some(pat_sp);
-                binding_spans.push(bind_to.source_info.span);
-            }
-        }
-        let Some(pat_span) = pat_span else { return Vec::new() };
+        let Some(binding_info) = self.pattern_binding_info(binds_to) else {
+            return None;
+        };
 
         let tcx = self.infcx.tcx;
-        let Some(body) = tcx.hir_maybe_body_owned_by(self.mir_def_id()) else { return Vec::new() };
+        let Some(body) = tcx.hir_maybe_body_owned_by(self.mir_def_id()) else {
+            return None;
+        };
         let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
         let mut finder = BindingFinder {
             typeck_results,
             tcx,
-            pat_span,
-            binding_spans,
+            pat_span: binding_info.pat_span,
+            binding_spans: binding_info.binding_spans,
             found_pat: false,
             ref_pat: None,
             has_adjustments: false,
@@ -1015,7 +1126,8 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         for (span, msg, suggestion) in suggestions {
             err.span_suggestion_verbose(span, msg, suggestion, Applicability::MachineApplicable);
         }
-        finder.desugar_binding_spans
+
+        Some(finder.desugar_binding_spans)
     }
 
     fn add_move_error_details(

tests/ui/binding/issue-40402-2.stderr

@@ -8,10 +8,14 @@ LL |     let (a, b) = x[0];
    |          data moved here
    |
    = note: move occurs because these variables have types that don't implement the `Copy` trait
-help: consider borrowing here
+help: consider borrowing the pattern binding
    |
-LL |     let (a, b) = &x[0];
-   |                  +
+LL |     let (ref a, b) = x[0];
+   |          +++
+help: consider borrowing the pattern binding
+   |
+LL |     let (a, ref b) = x[0];
+   |             +++
 
 error: aborting due to 1 previous error
 

tests/ui/borrowck/deref-field-pattern-ref-suggestion-issue-146995.fixed

@@ -0,0 +1,40 @@
+//@ run-rustfix
+#![allow(dead_code)]
+
+use std::ops::{Deref, DerefMut};
+
+fn take(mut wrap: Wrap<Struct>) {
+    if let Some(ref mut val) = wrap.field {
+        //~^ ERROR cannot move out of dereference of `Wrap<Struct>`
+        val.0 = ();
+    }
+}
+
+fn take_mut_ref_base(mut wrap: Wrap<Struct>) {
+    if let Some(ref mut val) = (&mut wrap).field {
+        //~^ ERROR cannot move out of dereference of `Wrap<Struct>`
+        val.0 = ();
+    }
+}
+
+struct Wrap<T>(T);
+struct Struct {
+    field: Option<NonCopy>,
+}
+struct NonCopy(());
+
+impl<T> Deref for Wrap<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<T> DerefMut for Wrap<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
+fn main() {}

tests/ui/borrowck/deref-field-pattern-ref-suggestion-issue-146995.rs

@@ -0,0 +1,40 @@
+//@ run-rustfix
+#![allow(dead_code)]
+
+use std::ops::{Deref, DerefMut};
+
+fn take(mut wrap: Wrap<Struct>) {
+    if let Some(mut val) = wrap.field {
+        //~^ ERROR cannot move out of dereference of `Wrap<Struct>`
+        val.0 = ();
+    }
+}
+
+fn take_mut_ref_base(mut wrap: Wrap<Struct>) {
+    if let Some(mut val) = (&mut wrap).field {
+        //~^ ERROR cannot move out of dereference of `Wrap<Struct>`
+        val.0 = ();
+    }
+}
+
+struct Wrap<T>(T);
+struct Struct {
+    field: Option<NonCopy>,
+}
+struct NonCopy(());
+
+impl<T> Deref for Wrap<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<T> DerefMut for Wrap<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
+fn main() {}

tests/ui/borrowck/deref-field-pattern-ref-suggestion-issue-146995.stderr

@@ -0,0 +1,29 @@
+error[E0507]: cannot move out of dereference of `Wrap<Struct>`
+  --> $DIR/deref-field-pattern-ref-suggestion-issue-146995.rs:7:28
+   |
+LL |     if let Some(mut val) = wrap.field {
+   |                 -------    ^^^^^^^^^^
+   |                 |
+   |                 data moved here because `val` has type `NonCopy`, which does not implement the `Copy` trait
+   |
+help: consider borrowing the pattern binding
+   |
+LL |     if let Some(ref mut val) = wrap.field {
+   |                 +++
+
+error[E0507]: cannot move out of dereference of `Wrap<Struct>`
+  --> $DIR/deref-field-pattern-ref-suggestion-issue-146995.rs:14:28
+   |
+LL |     if let Some(mut val) = (&mut wrap).field {
+   |                 -------    ^^^^^^^^^^^^^^^^^
+   |                 |
+   |                 data moved here because `val` has type `NonCopy`, which does not implement the `Copy` trait
+   |
+help: consider borrowing the pattern binding
+   |
+LL |     if let Some(ref mut val) = (&mut wrap).field {
+   |                 +++
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0507`.

tests/ui/borrowck/deref-index-pattern-ref-suggestion.fixed

@@ -0,0 +1,37 @@
+//@ run-rustfix
+#![allow(dead_code)]
+
+use std::ops::{Deref, DerefMut};
+
+fn take(mut wrap: Wrap<[Option<NonCopy>; 1]>) {
+    if let Some(ref mut val) = wrap[0] {
+        //~^ ERROR cannot move out of type `[Option<NonCopy>; 1]`, a non-copy array
+        val.0 = ();
+    }
+}
+
+fn take_mut_ref_base(mut wrap: Wrap<[Option<NonCopy>; 1]>) {
+    if let Some(ref mut val) = (&mut wrap)[0] {
+        //~^ ERROR cannot move out of type `[Option<NonCopy>; 1]`, a non-copy array
+        val.0 = ();
+    }
+}
+
+struct Wrap<T>(T);
+struct NonCopy(());
+
+impl<T> Deref for Wrap<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<T> DerefMut for Wrap<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
+fn main() {}