Skip to content

Commit 98cfed7

Browse files
estebankestebank
committed
Suggest cloning and point out obligation errors on move error
When encountering a move error, look for implementations of `Clone` for the moved type. If there is one, check if all its obligations are met. If they are, we suggest cloning without caveats. If they aren't, we suggest cloning while mentioning the unmet obligations, potentially suggesting `#[derive(Clone)]` when appropriate. ``` error[E0507]: cannot move out of a shared reference --> $DIR/suggest-clone-when-some-obligation-is-unmet.rs:20:28 | LL | let mut copy: Vec<U> = map.clone().into_values().collect(); | ^^^^^^^^^^^ ------------- value moved due to this method call | | | move occurs because value has type `HashMap<T, U, Hash128_1>`, which does not implement the `Copy` trait | note: `HashMap::<K, V, S>::into_values` takes ownership of the receiver `self`, which moves value --> $SRC_DIR/std/src/collections/hash/map.rs:LL:COL help: you could `clone` the value and consume it, if the `Hash128_1: Clone` trait bound could be satisfied | LL | let mut copy: Vec<U> = <HashMap<T, U, Hash128_1> as Clone>::clone(&map.clone()).into_values().collect(); | ++++++++++++++++++++++++++++++++++++++++++++ + help: consider annotating `Hash128_1` with `#[derive(Clone)]` | LL + #[derive(Clone)] LL | pub struct Hash128_1; | ``` Fix rust-lang#109429.
1 parent 03c88aa commit 98cfed7

File tree

10 files changed

+310
-65
lines changed

10 files changed

+310
-65
lines changed

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

