Rollup merge of rust-lang#133486 - dianne:fix-move-error-suggestion, r=estebank

borrowck diagnostics: make `add_move_error_suggestions` use the HIR rather than `SourceMap`

This PR aims to fix rust-lang#132806 by rewriting `add_move_error_suggestions`[^1]. Previously, it manually scanned the source text to find a leading `&`, which isn't always going to produce a correct result (see: that issue). Admittedly, the HIR visitor in this PR introduces a lot of boilerplate, but hopefully the logic at its core isn't too complicated (I go over it in the comments). I also tried a simpler version that didn't use a HIR visitor and suggested adding `ref` always, but the `&ref x` suggestions really didn't look good. As a bonus for the added complexity though, it's now able to produce nice `&`-removing suggestions in more cases.

I tried to do this such that it avoids edition-dependent checks and its suggestions can be applied together with those from the match ergonomics 2024 migration lint. I haven't added tests for that since the details of match ergonomics 2024 are still being sorted out, but I can try if desired once that's finalized.

[^1]: In brief, it fires on patterns where users try to bind by-value in such a way that moves out of a reference to a non-Copy type (including slice references with non-copy elements). The suggestions are to change the binding's mode to be by-reference, either by removing[^2] an enclosing `&`/`&mut` or adding `ref` to the binding.

[^2]: Incidentally, I find the terminology of "consider removing the borrow" a bit confusing for a suggestion to remove a `&` pattern in order to make bindings borrow rather than move. I'm not sure what a good, concise way to explain that would be though, and that should go in a separate PR anyway.

Author: Noratrieb

compiler/rustc_borrowck/src/diagnostics/move_errors.rs

@@ -1,9 +1,10 @@
 #![allow(rustc::diagnostic_outside_of_impl)]
 #![allow(rustc::untranslatable_diagnostic)]
 
+use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::{Applicability, Diag};
 use rustc_hir::intravisit::Visitor;
-use rustc_hir::{CaptureBy, ExprKind, HirId, Node};
+use rustc_hir::{self as hir, CaptureBy, ExprKind, HirId, Node};
 use rustc_middle::bug;
 use rustc_middle::mir::*;
 use rustc_middle::ty::{self, Ty};
