Skip to content

Commit 04d9bb7

Browse files
diannedianne
committed
add_move_error_suggestions: use a HIR visitor rather than SourceMap 
1 parent 6d22ff1 commit 04d9bb7

File tree

11 files changed

+249
-104
lines changed

11 files changed

+249
-104
lines changed

‎compiler/rustc_borrowck/src/diagnostics/move_errors.rs‎

Lines changed: 118 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
#![allow(rustc::diagnostic_outside_of_impl)]
22
#![allow(rustc::untranslatable_diagnostic)]
33

4+
use rustc_data_structures::fx::FxHashSet;
45
use rustc_errors::{Applicability, Diag};
56
use rustc_hir::intravisit::Visitor;
6-
use rustc_hir::{CaptureBy, ExprKind, HirId, Node};
7+
use rustc_hir::{self as hir, CaptureBy, ExprKind, HirId, Node};
78
use rustc_middle::bug;
89
use rustc_middle::mir::*;
910
use rustc_middle::ty::{self, Ty};
@@ -683,48 +684,126 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
683684
}
684685

685686
fn add_move_error_suggestions(&self, err: &mut Diag<'_>, binds_to: &[Local]) {
686-
let mut suggestions: Vec<(Span, String, String)> = Vec::new();
687+
/// A HIR visitor to associate each binding with a `&` or `&mut` that could be removed to
688+
/// make it bind by reference instead (if possible)
689+
struct BindingFinder<'tcx> {
690+
typeck_results: &'tcx ty::TypeckResults<'tcx>,
691+
hir: rustc_middle::hir::map::Map<'tcx>,
692+
/// Input: the span of the pattern we're finding bindings in
693+
pat_span: Span,
694+
/// Input: the spans of the bindings we're providing suggestions for
695+
binding_spans: Vec<Span>,
696+
/// Internal state: have we reached the pattern we're finding bindings in?
697+
found_pat: bool,
698+
/// Internal state: the innermost `&` or `&mut` "above" the visitor
699+
ref_pat: Option<&'tcx hir::Pat<'tcx>>,
700+
/// Internal state: could removing a `&` give bindings unexpected types?
701+
has_adjustments: bool,
702+
/// Output: for each input binding, the `&` or `&mut` to remove to make it by-ref
703+
ref_pat_for_binding: Vec<(Span, Option<&'tcx hir::Pat<'tcx>>)>,
704+
/// Output: ref patterns that can't be removed straightforwardly
705+
cannot_remove: FxHashSet<HirId>,
706+
}
707+
impl<'tcx> Visitor<'tcx> for BindingFinder<'tcx> {
708+
type NestedFilter = rustc_middle::hir::nested_filter::OnlyBodies;
709+
710+
fn nested_visit_map(&mut self) -> Self::Map {
711+
self.hir
712+
}
713+
714+
fn visit_expr(&mut self, ex: &'tcx hir::Expr<'tcx>) -> Self::Result {
715+
// Don't walk into const patterns or anything else that might confuse this
716+
if !self.found_pat {
717+
hir::intravisit::walk_expr(self, ex)
718+
}
719+
}
720+
721+
fn visit_pat(&mut self, p: &'tcx hir::Pat<'tcx>) {
722+
if p.span == self.pat_span {
723+
self.found_pat = true;
724+
}
725+
726+
let parent_has_adjustments = self.has_adjustments;
727+
self.has_adjustments |=
728+
self.typeck_results.pat_adjustments().contains_key(p.hir_id);
729+
730+
// Track the innermost `&` or `&mut` enclosing bindings, to suggest removing it.
731+
let parent_ref_pat = self.ref_pat;
732+
if let hir::PatKind::Ref(..) = p.kind {
733+
self.ref_pat = Some(p);
734+
// To avoid edition-dependent logic to figure out how many refs this `&` can
735+
// peel off, simply don't remove the "parent" `&`.
736+
self.cannot_remove.extend(parent_ref_pat.map(|r| r.hir_id));
737+
if self.has_adjustments {
738+
// Removing this `&` could give child bindings unexpected types, so don't.
739+
self.cannot_remove.insert(p.hir_id);
740+
// As long the `&` stays, child patterns' types should be as expected.
741+
self.has_adjustments = false;
742+
}
743+
}
744+
745+
if let hir::PatKind::Binding(_, _, ident, _) = p.kind {
746+
// the spans in `binding_spans` encompass both the ident and binding mode
747+
if let Some(&bind_sp) =
748+
self.binding_spans.iter().find(|bind_sp| bind_sp.contains(ident.span))
749+
{
750+
self.ref_pat_for_binding.push((bind_sp, self.ref_pat));
751+
} else {
752+
// we've encountered a binding that we're not reporting a move error for.
753+
// we don't want to change its type, so don't remove the surrounding `&`.
754+
if let Some(ref_pat) = self.ref_pat {
755+
self.cannot_remove.insert(ref_pat.hir_id);
756+
}
757+
}
758+
}
759+
760+
hir::intravisit::walk_pat(self, p);
761+
self.ref_pat = parent_ref_pat;
762+
self.has_adjustments = parent_has_adjustments;
763+
}
764+
}
765+
let mut pat_span = None;
766+
let mut binding_spans = Vec::new();
687767
for local in binds_to {
688768
let bind_to = &self.body.local_decls[*local];
689-
if let LocalInfo::User(BindingForm::Var(VarBindingForm { pat_span, .. })) =
769+
if let LocalInfo::User(BindingForm::Var(VarBindingForm { pat_span: pat_sp, .. })) =
690770
*bind_to.local_info()
691771
{
692-
let Ok(pat_snippet) = self.infcx.tcx.sess.source_map().span_to_snippet(pat_span)
693-
else {
694-
continue;
695-
};
696-
let Some(stripped) = pat_snippet.strip_prefix('&') else {
697-
suggestions.push((
698-
bind_to.source_info.span.shrink_to_lo(),
699-
"consider borrowing the pattern binding".to_string(),
700-
"ref ".to_string(),
701-
));
702-
continue;
703-
};
704-
let inner_pat_snippet = stripped.trim_start();
705-
let (pat_span, suggestion, to_remove) = if inner_pat_snippet.starts_with("mut")
706-
&& inner_pat_snippet["mut".len()..].starts_with(rustc_lexer::is_whitespace)
707-
{
708-
let inner_pat_snippet = inner_pat_snippet["mut".len()..].trim_start();
709-
let pat_span = pat_span.with_hi(
710-
pat_span.lo()
711-
+ BytePos((pat_snippet.len() - inner_pat_snippet.len()) as u32),
712-
);
713-
(pat_span, String::new(), "mutable borrow")
714-
} else {
715-
let pat_span = pat_span.with_hi(
716-
pat_span.lo()
717-
+ BytePos(
718-
(pat_snippet.len() - inner_pat_snippet.trim_start().len()) as u32,
719-
),
720-
);
721-
(pat_span, String::new(), "borrow")
722-
};
723-
suggestions.push((
724-
pat_span,
725-
format!("consider removing the {to_remove}"),
726-
suggestion,
727-
));
772+
pat_span = Some(pat_sp);
773+
binding_spans.push(bind_to.source_info.span);
774+
}
775+
}
776+
let Some(pat_span) = pat_span else { return };
777+
778+
let hir = self.infcx.tcx.hir();
779+
let Some(body) = hir.maybe_body_owned_by(self.mir_def_id()) else { return };
780+
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
781+
let mut finder = BindingFinder {
782+
typeck_results,
783+
hir,
784+
pat_span,
785+
binding_spans,
786+
found_pat: false,
787+
ref_pat: None,
788+
has_adjustments: false,
789+
ref_pat_for_binding: Vec::new(),
790+
cannot_remove: FxHashSet::default(),
791+
};
792+
finder.visit_body(body);
793+
794+
let mut suggestions = Vec::new();
795+
for (binding_span, opt_ref_pat) in finder.ref_pat_for_binding {
796+
if let Some(ref_pat) = opt_ref_pat
797+
&& !finder.cannot_remove.contains(&ref_pat.hir_id)
798+
&& let hir::PatKind::Ref(subpat, mutbl) = ref_pat.kind
799+
&& let Some(ref_span) = ref_pat.span.trim_end(subpat.span)
800+
{
801+
let mutable_str = if mutbl.is_mut() { "mutable " } else { "" };
802+
let msg = format!("consider removing the {mutable_str}borrow");
803+
suggestions.push((ref_span, msg, "".to_string()));
804+
} else {
805+
let msg = "consider borrowing the pattern binding".to_string();
806+
suggestions.push((binding_span.shrink_to_lo(), msg, "ref ".to_string()));
728807
}
729808
}
730809
suggestions.sort_unstable_by_key(|&(span, _, _)| span);