Lines changed: 124 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use rustc_hir::def::{CtorKind, Namespace};
1111
use rustc_hir::CoroutineKind;
1212
use rustc_index::IndexSlice;
1313
use rustc_infer::infer::BoundRegionConversionTime;
14+
use rustc_infer::traits::{FulfillmentErrorCode, SelectionError, TraitEngineExt};
1415
use rustc_middle::mir::tcx::PlaceTy;
1516
use rustc_middle::mir::{
1617
AggregateKind, CallSource, ConstOperand, FakeReadCause, Local, LocalInfo, LocalKind, Location,
@@ -24,7 +25,8 @@ use rustc_mir_dataflow::move_paths::{InitLocation, LookupResult};
2425
use rustc_span::def_id::LocalDefId;
2526
use rustc_span::{symbol::sym, Span, Symbol, DUMMY_SP};
2627
use rustc_target::abi::{FieldIdx, VariantIdx};
27-
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
28+
use rustc_trait_selection::solve::FulfillmentCtxt;
29+
use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt as _;
2830
use rustc_trait_selection::traits::{
2931
type_known_to_meet_bound_modulo_regions, Obligation, ObligationCause,
3032
};
@@ -1043,7 +1045,38 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
10431045
}
10441046
CallKind::Normal { self_arg, desugaring, method_did, method_args } => {
10451047
let self_arg = self_arg.unwrap();
1048+
let mut has_sugg = false;
10461049
let tcx = self.infcx.tcx;
1050+
// Avoid pointing to the same function in multiple different
1051+
// error messages.
1052+
if span != DUMMY_SP && self.fn_self_span_reported.insert(self_arg.span) {
1053+
self.explain_iterator_advancement_in_for_loop_if_applicable(
1054+
err,
1055+
span,
1056+
&move_spans,
1057+
);
1058+
1059+
let func = tcx.def_path_str(method_did);
1060+
err.subdiagnostic(CaptureReasonNote::FuncTakeSelf {
1061+
func,
1062+
place_name: place_name.clone(),
1063+
span: self_arg.span,
1064+
});
1065+
}
1066+
let parent_did = tcx.parent(method_did);
1067+
let parent_self_ty =
1068+
matches!(tcx.def_kind(parent_did), rustc_hir::def::DefKind::Impl { .. })
1069+
.then_some(parent_did)
1070+
.and_then(|did| match tcx.type_of(did).instantiate_identity().kind() {
1071+
ty::Adt(def, ..) => Some(def.did()),
1072+
_ => None,
1073+
});
1074+
let is_option_or_result = parent_self_ty.is_some_and(|def_id| {
1075+
matches!(tcx.get_diagnostic_name(def_id), Some(sym::Option | sym::Result))
1076+
});
1077+
if is_option_or_result && maybe_reinitialized_locations_is_empty {
1078+
err.subdiagnostic(CaptureReasonLabel::BorrowContent { var_span });
1079+
}
10471080
if let Some((CallDesugaringKind::ForLoopIntoIter, _)) = desugaring {
10481081
let ty = moved_place.ty(self.body, tcx).ty;
10491082
let suggest = match tcx.get_diagnostic_item(sym::IntoIterator) {
@@ -1108,7 +1141,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
11081141
// Erase and shadow everything that could be passed to the new infcx.
11091142
let ty = moved_place.ty(self.body, tcx).ty;
11101143

1111-
if let ty::Adt(def, args) = ty.kind()
1144+
if let ty::Adt(def, args) = ty.peel_refs().kind()
11121145
&& Some(def.did()) == tcx.lang_items().pin_type()
11131146
&& let ty::Ref(_, _, hir::Mutability::Mut) = args.type_at(0).kind()
11141147
&& let self_ty = self.infcx.instantiate_binder_with_fresh_vars(
@@ -1124,17 +1157,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
11241157
span: move_span.shrink_to_hi(),
11251158
},
11261159
);
1160+
has_sugg = true;
11271161
}
1128-
if let Some(clone_trait) = tcx.lang_items().clone_trait()
1129-
&& let trait_ref = ty::TraitRef::new(tcx, clone_trait, [ty])
1130-
&& let o = Obligation::new(
1131-
tcx,
1132-
ObligationCause::dummy(),
1133-
self.param_env,
1134-
ty::Binder::dummy(trait_ref),
1135-
)
1136-
&& self.infcx.predicate_must_hold_modulo_regions(&o)
1137-
{
1162+
if let Some(clone_trait) = tcx.lang_items().clone_trait() {
11381163
let sugg = if moved_place
11391164
.iter_projections()
11401165
.any(|(_, elem)| matches!(elem, ProjectionElem::Deref))
@@ -1150,43 +1175,94 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
11501175
} else {
11511176
vec![(move_span.shrink_to_hi(), ".clone()".to_string())]
11521177
};
1153-
err.multipart_suggestion_verbose(
1154-
"you can `clone` the value and consume it, but this might not be \
1155-
your desired behavior",
1156-
sugg,
1157-
Applicability::MaybeIncorrect,
1158-
);
1159-
}
1160-
}
1161-
// Avoid pointing to the same function in multiple different
1162-
// error messages.
1163-
if span != DUMMY_SP && self.fn_self_span_reported.insert(self_arg.span) {
1164-
self.explain_iterator_advancement_in_for_loop_if_applicable(
1165-
err,
1166-
span,
1167-
&move_spans,
1168-
);
1169-
1170-
let func = tcx.def_path_str(method_did);
1171-
err.subdiagnostic(CaptureReasonNote::FuncTakeSelf {
1172-
func,
1173-
place_name,
1174-
span: self_arg.span,
1175-
});
1176-
}
1177-
let parent_did = tcx.parent(method_did);
1178-
let parent_self_ty =
1179-
matches!(tcx.def_kind(parent_did), rustc_hir::def::DefKind::Impl { .. })
1180-
.then_some(parent_did)
1181-
.and_then(|did| match tcx.type_of(did).instantiate_identity().kind() {
1182-
ty::Adt(def, ..) => Some(def.did()),
1183-
_ => None,
1178+
self.infcx.probe(|_snapshot| {
1179+
if let ty::Adt(def, args) = ty.kind()
1180+
&& !has_sugg
1181+
&& let Some((def_id, _imp)) = tcx
1182+
.all_impls(clone_trait)
1183+
.filter_map(|def_id| {
1184+
tcx.impl_trait_ref(def_id).map(|r| (def_id, r))
1185+
})
1186+
.map(|(def_id, imp)| (def_id, imp.skip_binder()))
1187+
.filter(|(_, imp)| match imp.self_ty().peel_refs().kind() {
1188+
ty::Adt(i_def, _) if i_def.did() == def.did() => true,
1189+
_ => false,
1190+
})
1191+
.next()
1192+
{
1193+
let mut fulfill_cx = FulfillmentCtxt::new(self.infcx);
1194+
// We get all obligations from the impl to talk about specific
1195+
// trait bounds.
1196+
let obligations = tcx
1197+
.predicates_of(def_id)
1198+
.instantiate(tcx, args)
1199+
.into_iter()
1200+
.map(|(clause, span)| {
1201+
Obligation::new(
1202+
tcx,
1203+
ObligationCause::misc(
1204+
span,
1205+
self.body.source.def_id().expect_local(),
1206+
),
1207+
self.param_env,
1208+
clause,
1209+
)
1210+
})
1211+
.collect::<Vec<_>>();
1212+
fulfill_cx
1213+
.register_predicate_obligations(self.infcx, obligations);
1214+
let errors = fulfill_cx.select_all_or_error(self.infcx);
1215+
let msg = match &errors[..] {
1216+
[] => "you can `clone` the value and consume it, but this \
1217+
might not be your desired behavior"
1218+
.to_string(),
1219+
[error] => {
1220+
format!(
1221+
"you could `clone` the value and consume it, if \
1222+
the `{}` trait bound could be satisfied",
1223+
error.obligation.predicate,
1224+
)
1225+
}
1226+
[errors @ .., last] => {
1227+
format!(
1228+
"you could `clone` the value and consume it, if \
1229+
the following trait bounds could be satisfied: {} \
1230+
and `{}`",
1231+
errors
1232+
.iter()
1233+
.map(|e| format!(
1234+
"`{}`",
1235+
e.obligation.predicate
1236+
))
1237+
.collect::<Vec<_>>()
1238+
.join(", "),
1239+
last.obligation.predicate,
1240+
)
1241+
}
1242+
};
1243+
err.multipart_suggestion_verbose(
1244+
msg,
1245+
sugg.clone(),
1246+
Applicability::MaybeIncorrect,
1247+
);
1248+
for error in errors {
1249+
if let FulfillmentErrorCode::CodeSelectionError(
1250+
SelectionError::Unimplemented,
1251+
) = error.code
1252+
&& let ty::PredicateKind::Clause(ty::ClauseKind::Trait(
1253+
pred,
1254+
)) = error.obligation.predicate.kind().skip_binder()
1255+
{
1256+
self.infcx.err_ctxt().suggest_derive(
1257+
&error.obligation,
1258+
err,
1259+
error.obligation.predicate.kind().rebind(pred),
1260+
);
1261+
}
1262+
}
1263+
}
11841264
});
1185-
let is_option_or_result = parent_self_ty.is_some_and(|def_id| {
1186-
matches!(tcx.get_diagnostic_name(def_id), Some(sym::Option | sym::Result))
1187-
});
1188-
if is_option_or_result && maybe_reinitialized_locations_is_empty {
1189-
err.subdiagnostic(CaptureReasonLabel::BorrowContent { var_span });
1265+
}
11901266
}
11911267
}
11921268
// Other desugarings takes &self, which cannot cause a move