@@ -683,48 +684,126 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
     }
 
     fn add_move_error_suggestions(&self, err: &mut Diag<'_>, binds_to: &[Local]) {
-        let mut suggestions: Vec<(Span, String, String)> = Vec::new();
+        /// 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> {
+            typeck_results: &'tcx ty::TypeckResults<'tcx>,
+            hir: rustc_middle::hir::map::Map<'tcx>,
+            /// Input: the span of the pattern we're finding bindings in
+            pat_span: Span,
+            /// Input: the spans of the bindings we're providing suggestions for
+            binding_spans: Vec<Span>,
+            /// Internal state: have we reached the pattern we're finding bindings in?
+            found_pat: bool,
+            /// Internal state: the innermost `&` or `&mut` "above" the visitor
+            ref_pat: Option<&'tcx hir::Pat<'tcx>>,
+            /// Internal state: could removing a `&` give bindings unexpected types?
+            has_adjustments: bool,
+            /// Output: for each input binding, the `&` or `&mut` to remove to make it by-ref
+            ref_pat_for_binding: Vec<(Span, Option<&'tcx hir::Pat<'tcx>>)>,
+            /// Output: ref patterns that can't be removed straightforwardly
+            cannot_remove: FxHashSet<HirId>,
+        }
+        impl<'tcx> Visitor<'tcx> for BindingFinder<'tcx> {
+            type NestedFilter = rustc_middle::hir::nested_filter::OnlyBodies;
+
+            fn nested_visit_map(&mut self) -> Self::Map {
+                self.hir
+            }
+
+            fn visit_expr(&mut self, ex: &'tcx hir::Expr<'tcx>) -> Self::Result {
+                // Don't walk into const patterns or anything else that might confuse this
+                if !self.found_pat {
+                    hir::intravisit::walk_expr(self, ex)
+                }
+            }
+
+            fn visit_pat(&mut self, p: &'tcx hir::Pat<'tcx>) {
+                if p.span == self.pat_span {
+                    self.found_pat = true;
+                }
+
+                let parent_has_adjustments = self.has_adjustments;
+                self.has_adjustments |=
+                    self.typeck_results.pat_adjustments().contains_key(p.hir_id);
+
+                // Track the innermost `&` or `&mut` enclosing bindings, to suggest removing it.
+                let parent_ref_pat = self.ref_pat;
+                if let hir::PatKind::Ref(..) = p.kind {
+                    self.ref_pat = Some(p);
+                    // To avoid edition-dependent logic to figure out how many refs this `&` can
+                    // peel off, simply don't remove the "parent" `&`.
+                    self.cannot_remove.extend(parent_ref_pat.map(|r| r.hir_id));
+                    if self.has_adjustments {
+                        // Removing this `&` could give child bindings unexpected types, so don't.
+                        self.cannot_remove.insert(p.hir_id);
+                        // As long the `&` stays, child patterns' types should be as expected.
+                        self.has_adjustments = false;
+                    }
+                }
+
+                if let hir::PatKind::Binding(_, _, ident, _) = p.kind {
+                    // the spans in `binding_spans` encompass both the ident and binding mode
+                    if let Some(&bind_sp) =
+                        self.binding_spans.iter().find(|bind_sp| bind_sp.contains(ident.span))
+                    {
+                        self.ref_pat_for_binding.push((bind_sp, self.ref_pat));
+                    } else {
+                        // we've encountered a binding that we're not reporting a move error for.
+                        // we don't want to change its type, so don't remove the surrounding `&`.
+                        if let Some(ref_pat) = self.ref_pat {
+                            self.cannot_remove.insert(ref_pat.hir_id);
+                        }
+                    }
+                }
+
+                hir::intravisit::walk_pat(self, p);
+                self.ref_pat = parent_ref_pat;
+                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, .. })) =
+            if let LocalInfo::User(BindingForm::Var(VarBindingForm { pat_span: pat_sp, .. })) =
                 *bind_to.local_info()
             {
-                let Ok(pat_snippet) = self.infcx.tcx.sess.source_map().span_to_snippet(pat_span)
-                else {
-                    continue;
-                };
-                let Some(stripped) = pat_snippet.strip_prefix('&') else {
-                    suggestions.push((
-                        bind_to.source_info.span.shrink_to_lo(),
-                        "consider borrowing the pattern binding".to_string(),
-                        "ref ".to_string(),
-                    ));
-                    continue;
-                };
-                let inner_pat_snippet = stripped.trim_start();
-                let (pat_span, suggestion, to_remove) = if inner_pat_snippet.starts_with("mut")
-                    && inner_pat_snippet["mut".len()..].starts_with(rustc_lexer::is_whitespace)
-                {
-                    let inner_pat_snippet = inner_pat_snippet["mut".len()..].trim_start();
-                    let pat_span = pat_span.with_hi(
-                        pat_span.lo()
-                            + BytePos((pat_snippet.len() - inner_pat_snippet.len()) as u32),
-                    );
-                    (pat_span, String::new(), "mutable borrow")
-                } else {
-                    let pat_span = pat_span.with_hi(
-                        pat_span.lo()
-                            + BytePos(
-                                (pat_snippet.len() - inner_pat_snippet.trim_start().len()) as u32,
-                            ),
-                    );
-                    (pat_span, String::new(), "borrow")
-                };
-                suggestions.push((
-                    pat_span,
-                    format!("consider removing the {to_remove}"),
-                    suggestion,
-                ));
+                pat_span = Some(pat_sp);
+                binding_spans.push(bind_to.source_info.span);
+            }
+        }
+        let Some(pat_span) = pat_span else { return };
+
+        let hir = self.infcx.tcx.hir();
+        let Some(body) = hir.maybe_body_owned_by(self.mir_def_id()) else { return };
+        let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
+        let mut finder = BindingFinder {
+            typeck_results,
+            hir,
+            pat_span,
+            binding_spans,
+            found_pat: false,
+            ref_pat: None,
+            has_adjustments: false,
+            ref_pat_for_binding: Vec::new(),
+            cannot_remove: FxHashSet::default(),
+        };
+        finder.visit_body(body);
+
+        let mut suggestions = Vec::new();
+        for (binding_span, opt_ref_pat) in finder.ref_pat_for_binding {
+            if let Some(ref_pat) = opt_ref_pat
+                && !finder.cannot_remove.contains(&ref_pat.hir_id)
+                && let hir::PatKind::Ref(subpat, mutbl) = ref_pat.kind
+                && let Some(ref_span) = ref_pat.span.trim_end(subpat.span)
+            {
+                let mutable_str = if mutbl.is_mut() { "mutable " } else { "" };
+                let msg = format!("consider removing the {mutable_str}borrow");
+                suggestions.push((ref_span, msg, "".to_string()));
+            } else {
+                let msg = "consider borrowing the pattern binding".to_string();
+                suggestions.push((binding_span.shrink_to_lo(), msg, "ref ".to_string()));
             }
         }
         suggestions.sort_unstable_by_key(|&(span, _, _)| span);

