Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change pattern borrowing suggestions to be verbose and remove invalid suggestion #105476

Merged
merged 5 commits into from
Dec 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 64 additions & 36 deletions compiler/rustc_borrowck/src/diagnostics/move_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rustc_middle::ty;
use rustc_mir_dataflow::move_paths::{
IllegalMoveOrigin, IllegalMoveOriginKind, LookupResult, MoveError, MovePathIndex,
};
use rustc_span::Span;
use rustc_span::{BytePos, Span};

use crate::diagnostics::{DescribePlaceOpt, UseSpans};
use crate::prefixes::PrefixSet;
Expand Down Expand Up @@ -148,7 +148,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
match_span: Span,
statement_span: Span,
) {
debug!("append_binding_error(match_place={:?}, match_span={:?})", match_place, match_span);
debug!(?match_place, ?match_span, "append_binding_error");

let from_simple_let = match_place.is_none();
let match_place = match_place.unwrap_or(move_from);
Expand All @@ -160,7 +160,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
if let GroupedMoveError::MovesFromPlace { span, binds_to, .. } = ge
&& match_span == *span
{
debug!("appending local({:?}) to list", bind_to);
debug!("appending local({bind_to:?}) to list");
if !binds_to.is_empty() {
binds_to.push(bind_to);
}
Expand Down Expand Up @@ -198,7 +198,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
} = ge
{
if match_span == *span && mpi == *other_mpi {
debug!("appending local({:?}) to list", bind_to);
debug!("appending local({bind_to:?}) to list");
binds_to.push(bind_to);
return;
}
Expand Down Expand Up @@ -410,15 +410,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diagnostic, span: Span) {
match error {
GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => {
if let Ok(snippet) = self.infcx.tcx.sess.source_map().span_to_snippet(span) {
err.span_suggestion(
span,
"consider borrowing here",
format!("&{snippet}"),
Applicability::Unspecified,
);
}

self.add_borrow_suggestions(err, span);
if binds_to.is_empty() {
let place_ty = move_from.ty(self.body, self.infcx.tcx).ty;
let place_desc = match self.describe_place(move_from.as_ref()) {
Expand Down Expand Up @@ -461,39 +453,75 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
}
}

fn add_borrow_suggestions(&self, err: &mut Diagnostic, span: Span) {
match self.infcx.tcx.sess.source_map().span_to_snippet(span) {
Ok(snippet) if snippet.starts_with('*') => {
err.span_suggestion_verbose(
span.with_hi(span.lo() + BytePos(1)),
"consider removing the dereference here",
String::new(),
Applicability::MaybeIncorrect,
);
}
_ => {
err.span_suggestion_verbose(
span.shrink_to_lo(),
"consider borrowing here",
"&".to_string(),
Applicability::MaybeIncorrect,
);
}
}
}

fn add_move_error_suggestions(&self, err: &mut Diagnostic, binds_to: &[Local]) {
let mut suggestions: Vec<(Span, &str, String)> = Vec::new();
let mut suggestions: Vec<(Span, String, String)> = Vec::new();
for local in binds_to {
let bind_to = &self.body.local_decls[*local];
if let Some(box LocalInfo::User(ClearCrossCrate::Set(BindingForm::Var(
VarBindingForm { pat_span, .. },
)))) = bind_to.local_info
{
if let Ok(pat_snippet) = self.infcx.tcx.sess.source_map().span_to_snippet(pat_span)
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)
{
if let Some(stripped) = pat_snippet.strip_prefix('&') {
let pat_snippet = stripped.trim_start();
let (suggestion, to_remove) = if pat_snippet.starts_with("mut")
&& pat_snippet["mut".len()..].starts_with(rustc_lexer::is_whitespace)
{
(pat_snippet["mut".len()..].trim_start(), "&mut")
} else {
(pat_snippet, "&")
};
suggestions.push((pat_span, to_remove, suggestion.to_owned()));
}
}
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.to_string(),
));
}
}
suggestions.sort_unstable_by_key(|&(span, _, _)| span);
suggestions.dedup_by_key(|&mut (span, _, _)| span);
for (span, to_remove, suggestion) in suggestions {
err.span_suggestion(
span,
&format!("consider removing the `{to_remove}`"),
suggestion,
Applicability::MachineApplicable,
);
for (span, msg, suggestion) in suggestions {
err.span_suggestion_verbose(span, &msg, suggestion, Applicability::MachineApplicable);
}
}

Expand Down Expand Up @@ -521,8 +549,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {

if binds_to.len() > 1 {
err.note(
"move occurs because these variables have types that \
don't implement the `Copy` trait",
"move occurs because these variables have types that don't implement the `Copy` \
trait",
);
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
remainder_span,
pattern,
None,
Some((None, initializer_span)),
Some((Some(&destination), initializer_span)),
);
this.visit_primary_bindings(
pattern,
Expand Down
13 changes: 9 additions & 4 deletions src/test/ui/borrowck/access-mode-in-closures.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,15 @@ error[E0507]: cannot move out of `s` which is behind a shared reference
|
LL | match *s { S(v) => v }
| ^^ -
| | |
| | data moved here
| | move occurs because `v` has type `Vec<isize>`, which does not implement the `Copy` trait
| help: consider borrowing here: `&*s`
| |
| data moved here
| move occurs because `v` has type `Vec<isize>`, which does not implement the `Copy` trait
|
help: consider removing the dereference here
|
LL - match *s { S(v) => v }
LL + match s { S(v) => v }
|

error: aborting due to previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,31 +2,46 @@ error[E0507]: cannot move out of a shared reference
--> $DIR/borrowck-for-loop-correct-cmt-for-pattern.rs:12:15
|
LL | for &a in x.iter() {
| -- ^^^^^^^^
| ||
| |data moved here
| |move occurs because `a` has type `&mut i32`, which does not implement the `Copy` trait
| help: consider removing the `&`: `a`
| - ^^^^^^^^
| |
| data moved here
| move occurs because `a` has type `&mut i32`, which does not implement the `Copy` trait
|
help: consider removing the borrow
|
LL - for &a in x.iter() {
LL + for a in x.iter() {
|

error[E0507]: cannot move out of a shared reference
--> $DIR/borrowck-for-loop-correct-cmt-for-pattern.rs:18:15
|
LL | for &a in &f.a {
| -- ^^^^
| ||
| |data moved here
| |move occurs because `a` has type `Box<isize>`, which does not implement the `Copy` trait
| help: consider removing the `&`: `a`
| - ^^^^
| |
| data moved here
| move occurs because `a` has type `Box<isize>`, which does not implement the `Copy` trait
|
help: consider removing the borrow
|
LL - for &a in &f.a {
LL + for a in &f.a {
|

error[E0507]: cannot move out of a shared reference
--> $DIR/borrowck-for-loop-correct-cmt-for-pattern.rs:22:15
|
LL | for &a in x.iter() {
| -- ^^^^^^^^
| ||
| |data moved here
| |move occurs because `a` has type `Box<i32>`, which does not implement the `Copy` trait
| help: consider removing the `&`: `a`
| - ^^^^^^^^
| |
| data moved here
| move occurs because `a` has type `Box<i32>`, which does not implement the `Copy` trait
|
help: consider removing the borrow
|
LL - for &a in x.iter() {
LL + for a in x.iter() {
|

error: aborting due to 3 previous errors

Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/borrowck/borrowck-issue-2657-2.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// run-rustfix
fn main() {

let x: Option<Box<_>> = Some(Box::new(1));

match x {
Some(ref y) => {
let _b = y; //~ ERROR cannot move out
}
_ => {}
}
}
1 change: 1 addition & 0 deletions src/test/ui/borrowck/borrowck-issue-2657-2.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// run-rustfix
fn main() {

let x: Option<Box<_>> = Some(Box::new(1));
Expand Down
13 changes: 8 additions & 5 deletions src/test/ui/borrowck/borrowck-issue-2657-2.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
error[E0507]: cannot move out of `*y` which is behind a shared reference
--> $DIR/borrowck-issue-2657-2.rs:7:18
--> $DIR/borrowck-issue-2657-2.rs:8:18
|
LL | let _b = *y;
| ^^
| |
| move occurs because `*y` has type `Box<i32>`, which does not implement the `Copy` trait
| help: consider borrowing here: `&*y`
| ^^ move occurs because `*y` has type `Box<i32>`, which does not implement the `Copy` trait
|
help: consider removing the dereference here
|
LL - let _b = *y;
LL + let _b = y;
|

error: aborting due to previous error

Expand Down
56 changes: 56 additions & 0 deletions src/test/ui/borrowck/borrowck-move-error-with-note.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// run-rustfix
#![allow(unused)]
enum Foo {
Foo1(Box<u32>, Box<u32>),
Foo2(Box<u32>),
Foo3,
}



fn blah() {
let f = &Foo::Foo1(Box::new(1), Box::new(2));
match f { //~ ERROR cannot move out of
Foo::Foo1(num1,
num2) => (),
Foo::Foo2(num) => (),
Foo::Foo3 => ()
}
}

struct S {
f: String,
g: String
}
impl Drop for S {
fn drop(&mut self) { println!("{}", self.f); }
}

fn move_in_match() {
match (S {f: "foo".to_string(), g: "bar".to_string()}) {
//~^ ERROR cannot move out of type `S`, which implements the `Drop` trait
S {
f: ref _s,
g: ref _t
} => {}
}
}

// from issue-8064
struct A {
a: Box<isize>,
}

fn free<T>(_: T) {}

fn blah2() {
let a = &A { a: Box::new(1) };
match &a.a { //~ ERROR cannot move out of
n => {
free(n)
}
}
free(a)
}

fn main() {}
2 changes: 2 additions & 0 deletions src/test/ui/borrowck/borrowck-move-error-with-note.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// run-rustfix
#![allow(unused)]
enum Foo {
Foo1(Box<u32>, Box<u32>),
Foo2(Box<u32>),
Expand Down
Loading