‎tests/ui/borrowck/issue-83760.fixed‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// run-rustfix
2+
#![allow(unused_variables, dead_code)]
3+
#[derive(Clone)]
4+
struct Struct;
5+
#[derive(Clone)]
6+
struct Struct2;
7+
// We use a second one here because otherwise when applying suggestions we'd end up with two
8+
// `#[derive(Clone)]` annotations.
9+
10+
fn test1() {
11+
let mut val = Some(Struct);
12+
while let Some(ref foo) = val { //~ ERROR use of moved value
13+
if true {
14+
val = None;
15+
} else {
16+
17+
}
18+
}
19+
}
20+
21+
fn test2() {
22+
let mut foo = Some(Struct);
23+
let _x = foo.clone().unwrap();
24+
if true {
25+
foo = Some(Struct);
26+
} else {
27+
}
28+
let _y = foo; //~ ERROR use of moved value: `foo`
29+
}
30+
31+
fn test3() {
32+
let mut foo = Some(Struct2);
33+
let _x = foo.clone().unwrap();
34+
if true {
35+
foo = Some(Struct2);
36+
} else if true {
37+
foo = Some(Struct2);
38+
} else if true {
39+
foo = Some(Struct2);
40+
} else if true {
41+
foo = Some(Struct2);
42+
} else {
43+
}
44+
let _y = foo; //~ ERROR use of moved value: `foo`
45+
}
46+
47+
fn main() {}

‎tests/ui/borrowck/issue-83760.rs‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
1+
// run-rustfix
2+
#![allow(unused_variables, dead_code)]
13
struct Struct;
4+
struct Struct2;
5+
// We use a second one here because otherwise when applying suggestions we'd end up with two
6+
// `#[derive(Clone)]` annotations.
27

