Skip to content

Commit

Permalink
Suggest cloning and point out obligation errors on move error
Browse files Browse the repository at this point in the history 
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 #109429.
  • Loading branch information
@estebank
estebank committed Dec 4, 2023
1 parent 03c88aa commit 98cfed7
Show file tree
Hide file tree
Showing 10 changed files with 310 additions and 65 deletions.
172 changes: 124 additions & 48 deletions compiler/rustc_borrowck/src/diagnostics/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use rustc_hir::def::{CtorKind, Namespace};
use rustc_hir::CoroutineKind;
use rustc_index::IndexSlice;
use rustc_infer::infer::BoundRegionConversionTime;
use rustc_infer::traits::{FulfillmentErrorCode, SelectionError, TraitEngineExt};
use rustc_middle::mir::tcx::PlaceTy;
use rustc_middle::mir::{
AggregateKind, CallSource, ConstOperand, FakeReadCause, Local, LocalInfo, LocalKind, Location,
Expand All @@ -24,7 +25,8 @@ use rustc_mir_dataflow::move_paths::{InitLocation, LookupResult};
use rustc_span::def_id::LocalDefId;
use rustc_span::{symbol::sym, Span, Symbol, DUMMY_SP};
use rustc_target::abi::{FieldIdx, VariantIdx};
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
use rustc_trait_selection::solve::FulfillmentCtxt;
use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt as _;
use rustc_trait_selection::traits::{
type_known_to_meet_bound_modulo_regions, Obligation, ObligationCause,
};
Expand Down Expand Up @@ -1043,7 +1045,38 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
CallKind::Normal { self_arg, desugaring, method_did, method_args } => {
let self_arg = self_arg.unwrap();
let mut has_sugg = false;
let tcx = self.infcx.tcx;
// Avoid pointing to the same function in multiple different
// error messages.
if span != DUMMY_SP && self.fn_self_span_reported.insert(self_arg.span) {
self.explain_iterator_advancement_in_for_loop_if_applicable(
err,
span,
&move_spans,
);

let func = tcx.def_path_str(method_did);
err.subdiagnostic(CaptureReasonNote::FuncTakeSelf {
func,
place_name: place_name.clone(),
span: self_arg.span,
});
}
let parent_did = tcx.parent(method_did);
let parent_self_ty =
matches!(tcx.def_kind(parent_did), rustc_hir::def::DefKind::Impl { .. })
.then_some(parent_did)
.and_then(|did| match tcx.type_of(did).instantiate_identity().kind() {
ty::Adt(def, ..) => Some(def.did()),
_ => None,
});
let is_option_or_result = parent_self_ty.is_some_and(|def_id| {
matches!(tcx.get_diagnostic_name(def_id), Some(sym::Option | sym::Result))
});
if is_option_or_result && maybe_reinitialized_locations_is_empty {
err.subdiagnostic(CaptureReasonLabel::BorrowContent { var_span });
}
if let Some((CallDesugaringKind::ForLoopIntoIter, _)) = desugaring {
let ty = moved_place.ty(self.body, tcx).ty;
let suggest = match tcx.get_diagnostic_item(sym::IntoIterator) {
Expand Down Expand Up @@ -1108,7 +1141,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// Erase and shadow everything that could be passed to the new infcx.
let ty = moved_place.ty(self.body, tcx).ty;

if let ty::Adt(def, args) = ty.kind()
if let ty::Adt(def, args) = ty.peel_refs().kind()
&& Some(def.did()) == tcx.lang_items().pin_type()
&& let ty::Ref(_, _, hir::Mutability::Mut) = args.type_at(0).kind()
&& let self_ty = self.infcx.instantiate_binder_with_fresh_vars(
Expand All @@ -1124,17 +1157,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
span: move_span.shrink_to_hi(),
},
);
has_sugg = true;
}
if let Some(clone_trait) = tcx.lang_items().clone_trait()
&& let trait_ref = ty::TraitRef::new(tcx, clone_trait, [ty])
&& let o = Obligation::new(
tcx,
ObligationCause::dummy(),
self.param_env,
ty::Binder::dummy(trait_ref),
)
&& self.infcx.predicate_must_hold_modulo_regions(&o)
{
if let Some(clone_trait) = tcx.lang_items().clone_trait() {
let sugg = if moved_place
.iter_projections()
.any(|(_, elem)| matches!(elem, ProjectionElem::Deref))
Expand All @@ -1150,43 +1175,94 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
} else {
vec![(move_span.shrink_to_hi(), ".clone()".to_string())]
};
err.multipart_suggestion_verbose(
"you can `clone` the value and consume it, but this might not be \
your desired behavior",
sugg,
Applicability::MaybeIncorrect,
);
}
}
// Avoid pointing to the same function in multiple different
// error messages.
if span != DUMMY_SP && self.fn_self_span_reported.insert(self_arg.span) {
self.explain_iterator_advancement_in_for_loop_if_applicable(
err,
span,
&move_spans,
);

let func = tcx.def_path_str(method_did);
err.subdiagnostic(CaptureReasonNote::FuncTakeSelf {
func,
place_name,
span: self_arg.span,
});
}
let parent_did = tcx.parent(method_did);
let parent_self_ty =
matches!(tcx.def_kind(parent_did), rustc_hir::def::DefKind::Impl { .. })
.then_some(parent_did)
.and_then(|did| match tcx.type_of(did).instantiate_identity().kind() {
ty::Adt(def, ..) => Some(def.did()),
_ => None,
self.infcx.probe(|_snapshot| {
if let ty::Adt(def, args) = ty.kind()
&& !has_sugg
&& let Some((def_id, _imp)) = tcx
.all_impls(clone_trait)
.filter_map(|def_id| {
tcx.impl_trait_ref(def_id).map(|r| (def_id, r))
})
.map(|(def_id, imp)| (def_id, imp.skip_binder()))
.filter(|(_, imp)| match imp.self_ty().peel_refs().kind() {
ty::Adt(i_def, _) if i_def.did() == def.did() => true,
_ => false,
})
.next()
{
let mut fulfill_cx = FulfillmentCtxt::new(self.infcx);
// We get all obligations from the impl to talk about specific
// trait bounds.
let obligations = tcx
.predicates_of(def_id)
.instantiate(tcx, args)
.into_iter()
.map(|(clause, span)| {
Obligation::new(
tcx,
ObligationCause::misc(
span,
self.body.source.def_id().expect_local(),
),
self.param_env,
clause,
)
})
.collect::<Vec<_>>();
fulfill_cx
.register_predicate_obligations(self.infcx, obligations);
let errors = fulfill_cx.select_all_or_error(self.infcx);
let msg = match &errors[..] {
[] => "you can `clone` the value and consume it, but this \
might not be your desired behavior"
.to_string(),
[error] => {
format!(
"you could `clone` the value and consume it, if \
the `{}` trait bound could be satisfied",
error.obligation.predicate,
)
}
[errors @ .., last] => {
format!(
"you could `clone` the value and consume it, if \
the following trait bounds could be satisfied: {} \
and `{}`",
errors
.iter()
.map(|e| format!(
"`{}`",
e.obligation.predicate
))
.collect::<Vec<_>>()
.join(", "),
last.obligation.predicate,
)
}
};
err.multipart_suggestion_verbose(
msg,
sugg.clone(),
Applicability::MaybeIncorrect,
);
for error in errors {
if let FulfillmentErrorCode::CodeSelectionError(
SelectionError::Unimplemented,
) = error.code
&& let ty::PredicateKind::Clause(ty::ClauseKind::Trait(
pred,
)) = error.obligation.predicate.kind().skip_binder()
{
self.infcx.err_ctxt().suggest_derive(
&error.obligation,
err,
error.obligation.predicate.kind().rebind(pred),
);
}
}
}
});
let is_option_or_result = parent_self_ty.is_some_and(|def_id| {
matches!(tcx.get_diagnostic_name(def_id), Some(sym::Option | sym::Result))
});
if is_option_or_result && maybe_reinitialized_locations_is_empty {
err.subdiagnostic(CaptureReasonLabel::BorrowContent { var_span });
}
}
}
// Other desugarings takes &self, which cannot cause a move
Expand Down
47 changes: 47 additions & 0 deletions tests/ui/borrowck/issue-83760.fixed
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// run-rustfix
#![allow(unused_variables, dead_code)]
#[derive(Clone)]
struct Struct;
#[derive(Clone)]
struct Struct2;
// We use a second one here because otherwise when applying suggestions we'd end up with two
// `#[derive(Clone)]` annotations.

fn test1() {
let mut val = Some(Struct);
while let Some(ref foo) = val { //~ ERROR use of moved value
if true {
val = None;
} else {

}
}
}

fn test2() {
let mut foo = Some(Struct);
let _x = foo.clone().unwrap();
if true {
foo = Some(Struct);
} else {
}
let _y = foo; //~ ERROR use of moved value: `foo`
}

fn test3() {
let mut foo = Some(Struct2);
let _x = foo.clone().unwrap();
if true {
foo = Some(Struct2);
} else if true {
foo = Some(Struct2);
} else if true {
foo = Some(Struct2);
} else if true {
foo = Some(Struct2);
} else {
}
let _y = foo; //~ ERROR use of moved value: `foo`
}

fn main() {}
15 changes: 10 additions & 5 deletions tests/ui/borrowck/issue-83760.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
// run-rustfix
#![allow(unused_variables, dead_code)]
struct Struct;
struct Struct2;
// We use a second one here because otherwise when applying suggestions we'd end up with two
// `#[derive(Clone)]` annotations.

fn test1() {
let mut val = Some(Struct);
Expand All @@ -22,16 +27,16 @@ fn test2() {
}

fn test3() {
let mut foo = Some(Struct);
let mut foo = Some(Struct2);
let _x = foo.unwrap();
if true {
foo = Some(Struct);
foo = Some(Struct2);
} else if true {
foo = Some(Struct);
foo = Some(Struct2);
} else if true {
foo = Some(Struct);
foo = Some(Struct2);
} else if true {
foo = Some(Struct);
foo = Some(Struct2);
} else {
}
let _y = foo; //~ ERROR use of moved value: `foo`
Expand Down
42 changes: 30 additions & 12 deletions tests/ui/borrowck/issue-83760.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0382]: use of moved value
--> $DIR/issue-83760.rs:5:20
--> $DIR/issue-83760.rs:10:20
|
LL | while let Some(foo) = val {
| ^^^ value moved here, in previous iteration of loop
Expand All @@ -14,7 +14,7 @@ LL | while let Some(ref foo) = val {
| +++

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

error[E0382]: use of moved value: `foo`
--> $DIR/issue-83760.rs:37:14
--> $DIR/issue-83760.rs:42:14
|
LL | let mut foo = Some(Struct);
| ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
LL | let mut foo = Some(Struct2);
| ------- move occurs because `foo` has type `Option<Struct2>`, which does not implement the `Copy` trait
LL | let _x = foo.unwrap();
| -------- `foo` moved due to this method call
...
LL | let _y = foo;
| ^^^ value used here after move
|
note: these 3 reinitializations and 1 other might get skipped
--> $DIR/issue-83760.rs:30:9
--> $DIR/issue-83760.rs:35:9
|
LL | foo = Some(Struct);
| ^^^^^^^^^^^^^^^^^^
LL | foo = Some(Struct2);
| ^^^^^^^^^^^^^^^^^^^
LL | } else if true {
LL | foo = Some(Struct);
| ^^^^^^^^^^^^^^^^^^
LL | foo = Some(Struct2);
| ^^^^^^^^^^^^^^^^^^^
LL | } else if true {
LL | foo = Some(Struct);
| ^^^^^^^^^^^^^^^^^^
LL | foo = Some(Struct2);
| ^^^^^^^^^^^^^^^^^^^
note: `Option::<T>::unwrap` takes ownership of the receiver `self`, which moves `foo`
--> $SRC_DIR/core/src/option.rs:LL:COL
help: you could `clone` the value and consume it, if the `Struct2: Clone` trait bound could be satisfied
|
LL | let _x = foo.clone().unwrap();
| ++++++++
help: consider annotating `Struct2` with `#[derive(Clone)]`
|
LL + #[derive(Clone)]
LL | struct Struct2;
|

error: aborting due to 3 previous errors

Expand Down
4 changes: 4 additions & 0 deletions tests/ui/borrowck/suggest-as-ref-on-mut-closure.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ LL | cb.map(|cb| cb());
|
note: `Option::<T>::map` takes ownership of the receiver `self`, which moves `*cb`
--> $SRC_DIR/core/src/option.rs:LL:COL
help: you could `clone` the value and consume it, if the `&mut dyn FnMut(): Clone` trait bound could be satisfied
|
LL | <Option<&mut dyn FnMut()> as Clone>::clone(&cb).map(|cb| cb());
| ++++++++++++++++++++++++++++++++++++++++++++ +

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

0 comments on commit 98cfed7

Please sign in to comment.