tests/ui/issues/issue-12567.stderr

@@ -11,14 +11,16 @@ LL |         (&[hd1, ..], &[hd2, ..])
    |                        --- ...and here
    |
    = note: move occurs because these variables have types that don't implement the `Copy` trait
-help: consider borrowing the pattern binding
+help: consider removing the borrow
    |
-LL |         (&[], &[ref hd, ..]) | (&[hd, ..], &[])
-   |                 +++
-help: consider borrowing the pattern binding
+LL -         (&[], &[hd, ..]) | (&[hd, ..], &[])
+LL +         (&[], [hd, ..]) | (&[hd, ..], &[])
+   |
+help: consider removing the borrow
+   |
+LL -         (&[hd1, ..], &[hd2, ..])
+LL +         (&[hd1, ..], [hd2, ..])
    |
-LL |         (&[hd1, ..], &[ref hd2, ..])
-   |                        +++
 
 error[E0508]: cannot move out of type `[T]`, a non-copy slice
   --> $DIR/issue-12567.rs:2:11
@@ -33,14 +35,16 @@ LL |         (&[hd1, ..], &[hd2, ..])
    |            --- ...and here
    |
    = note: move occurs because these variables have types that don't implement the `Copy` trait
-help: consider borrowing the pattern binding
+help: consider removing the borrow
+   |
+LL -         (&[], &[hd, ..]) | (&[hd, ..], &[])
+LL +         (&[], [hd, ..]) | (&[hd, ..], &[])
+   |
+help: consider removing the borrow
    |
-LL |         (&[], &[ref hd, ..]) | (&[hd, ..], &[])
-   |                 +++
-help: consider borrowing the pattern binding
+LL -         (&[hd1, ..], &[hd2, ..])
+LL +         ([hd1, ..], &[hd2, ..])
    |
-LL |         (&[ref hd1, ..], &[hd2, ..])
-   |            +++
 
 error: aborting due to 2 previous errors
 

tests/ui/match/ref_pat_eat_one_layer_2024/ref_pat_eat_one_layer_2024_fail2.stderr