38
fn test1() {
49
let mut val = Some(Struct);
@@ -22,16 +27,16 @@ fn test2() {
2227
}
2328

2429
fn test3() {
25-
let mut foo = Some(Struct);
30+
let mut foo = Some(Struct2);
2631
let _x = foo.unwrap();
2732
if true {
28-
foo = Some(Struct);
33+
foo = Some(Struct2);
2934
} else if true {
30-
foo = Some(Struct);
35+
foo = Some(Struct2);
3136
} else if true {
32-
foo = Some(Struct);
37+
foo = Some(Struct2);
3338
} else if true {
34-
foo = Some(Struct);
39+
foo = Some(Struct2);
3540
} else {
3641
}
3742
let _y = foo; //~ ERROR use of moved value: `foo`

‎tests/ui/borrowck/issue-83760.stderr‎

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0382]: use of moved value
2-
--> $DIR/issue-83760.rs:5:20
2+
--> $DIR/issue-83760.rs:10:20
33
|
44
LL | while let Some(foo) = val {
55
| ^^^ value moved here, in previous iteration of loop
@@ -14,7 +14,7 @@ LL | while let Some(ref foo) = val {
1414
| +++
1515

1616
error[E0382]: use of moved value: `foo`
17-
--> $DIR/issue-83760.rs:21:14
17+
--> $DIR/issue-83760.rs:26:14
1818
|
1919
LL | let mut foo = Some(Struct);
2020
| ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
@@ -29,31 +29,49 @@ LL | let _y = foo;
2929
|
3030
note: `Option::<T>::unwrap` takes ownership of the receiver `self`, which moves `foo`
3131
--> $SRC_DIR/core/src/option.rs:LL:COL
32+
help: you could `clone` the value and consume it, if the `Struct: Clone` trait bound could be satisfied
33+
|
34+
LL | let _x = foo.clone().unwrap();
35+
| ++++++++
36+
help: consider annotating `Struct` with `#[derive(Clone)]`
37+
|
38+
LL + #[derive(Clone)]
39+
LL | struct Struct;
40+
|
3241

3342
error[E0382]: use of moved value: `foo`
34-
--> $DIR/issue-83760.rs:37:14
43+
--> $DIR/issue-83760.rs:42:14
3544
|
36-
LL | let mut foo = Some(Struct);
37-
| ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
45+
LL | let mut foo = Some(Struct2);
46+
| ------- move occurs because `foo` has type `Option<Struct2>`, which does not implement the `Copy` trait
3847
LL | let _x = foo.unwrap();
3948
| -------- `foo` moved due to this method call
4049
...
4150
LL | let _y = foo;
4251
| ^^^ value used here after move
4352
|
4453
note: these 3 reinitializations and 1 other might get skipped
45-
--> $DIR/issue-83760.rs:30:9
54+
--> $DIR/issue-83760.rs:35:9
4655
|
47-
LL | foo = Some(Struct);
48-
| ^^^^^^^^^^^^^^^^^^
56+
LL | foo = Some(Struct2);
57+
| ^^^^^^^^^^^^^^^^^^^
4958
LL | } else if true {
50-
LL | foo = Some(Struct);
51-
| ^^^^^^^^^^^^^^^^^^
59+
LL | foo = Some(Struct2);
60+
| ^^^^^^^^^^^^^^^^^^^
5261
LL | } else if true {
53-
LL | foo = Some(Struct);
54-
| ^^^^^^^^^^^^^^^^^^
62+
LL | foo = Some(Struct2);
63+
| ^^^^^^^^^^^^^^^^^^^
5564
note: `Option::<T>::unwrap` takes ownership of the receiver `self`, which moves `foo`
5665
--> $SRC_DIR/core/src/option.rs:LL:COL
66+
help: you could `clone` the value and consume it, if the `Struct2: Clone` trait bound could be satisfied
67+
|
68+
LL | let _x = foo.clone().unwrap();
69+
| ++++++++
70+
help: consider annotating `Struct2` with `#[derive(Clone)]`
71+
|
72+
LL + #[derive(Clone)]
73+
LL | struct Struct2;
74+
|
5775

5876
error: aborting due to 3 previous errors
5977

‎tests/ui/borrowck/suggest-as-ref-on-mut-closure.stderr‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@ LL | cb.map(|cb| cb());
99
|
1010
note: `Option::<T>::map` takes ownership of the receiver `self`, which moves `*cb`
1111
--> $SRC_DIR/core/src/option.rs:LL:COL
12+
help: you could `clone` the value and consume it, if the `&mut dyn FnMut(): Clone` trait bound could be satisfied
13+
|
14+
LL | <Option<&mut dyn FnMut()> as Clone>::clone(&cb).map(|cb| cb());
15+
| ++++++++++++++++++++++++++++++++++++++++++++ +
1216

1317
error[E0596]: cannot borrow `*cb` as mutable, as it is behind a `&` reference
1418
--> $DIR/suggest-as-ref-on-mut-closure.rs:12:26

0 commit comments

Comments
 (0)