diff --git a/compiler/rustc_borrowck/src/diagnostics/move_errors.rs b/compiler/rustc_borrowck/src/diagnostics/move_errors.rs index 4ba6b2e94ec56..beacbdbd3fa7b 100644 --- a/compiler/rustc_borrowck/src/diagnostics/move_errors.rs +++ b/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, + /// 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, + } + 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); diff --git a/tests/ui/issues/issue-12567.stderr b/tests/ui/issues/issue-12567.stderr index 3f95f18a96743..0b19299ece3ec 100644 --- a/tests/ui/issues/issue-12567.stderr +++ b/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 diff --git a/tests/ui/match/ref_pat_eat_one_layer_2024/ref_pat_eat_one_layer_2024_fail2.stderr b/tests/ui/match/ref_pat_eat_one_layer_2024/ref_pat_eat_one_layer_2024_fail2.stderr index 52f4c09e5c024..a8b813941109c 100644 --- a/tests/ui/match/ref_pat_eat_one_layer_2024/ref_pat_eat_one_layer_2024_fail2.stderr +++ b/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 diff --git a/tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.fixed b/tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.fixed new file mode 100644 index 0000000000000..46b05e4c0a3c2 --- /dev/null +++ b/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::::new().iter().filter(|&(_k, &_v)| { true }); + //~^ ERROR cannot move out of a shared reference +} diff --git a/tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.rs b/tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.rs new file mode 100644 index 0000000000000..1312fd6425b65 --- /dev/null +++ b/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::::new().iter().filter(|&(&_k, &_v)| { true }); + //~^ ERROR cannot move out of a shared reference +} diff --git a/tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.stderr b/tests/ui/moves/do-not-suggest-removing-wrong-ref-pattern-issue-132806.stderr new file mode 100644 index 0000000000000..ff579f934137f --- /dev/null +++ b/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::::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::::new().iter().filter(|&(&_k, &_v)| { true }); +LL + let _ = HashMap::::new().iter().filter(|&(_k, &_v)| { true }); + | + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0507`. diff --git a/tests/ui/nll/move-errors.stderr b/tests/ui/nll/move-errors.stderr index d138412137959..bcb2ab84a239b 100644 --- a/tests/ui/nll/move-errors.stderr +++ b/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 diff --git a/tests/ui/pattern/deref-patterns/cant_move_out_of_pattern.stderr b/tests/ui/pattern/deref-patterns/cant_move_out_of_pattern.stderr index 108db6d9e4b53..2cf435b117999 100644 --- a/tests/ui/pattern/deref-patterns/cant_move_out_of_pattern.stderr +++ b/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 diff --git a/tests/ui/suggestions/dont-suggest-ref/simple.rs b/tests/ui/suggestions/dont-suggest-ref/simple.rs index 1e40e60a1ce12..4dea5319264ed 100644 --- a/tests/ui/suggestions/dont-suggest-ref/simple.rs +++ b/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 diff --git a/tests/ui/suggestions/dont-suggest-ref/simple.stderr b/tests/ui/suggestions/dont-suggest-ref/simple.stderr index 7d902dbccc4d4..41571bf9b2ca4 100644 --- a/tests/ui/suggestions/dont-suggest-ref/simple.stderr +++ b/tests/ui/suggestions/dont-suggest-ref/simple.stderr @@ -578,10 +578,11 @@ LL | let (&X(_t),) = (&x.clone(),); | data moved here | move occurs because `_t` has type `Y`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the borrow + | +LL - let (&X(_t),) = (&x.clone(),); +LL + let (X(_t),) = (&x.clone(),); | -LL | let (&X(ref _t),) = (&x.clone(),); - | +++ error[E0507]: cannot move out of a shared reference --> $DIR/simple.rs:223:34 @@ -592,10 +593,11 @@ LL | if let (&Either::One(_t),) = (&e.clone(),) { } | data moved here | move occurs because `_t` has type `X`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the borrow + | +LL - if let (&Either::One(_t),) = (&e.clone(),) { } +LL + if let (Either::One(_t),) = (&e.clone(),) { } | -LL | if let (&Either::One(ref _t),) = (&e.clone(),) { } - | +++ error[E0507]: cannot move out of a shared reference --> $DIR/simple.rs:226:37 @@ -606,10 +608,11 @@ LL | while let (&Either::One(_t),) = (&e.clone(),) { } | data moved here | move occurs because `_t` has type `X`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the borrow + | +LL - while let (&Either::One(_t),) = (&e.clone(),) { } +LL + while let (Either::One(_t),) = (&e.clone(),) { } | -LL | while let (&Either::One(ref _t),) = (&e.clone(),) { } - | +++ error[E0507]: cannot move out of a shared reference --> $DIR/simple.rs:229:11 @@ -623,10 +626,11 @@ LL | (&Either::One(_t),) | data moved here | move occurs because `_t` has type `X`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the borrow + | +LL - (&Either::One(_t),) +LL + (Either::One(_t),) | -LL | (&Either::One(ref _t),) - | +++ error[E0507]: cannot move out of a mutable reference --> $DIR/simple.rs:239:25 @@ -637,10 +641,11 @@ LL | let (&mut X(_t),) = (&mut xm.clone(),); | data moved here | move occurs because `_t` has type `Y`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the mutable borrow + | +LL - let (&mut X(_t),) = (&mut xm.clone(),); +LL + let (X(_t),) = (&mut xm.clone(),); | -LL | let (&mut X(ref _t),) = (&mut xm.clone(),); - | +++ error[E0507]: cannot move out of a mutable reference --> $DIR/simple.rs:242:38 @@ -651,10 +656,11 @@ LL | if let (&mut Either::One(_t),) = (&mut em.clone(),) { } | data moved here | move occurs because `_t` has type `X`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the mutable borrow + | +LL - if let (&mut Either::One(_t),) = (&mut em.clone(),) { } +LL + if let (Either::One(_t),) = (&mut em.clone(),) { } | -LL | if let (&mut Either::One(ref _t),) = (&mut em.clone(),) { } - | +++ error[E0507]: cannot move out of a mutable reference --> $DIR/simple.rs:245:41 @@ -665,10 +671,11 @@ LL | while let (&mut Either::One(_t),) = (&mut em.clone(),) { } | data moved here | move occurs because `_t` has type `X`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the mutable borrow + | +LL - while let (&mut Either::One(_t),) = (&mut em.clone(),) { } +LL + while let (Either::One(_t),) = (&mut em.clone(),) { } | -LL | while let (&mut Either::One(ref _t),) = (&mut em.clone(),) { } - | +++ error[E0507]: cannot move out of a mutable reference --> $DIR/simple.rs:248:11 @@ -683,14 +690,16 @@ LL | (&mut Either::Two(_t),) => (), | -- ...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 mutable borrow | -LL | (&mut Either::One(ref _t),) => (), - | +++ -help: consider borrowing the pattern binding +LL - (&mut Either::One(_t),) => (), +LL + (Either::One(_t),) => (), + | +help: consider removing the mutable borrow + | +LL - (&mut Either::Two(_t),) => (), +LL + (Either::Two(_t),) => (), | -LL | (&mut Either::Two(ref _t),) => (), - | +++ error[E0507]: cannot move out of a shared reference --> $DIR/simple.rs:261:18 @@ -947,10 +956,11 @@ LL | fn f3((&X(_t),): (&X,)) { } | data moved here | move occurs because `_t` has type `Y`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the borrow + | +LL - fn f3((&X(_t),): (&X,)) { } +LL + fn f3((X(_t),): (&X,)) { } | -LL | fn f3((&X(ref _t),): (&X,)) { } - | +++ error[E0507]: cannot move out of a mutable reference --> $DIR/simple.rs:255:11 @@ -961,10 +971,11 @@ LL | fn f4((&mut X(_t),): (&mut X,)) { } | data moved here | move occurs because `_t` has type `Y`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the mutable borrow + | +LL - fn f4((&mut X(_t),): (&mut X,)) { } +LL + fn f4((X(_t),): (&mut X,)) { } | -LL | fn f4((&mut X(ref _t),): (&mut X,)) { } - | +++ error[E0507]: cannot move out of `a.a` as enum variant `Some` which is behind a shared reference --> $DIR/simple.rs:331:20 diff --git a/tests/ui/suggestions/option-content-move-from-tuple-match.stderr b/tests/ui/suggestions/option-content-move-from-tuple-match.stderr index 63314acb87cbf..c93570c579ee7 100644 --- a/tests/ui/suggestions/option-content-move-from-tuple-match.stderr +++ b/tests/ui/suggestions/option-content-move-from-tuple-match.stderr @@ -10,10 +10,11 @@ LL | (None, &c) => &c.unwrap(), | data moved here | move occurs because `c` has type `Option`, which does not implement the `Copy` trait | -help: consider borrowing the pattern binding +help: consider removing the borrow + | +LL - (None, &c) => &c.unwrap(), +LL + (None, c) => &c.unwrap(), | -LL | (None, &ref c) => &c.unwrap(), - | +++ error: aborting due to 1 previous error