@@ -7,10 +7,11 @@ LL |     if let Some(&Some(x)) = Some(&Some(&mut 0)) {
    |                       data moved here
    |                       move occurs because `x` has type `&mut u32`, which does not implement the `Copy` trait
    |
-help: consider borrowing the pattern binding
+help: consider removing the borrow
+   |
+LL -     if let Some(&Some(x)) = Some(&Some(&mut 0)) {
+LL +     if let Some(Some(x)) = Some(&Some(&mut 0)) {
    |
-LL |     if let Some(&Some(ref x)) = Some(&Some(&mut 0)) {
-   |                       +++
 
 error[E0596]: cannot borrow data in a `&` reference as mutable
   --> $DIR/ref_pat_eat_one_layer_2024_fail2.rs:11:10

tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.fixed

@@ -0,0 +1,10 @@
+//@ run-rustfix
+//! diagnostic test for #132806: make sure the suggestion to bind by-reference in patterns doesn't
+//! erroneously remove the wrong `&`
+
+use std::collections::HashMap;
+
+fn main() {
+    let _ = HashMap::<String, i32>::new().iter().filter(|&(_k, &_v)| { true });
+    //~^ ERROR cannot move out of a shared reference
+}

tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.rs

@@ -0,0 +1,10 @@
+//@ run-rustfix
+//! diagnostic test for #132806: make sure the suggestion to bind by-reference in patterns doesn't
+//! erroneously remove the wrong `&`
+
+use std::collections::HashMap;
+
+fn main() {
+    let _ = HashMap::<String, i32>::new().iter().filter(|&(&_k, &_v)| { true });
+    //~^ ERROR cannot move out of a shared reference
+}

tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.stderr

@@ -0,0 +1,18 @@
+error[E0507]: cannot move out of a shared reference
+  --> $DIR/do-not-suggest-removing-wrong-ref-pattern-issue-132806.rs:8:58
+   |
+LL |     let _ = HashMap::<String, i32>::new().iter().filter(|&(&_k, &_v)| { true });
+   |                                                          ^^^--^^^^^^
+   |                                                             |
+   |                                                             data moved here
+   |                                                             move occurs because `_k` has type `String`, which does not implement the `Copy` trait
+   |
+help: consider removing the borrow
+   |
+LL -     let _ = HashMap::<String, i32>::new().iter().filter(|&(&_k, &_v)| { true });
+LL +     let _ = HashMap::<String, i32>::new().iter().filter(|&(_k, &_v)| { true });
+   |
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0507`.

tests/ui/nll/move-errors.stderr

@@ -209,10 +209,11 @@ LL |         (D(s), &t) => (),
    |                 data moved here
    |                 move occurs because `t` has type `String`, which does not implement the `Copy` trait
    |
-help: consider borrowing the pattern binding
+help: consider removing the borrow
+   |
+LL -         (D(s), &t) => (),
+LL +         (D(s), t) => (),
    |
-LL |         (D(s), &ref t) => (),
-   |                 +++
 
 error[E0509]: cannot move out of type `F`, which implements the `Drop` trait
   --> $DIR/move-errors.rs:102:11

tests/ui/pattern/deref-patterns/cant_move_out_of_pattern.stderr

@@ -9,6 +9,11 @@ LL |         deref!(x) => x,
    |                |
    |                data moved here
    |                move occurs because `x` has type `Struct`, which does not implement the `Copy` trait
+   |
+help: consider borrowing the pattern binding
+   |
+LL |         deref!(ref x) => x,
+   |                +++
 
 error[E0507]: cannot move out of a shared reference
   --> $DIR/cant_move_out_of_pattern.rs:17:11
@@ -21,6 +26,11 @@ LL |         deref!(x) => x,
    |                |
    |                data moved here
    |                move occurs because `x` has type `Struct`, which does not implement the `Copy` trait
+   |
+help: consider borrowing the pattern binding
+   |
+LL |         deref!(ref x) => x,
+   |                +++
 
 error: aborting due to 2 previous errors
 

tests/ui/suggestions/dont-suggest-ref/simple.rs

@@ -219,42 +219,42 @@ pub fn main() {
 
     let (&X(_t),) = (&x.clone(),);
     //~^ ERROR cannot move
-    //~| HELP consider borrowing the pattern binding
+    //~| HELP consider removing the borrow
     if let (&Either::One(_t),) = (&e.clone(),) { }
     //~^ ERROR cannot move
-    //~| HELP consider borrowing the pattern binding
+    //~| HELP consider removing the borrow
     while let (&Either::One(_t),) = (&e.clone(),) { }
     //~^ ERROR cannot move
-    //~| HELP consider borrowing the pattern binding
+    //~| HELP consider removing the borrow
     match (&e.clone(),) {
         //~^ ERROR cannot move
         (&Either::One(_t),)
-        //~^ HELP consider borrowing the pattern binding
+        //~^ HELP consider removing the borrow
         | (&Either::Two(_t),) => (),
     }
     fn f3((&X(_t),): (&X,)) { }
     //~^ ERROR cannot move
-    //~| HELP consider borrowing the pattern binding
+    //~| HELP consider removing the borrow
 
     let (&mut X(_t),) = (&mut xm.clone(),);
     //~^ ERROR cannot move
-    //~| HELP consider borrowing the pattern binding
+    //~| HELP consider removing the mutable borrow
     if let (&mut Either::One(_t),) = (&mut em.clone(),) { }
     //~^ ERROR cannot move
-    //~| HELP consider borrowing the pattern binding
+    //~| HELP consider removing the mutable borrow
     while let (&mut Either::One(_t),) = (&mut em.clone(),) { }
     //~^ ERROR cannot move
-    //~| HELP consider borrowing the pattern binding
+    //~| HELP consider removing the mutable borrow
     match (&mut em.clone(),) {
         //~^ ERROR cannot move
         (&mut Either::One(_t),) => (),
-        //~^ HELP consider borrowing the pattern binding
+        //~^ HELP consider removing the mutable borrow
         (&mut Either::Two(_t),) => (),
-        //~^ HELP consider borrowing the pattern binding
+        //~^ HELP consider removing the mutable borrow
     }
     fn f4((&mut X(_t),): (&mut X,)) { }
     //~^ ERROR cannot move
-    //~| HELP consider borrowing the pattern binding
+    //~| HELP consider removing the mutable borrow
 
     // move from &Either/&X value