‎tests/ui/issues/issue-12567.stderr‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,16 @@ LL | (&[hd1, ..], &[hd2, ..])
1111
| --- ...and here
1212
|
1313
= note: move occurs because these variables have types that don't implement the `Copy` trait
14-
help: consider borrowing the pattern binding
14+
help: consider removing the borrow
1515
|
16-
LL | (&[], &[ref hd, ..]) | (&[hd, ..], &[])
17-
| +++
18-
help: consider borrowing the pattern binding
16+
LL - (&[], &[hd, ..]) | (&[hd, ..], &[])
17+
LL + (&[], [hd, ..]) | (&[hd, ..], &[])
18+
|
19+
help: consider removing the borrow
20+
|
21+
LL - (&[hd1, ..], &[hd2, ..])
22+
LL + (&[hd1, ..], [hd2, ..])
1923
|
20-
LL | (&[hd1, ..], &[ref hd2, ..])
21-
| +++
2224

2325
error[E0508]: cannot move out of type `[T]`, a non-copy slice
2426
--> $DIR/issue-12567.rs:2:11
@@ -33,14 +35,16 @@ LL | (&[hd1, ..], &[hd2, ..])
3335
| --- ...and here
3436
|
3537
= note: move occurs because these variables have types that don't implement the `Copy` trait
36-
help: consider borrowing the pattern binding
38+
help: consider removing the borrow
39+
|
40+
LL - (&[], &[hd, ..]) | (&[hd, ..], &[])
41+
LL + (&[], [hd, ..]) | (&[hd, ..], &[])
42+
|
43+
help: consider removing the borrow
3744
|
38-
LL | (&[], &[ref hd, ..]) | (&[hd, ..], &[])
39-
| +++
40-
help: consider borrowing the pattern binding
45+
LL - (&[hd1, ..], &[hd2, ..])
46+
LL + ([hd1, ..], &[hd2, ..])
4147
|
42-
LL | (&[ref hd1, ..], &[hd2, ..])
43-
| +++
4448

