From 98cfed7b9756aac0f0f74d9bc307eac22151c17e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 20 Nov 2023 22:51:52 +0000 Subject: [PATCH] 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 = map.clone().into_values().collect(); | ^^^^^^^^^^^ ------------- value moved due to this method call | | | move occurs because value has type `HashMap`, which does not implement the `Copy` trait | note: `HashMap::::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 = 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. --- .../rustc_borrowck/src/diagnostics/mod.rs | 172 +++++++++++++----- tests/ui/borrowck/issue-83760.fixed | 47 +++++ tests/ui/borrowck/issue-83760.rs | 15 +- tests/ui/borrowck/issue-83760.stderr | 42 +++-- .../suggest-as-ref-on-mut-closure.stderr | 4 + tests/ui/moves/move-fn-self-receiver.stderr | 8 + ...-clone-when-some-obligation-is-unmet.fixed | 28 +++ ...est-clone-when-some-obligation-is-unmet.rs | 27 +++ ...clone-when-some-obligation-is-unmet.stderr | 23 +++ tests/ui/suggestions/as-ref-2.stderr | 9 + 10 files changed, 310 insertions(+), 65 deletions(-) create mode 100644 tests/ui/borrowck/issue-83760.fixed create mode 100644 tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.fixed create mode 100644 tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.rs create mode 100644 tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.stderr diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs index 31cb732cfd1..08c2a690b30 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mod.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs @@ -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, @@ -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, }; @@ -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) { @@ -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( @@ -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)) @@ -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::>(); + 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::>() + .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 diff --git a/tests/ui/borrowck/issue-83760.fixed b/tests/ui/borrowck/issue-83760.fixed new file mode 100644 index 00000000000..4544eeb6e19 --- /dev/null +++ b/tests/ui/borrowck/issue-83760.fixed @@ -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() {} diff --git a/tests/ui/borrowck/issue-83760.rs b/tests/ui/borrowck/issue-83760.rs index e25b4f72785..81bfdf0fcc7 100644 --- a/tests/ui/borrowck/issue-83760.rs +++ b/tests/ui/borrowck/issue-83760.rs @@ -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); @@ -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` diff --git a/tests/ui/borrowck/issue-83760.stderr b/tests/ui/borrowck/issue-83760.stderr index a585bff0c65..d120adbc03b 100644 --- a/tests/ui/borrowck/issue-83760.stderr +++ b/tests/ui/borrowck/issue-83760.stderr @@ -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 @@ -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`, which does not implement the `Copy` trait @@ -29,12 +29,21 @@ LL | let _y = foo; | note: `Option::::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`, which does not implement the `Copy` trait +LL | let mut foo = Some(Struct2); + | ------- move occurs because `foo` has type `Option`, which does not implement the `Copy` trait LL | let _x = foo.unwrap(); | -------- `foo` moved due to this method call ... @@ -42,18 +51,27 @@ 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::::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 diff --git a/tests/ui/borrowck/suggest-as-ref-on-mut-closure.stderr b/tests/ui/borrowck/suggest-as-ref-on-mut-closure.stderr index bada08368fc..1e98006a9a7 100644 --- a/tests/ui/borrowck/suggest-as-ref-on-mut-closure.stderr +++ b/tests/ui/borrowck/suggest-as-ref-on-mut-closure.stderr @@ -9,6 +9,10 @@ LL | cb.map(|cb| cb()); | note: `Option::::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 | 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 diff --git a/tests/ui/moves/move-fn-self-receiver.stderr b/tests/ui/moves/move-fn-self-receiver.stderr index c91a8b5efac..0abfcd112ef 100644 --- a/tests/ui/moves/move-fn-self-receiver.stderr +++ b/tests/ui/moves/move-fn-self-receiver.stderr @@ -55,6 +55,10 @@ note: `Foo::use_box_self` takes ownership of the receiver `self`, which moves `b | LL | fn use_box_self(self: Box) {} | ^^^^ +help: you can `clone` the value and consume it, but this might not be your desired behavior + | +LL | boxed_foo.clone().use_box_self(); + | ++++++++ error[E0382]: use of moved value: `pin_box_foo` --> $DIR/move-fn-self-receiver.rs:46:5 @@ -71,6 +75,10 @@ note: `Foo::use_pin_box_self` takes ownership of the receiver `self`, which move | LL | fn use_pin_box_self(self: Pin>) {} | ^^^^ +help: you could `clone` the value and consume it, if the `Box: Clone` trait bound could be satisfied + | +LL | pin_box_foo.clone().use_pin_box_self(); + | ++++++++ error[E0505]: cannot move out of `mut_foo` because it is borrowed --> $DIR/move-fn-self-receiver.rs:50:5 diff --git a/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.fixed b/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.fixed new file mode 100644 index 00000000000..a4e219e1c9b --- /dev/null +++ b/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.fixed @@ -0,0 +1,28 @@ +// run-rustfix +// Issue #109429 +use std::collections::hash_map::DefaultHasher; +use std::collections::HashMap; +use std::hash::BuildHasher; +use std::hash::Hash; + +#[derive(Clone)] +pub struct Hash128_1; + +impl BuildHasher for Hash128_1 { + type Hasher = DefaultHasher; + fn build_hasher(&self) -> DefaultHasher { DefaultHasher::default() } +} + +#[allow(unused)] +pub fn hashmap_copy( + map: &HashMap, +) where T: Hash + Clone, U: Clone +{ + let mut copy: Vec = as Clone>::clone(&map.clone()).into_values().collect(); //~ ERROR +} + +pub fn make_map() -> HashMap +{ + HashMap::with_hasher(Hash128_1) +} +fn main() {} diff --git a/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.rs b/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.rs new file mode 100644 index 00000000000..efe035ebae0 --- /dev/null +++ b/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.rs @@ -0,0 +1,27 @@ +// run-rustfix +// Issue #109429 +use std::collections::hash_map::DefaultHasher; +use std::collections::HashMap; +use std::hash::BuildHasher; +use std::hash::Hash; + +pub struct Hash128_1; + +impl BuildHasher for Hash128_1 { + type Hasher = DefaultHasher; + fn build_hasher(&self) -> DefaultHasher { DefaultHasher::default() } +} + +#[allow(unused)] +pub fn hashmap_copy( + map: &HashMap, +) where T: Hash + Clone, U: Clone +{ + let mut copy: Vec = map.clone().into_values().collect(); //~ ERROR +} + +pub fn make_map() -> HashMap +{ + HashMap::with_hasher(Hash128_1) +} +fn main() {} diff --git a/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.stderr b/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.stderr new file mode 100644 index 00000000000..0a8fdb72ce8 --- /dev/null +++ b/tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.stderr @@ -0,0 +1,23 @@ +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 = map.clone().into_values().collect(); + | ^^^^^^^^^^^ ------------- value moved due to this method call + | | + | move occurs because value has type `HashMap`, which does not implement the `Copy` trait + | +note: `HashMap::::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 = as Clone>::clone(&map.clone()).into_values().collect(); + | ++++++++++++++++++++++++++++++++++++++++++++ + +help: consider annotating `Hash128_1` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | pub struct Hash128_1; + | + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0507`. diff --git a/tests/ui/suggestions/as-ref-2.stderr b/tests/ui/suggestions/as-ref-2.stderr index 5432fcb1a58..30928577537 100644 --- a/tests/ui/suggestions/as-ref-2.stderr +++ b/tests/ui/suggestions/as-ref-2.stderr @@ -12,6 +12,15 @@ LL | let _y = foo; | note: `Option::::map` 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: Option = foo.clone().map(|s| bar(&s)); + | ++++++++ +help: consider annotating `Struct` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | struct Struct; + | error: aborting due to 1 previous error