Skip to content

Commit 9b7f17f

Browse files
committed
Implement clone suggestion on move errors
This is continuation on the "copy suggestions". The logic is as follows: - If the type is already `Clone` - clone it - If it can be `Copy` - make it copy - If it can be `Clone` - make it clone and the clone it - Otherwise feel sad Note that clone suggestions may be incorrect in many cases, like for example `S{t}; t` that suggests `S{t.clone()}; t`. See `use_of_moved_value_clone_suggestions_bad.rs` for more problematic cases.
1 parent 35c9ddf commit 9b7f17f

File tree

4 files changed

+122
-13
lines changed

4 files changed

+122
-13
lines changed

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

Lines changed: 57 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -175,12 +175,15 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
175175
let mut is_loop_move = false;
176176
let mut in_pattern = false;
177177

178+
let mut to_clone_spans = Vec::new();
179+
178180
for move_site in &move_site_vec {
179181
let move_out = self.move_data.moves[(*move_site).moi];
180182
let moved_place = &self.move_data.move_paths[move_out.path].place;
181183

182184
let move_spans = self.move_spans(moved_place.as_ref(), move_out.source);
183185
let move_span = move_spans.args_or_use();
186+
to_clone_spans.push(move_spans.var_or_use_path_span());
184187

185188
let move_msg = if move_spans.for_closure() { " into closure" } else { "" };
186189

@@ -293,18 +296,62 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
293296
.and_then(|def_id| tcx.hir().get_generics(def_id))
294297
{
295298
let copy_did = tcx.lang_items().copy_trait().unwrap();
296-
let predicates =
299+
let copy_predicates =
297300
self.try_find_missing_generic_bounds(ty, copy_did, generics, span);
298301

299-
if let Ok(predicates) = predicates {
300-
suggest_constraining_type_params(
301-
tcx,
302-
hir_generics,
303-
&mut err,
304-
predicates.iter().map(|(param, constraint)| {
305-
(param.name.as_str(), &**constraint, None)
306-
}),
307-
);
302+
let clone_did = tcx.lang_items().clone_trait().unwrap();
303+
let clone_predicates =
304+
self.try_find_missing_generic_bounds(ty, clone_did, generics, span);
305+
306+
match (copy_predicates, clone_predicates) {
307+
// The type is already `Clone`, suggest cloning all values
308+
(_, Ok(clone_predicates)) if clone_predicates.is_empty() => {
309+
err.multipart_suggestion_verbose(
310+
&format!("consider cloning {}", note_msg),
311+
to_clone_spans
312+
.into_iter()
313+
.map(|move_span| {
314+
(move_span.shrink_to_hi(), ".clone()".to_owned())
315+
})
316+
.collect(),
317+
Applicability::MaybeIncorrect,
318+
);
319+
}
320+
// The type can *not* be `Copy`, but can be `Clone`, suggest adding bounds to make the type `Clone` and then cloning all values
321+
(Err(_), Ok(clone_predicates)) => {
322+
suggest_constraining_type_params(
323+
tcx,
324+
hir_generics,
325+
&mut err,
326+
clone_predicates.iter().map(|(param, constraint)| {
327+
(param.name.as_str(), &**constraint, None)
328+
}),
329+
);
330+
331+
err.multipart_suggestion_verbose(
332+
&format!("...and cloning {}", note_msg),
333+
to_clone_spans
334+
.into_iter()
335+
.map(|move_span| {
336+
(move_span.shrink_to_hi(), ".clone()".to_owned())
337+
})
338+
.collect(),
339+
Applicability::MaybeIncorrect,
340+
);
341+
}
342+
// The type can be `Copy`, suggest adding bound to make it `Copy`
343+
(Ok(copy_predicates), _) => {
344+
suggest_constraining_type_params(
345+
tcx,
346+
hir_generics,
347+
&mut err,
348+
copy_predicates.iter().map(|(param, constraint)| {
349+
(param.name.as_str(), &**constraint, None)
350+
}),
351+
);
352+
}
353+
// The type can never be `Clone` or `Copy`, there is nothing to suggest :(
354+
(Err(_), Err(_)) => {}
308355
}
309356
}
310357

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,32 @@
1-
// `Rc` is not ever `Copy`, we should not suggest adding `T: Copy` constraint
2-
fn duplicate_rc<T>(t: std::rc::Rc<T>) -> (std::rc::Rc<T>, std::rc::Rc<T>) {
3-
(t, t) //~ use of moved value: `t`
1+
// run-rustfix
2+
#![allow(dead_code)]
3+
4+
// `Rc` is not ever `Copy`, we should not suggest adding `T: Copy` constraint.
5+
// But should suggest adding `.clone()`.
6+
fn move_rc<T>(t: std::rc::Rc<T>) {
7+
[t, t]; //~ use of moved value: `t`
8+
}
9+
10+
// Even though `T` could be `Copy` it's already `Clone` so don't suggest adding `T: Copy` constraint,
11+
// instead suggest adding `.clone()`.
12+
fn move_clone_already<T: Clone>(t: T) {
13+
[t, t]; //~ use of moved value: `t`
14+
}
15+
16+
// Same as `Rc`
17+
fn move_clone_only<T>(t: (T, String)) {
18+
[t, t]; //~ use of moved value: `t`
19+
}
20+
21+
// loop
22+
fn move_in_a_loop<T: Clone>(t: T) {
23+
loop {
24+
if true {
25+
drop(t); //~ use of moved value: `t`
26+
} else {
27+
drop(t);
28+
}
29+
}
430
}
531

632
fn main() {}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// Bad `.clone()` suggestions
2+
3+
struct S<A> {
4+
t: A,
5+
}
6+
7+
fn struct_field_shortcut_move<T: Clone>(t: T) {
8+
S { t };
9+
t; //~ use of moved value: `t`
10+
}
11+
12+
fn closure_clone_will_not_help<T: Clone>(t: T) {
13+
(move || {
14+
t;
15+
})();
16+
t; //~ use of moved value: `t`
17+
}
18+
19+
#[derive(Clone)]
20+
struct CloneOnly;
21+
22+
fn update_syntax<T: Clone>(s: S<T>) {
23+
S { ..s };
24+
S { ..s }; //~ use of moved value: `s.t`
25+
}
26+
27+
fn main() {}
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// No suggestions? :(
2+
3+
// In the future, we may want to suggest deriving `Clone, Copy` for `No` (and then adding `T: Copy`)
4+
struct No;
5+
fn move_non_clone_non_copy<T>(t: (T, No)) {
6+
[t, t]; //~ use of moved value: `t`
7+
}
8+
9+
fn main() {}

0 commit comments

Comments
 (0)