4549
error: aborting due to 2 previous errors
4650

‎tests/ui/match/ref_pat_eat_one_layer_2024/ref_pat_eat_one_layer_2024_fail2.stderr‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,11 @@ LL | if let Some(&Some(x)) = Some(&Some(&mut 0)) {
77
| data moved here
88
| move occurs because `x` has type `&mut u32`, which does not implement the `Copy` trait
99
|
10-
help: consider borrowing the pattern binding
10+
help: consider removing the borrow
11+
|
12+
LL - if let Some(&Some(x)) = Some(&Some(&mut 0)) {
13+
LL + if let Some(Some(x)) = Some(&Some(&mut 0)) {
1114
|
12-
LL | if let Some(&Some(ref x)) = Some(&Some(&mut 0)) {
13-
| +++
1415

1516
error[E0596]: cannot borrow data in a `&` reference as mutable
1617
--> $DIR/ref_pat_eat_one_layer_2024_fail2.rs:11:10

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

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

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

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

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
error[E0507]: cannot move out of a shared reference
2+
--> $DIR/do-not-suggest-removing-wrong-ref-pattern-issue-132806.rs:8:58
3+
|
4+
LL | let _ = HashMap::<String, i32>::new().iter().filter(|&(&_k, &_v)| { true });
5+
| ^^^--^^^^^^
6+
| |
7+
| data moved here
8+
| move occurs because `_k` has type `String`, which does not implement the `Copy` trait
9+
|
10+
help: consider removing the borrow
11+
|
12+
LL - let _ = HashMap::<String, i32>::new().iter().filter(|&(&_k, &_v)| { true });
13+
LL + let _ = HashMap::<String, i32>::new().iter().filter(|&(_k, &_v)| { true });
14+
|
15+
16+
error: aborting due to 1 previous error
17+
18+
For more information about this error, try `rustc --explain E0507`.

‎tests/ui/nll/move-errors.stderr‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -209,10 +209,11 @@ LL | (D(s), &t) => (),
209209
| data moved here
210210
| move occurs because `t` has type `String`, which does not implement the `Copy` trait
211211
|
212-
help: consider borrowing the pattern binding
212+
help: consider removing the borrow
213+
|
214+
LL - (D(s), &t) => (),
215+
LL + (D(s), t) => (),
213216
|
214-
LL | (D(s), &ref t) => (),
215-
| +++
216217

217218
error[E0509]: cannot move out of type `F`, which implements the `Drop` trait
218219
--> $DIR/move-errors.rs:102:11

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ LL | deref!(x) => x,
99
| |
1010
| data moved here
1111
| move occurs because `x` has type `Struct`, which does not implement the `Copy` trait
12+
|
13+
help: consider borrowing the pattern binding
14+
|
15+
LL | deref!(ref x) => x,
16+
| +++
1217

1318
error[E0507]: cannot move out of a shared reference
1419
--> $DIR/cant_move_out_of_pattern.rs:17:11
@@ -21,6 +26,11 @@ LL | deref!(x) => x,
2126
| |
2227
| data moved here
2328
| move occurs because `x` has type `Struct`, which does not implement the `Copy` trait
29+
|
30+
help: consider borrowing the pattern binding
31+
|
32+
LL | deref!(ref x) => x,
33+
| +++
2434

2535
error: aborting due to 2 previous errors
2636

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

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -219,42 +219,42 @@ pub fn main() {
219219

220220
let (&X(_t),) = (&x.clone(),);
221221
//~^ ERROR cannot move
222-
//~| HELP consider borrowing the pattern binding
222+
//~| HELP consider removing the borrow
223223
if let (&Either::One(_t),) = (&e.clone(),) { }
224224
//~^ ERROR cannot move
225-
//~| HELP consider borrowing the pattern binding
225+
//~| HELP consider removing the borrow
226226
while let (&Either::One(_t),) = (&e.clone(),) { }
227227
//~^ ERROR cannot move
228-
//~| HELP consider borrowing the pattern binding
228+
//~| HELP consider removing the borrow
229229
match (&e.clone(),) {
230230
//~^ ERROR cannot move
231231
(&Either::One(_t),)
232-
//~^ HELP consider borrowing the pattern binding
232+
//~^ HELP consider removing the borrow
233233
| (&Either::Two(_t),) => (),
234234
}
235235
fn f3((&X(_t),): (&X,)) { }
236236
//~^ ERROR cannot move
237-
//~| HELP consider borrowing the pattern binding
237+
//~| HELP consider removing the borrow
238238

239239
let (&mut X(_t),) = (&mut xm.clone(),);
240240
//~^ ERROR cannot move
241-
//~| HELP consider borrowing the pattern binding
241+
//~| HELP consider removing the mutable borrow
242242
if let (&mut Either::One(_t),) = (&mut em.clone(),) { }
243243
//~^ ERROR cannot move
244-
//~| HELP consider borrowing the pattern binding
244+
//~| HELP consider removing the mutable borrow
245245
while let (&mut Either::One(_t),) = (&mut em.clone(),) { }
246246
//~^ ERROR cannot move
247-
//~| HELP consider borrowing the pattern binding
247+
//~| HELP consider removing the mutable borrow
248248
match (&mut em.clone(),) {
249249
//~^ ERROR cannot move
250250
(&mut Either::One(_t),) => (),
251-
//~^ HELP consider borrowing the pattern binding
251+
//~^ HELP consider removing the mutable borrow
252252
(&mut Either::Two(_t),) => (),
253-
//~^ HELP consider borrowing the pattern binding
253+
//~^ HELP consider removing the mutable borrow
254254
}
255255
fn f4((&mut X(_t),): (&mut X,)) { }
256256
//~^ ERROR cannot move
257-
//~| HELP consider borrowing the pattern binding
257+
//~| HELP consider removing the mutable borrow
258258

259259
// move from &Either/&X value
260260

0 commit comments

Comments
 (0)