From 094cf6a4ce029d59312bde233027c3cfde29acc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 19 Nov 2023 23:53:31 +0000 Subject: [PATCH 01/12] Tweak `.clone()` suggestion to work in more cases When going through auto-deref, the `` impl sometimes needs to be specified for rustc to actually clone the value and not the reference. ``` error[E0507]: cannot move out of dereference of `S` --> $DIR/needs-clone-through-deref.rs:15:18 | LL | for _ in self.clone().into_iter() {} | ^^^^^^^^^^^^ ----------- value moved due to this method call | | | move occurs because value has type `Vec`, which does not implement the `Copy` trait | note: `into_iter` takes ownership of the receiver `self`, which moves value --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL help: you can `clone` the value and consume it, but this might not be your desired behavior | LL | for _ in as Clone>::clone(&self.clone()).into_iter() {} | ++++++++++++++++++++++++++++++ + ``` CC #109429. --- .../rustc_borrowck/src/diagnostics/mod.rs | 20 ++++++++++++++++--- ...k-move-out-of-overloaded-auto-deref.stderr | 4 ++-- .../borrowck/clone-span-on-try-operator.fixed | 2 +- .../clone-span-on-try-operator.stderr | 4 ++-- ...move-upvar-from-non-once-ref-closure.fixed | 2 +- ...ove-upvar-from-non-once-ref-closure.stderr | 4 ++-- .../ui/moves/needs-clone-through-deref.fixed | 18 +++++++++++++++++ tests/ui/moves/needs-clone-through-deref.rs | 18 +++++++++++++++++ .../ui/moves/needs-clone-through-deref.stderr | 18 +++++++++++++++++ tests/ui/moves/suggest-clone.fixed | 2 +- tests/ui/moves/suggest-clone.stderr | 4 ++-- .../ui/suggestions/option-content-move.stderr | 8 ++++---- 12 files changed, 86 insertions(+), 18 deletions(-) create mode 100644 tests/ui/moves/needs-clone-through-deref.fixed create mode 100644 tests/ui/moves/needs-clone-through-deref.rs create mode 100644 tests/ui/moves/needs-clone-through-deref.stderr diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs index c85b2f0a9d751..e0f2777dd476f 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mod.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs @@ -1135,11 +1135,25 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { ) && self.infcx.predicate_must_hold_modulo_regions(&o) { - err.span_suggestion_verbose( - move_span.shrink_to_hi(), + let sugg = if moved_place + .iter_projections() + .any(|(_, elem)| matches!(elem, ProjectionElem::Deref)) + { + vec![ + // We use the fully-qualified path because `.clone()` can + // sometimes choose `<&T as Clone>` instead of `` + // when going through auto-deref, so this ensures that doesn't + // happen, causing suggestions for `.clone().clone()`. + (move_span.shrink_to_lo(), format!("<{ty} as Clone>::clone(&")), + (move_span.shrink_to_hi(), ")".to_string()), + ] + } 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", - ".clone()".to_string(), + sugg, Applicability::MaybeIncorrect, ); } diff --git a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr index 934dd8df1d2d7..a86afab3a12db 100644 --- a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr +++ b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr @@ -10,8 +10,8 @@ note: `into_iter` takes ownership of the receiver `self`, which moves value --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL help: you can `clone` the value and consume it, but this might not be your desired behavior | -LL | let _x = Rc::new(vec![1, 2]).clone().into_iter(); - | ++++++++ +LL | let _x = as Clone>::clone(&Rc::new(vec![1, 2])).into_iter(); + | ++++++++++++++++++++++++++++ + error: aborting due to previous error diff --git a/tests/ui/borrowck/clone-span-on-try-operator.fixed b/tests/ui/borrowck/clone-span-on-try-operator.fixed index 52f66e43a930a..4fad75b9a3d61 100644 --- a/tests/ui/borrowck/clone-span-on-try-operator.fixed +++ b/tests/ui/borrowck/clone-span-on-try-operator.fixed @@ -7,5 +7,5 @@ impl Foo { } fn main() { let foo = &Foo; - (*foo).clone().foo(); //~ ERROR cannot move out + ::clone(&(*foo)).foo(); //~ ERROR cannot move out } diff --git a/tests/ui/borrowck/clone-span-on-try-operator.stderr b/tests/ui/borrowck/clone-span-on-try-operator.stderr index 85785e670729d..685ef19e26027 100644 --- a/tests/ui/borrowck/clone-span-on-try-operator.stderr +++ b/tests/ui/borrowck/clone-span-on-try-operator.stderr @@ -13,8 +13,8 @@ LL | fn foo(self) {} | ^^^^ help: you can `clone` the value and consume it, but this might not be your desired behavior | -LL | (*foo).clone().foo(); - | ++++++++ +LL | ::clone(&(*foo)).foo(); + | +++++++++++++++++++++++ + error: aborting due to previous error diff --git a/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.fixed b/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.fixed index b0c5376105b28..85acafd88f671 100644 --- a/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.fixed +++ b/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.fixed @@ -9,7 +9,7 @@ fn call(f: F) where F : Fn() { fn main() { let y = vec![format!("World")]; call(|| { - y.clone().into_iter(); + as Clone>::clone(&y).into_iter(); //~^ ERROR cannot move out of `y`, a captured variable in an `Fn` closure }); } diff --git a/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.stderr b/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.stderr index a2f6365b74e60..2b17755d6ac99 100644 --- a/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.stderr +++ b/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.stderr @@ -14,8 +14,8 @@ note: `into_iter` takes ownership of the receiver `self`, which moves `y` --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL help: you can `clone` the value and consume it, but this might not be your desired behavior | -LL | y.clone().into_iter(); - | ++++++++ +LL | as Clone>::clone(&y).into_iter(); + | +++++++++++++++++++++++++++++++ + error: aborting due to previous error diff --git a/tests/ui/moves/needs-clone-through-deref.fixed b/tests/ui/moves/needs-clone-through-deref.fixed new file mode 100644 index 0000000000000..419718175e98c --- /dev/null +++ b/tests/ui/moves/needs-clone-through-deref.fixed @@ -0,0 +1,18 @@ +// run-rustfix +#![allow(dead_code, noop_method_call)] +use std::ops::Deref; +struct S(Vec); +impl Deref for S { + type Target = Vec; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl S { + fn foo(&self) { + // `self.clone()` returns `&S`, not `Vec` + for _ in as Clone>::clone(&self.clone()).into_iter() {} //~ ERROR cannot move out of dereference of `S` + } +} +fn main() {} diff --git a/tests/ui/moves/needs-clone-through-deref.rs b/tests/ui/moves/needs-clone-through-deref.rs new file mode 100644 index 0000000000000..8116008ffe3cb --- /dev/null +++ b/tests/ui/moves/needs-clone-through-deref.rs @@ -0,0 +1,18 @@ +// run-rustfix +#![allow(dead_code, noop_method_call)] +use std::ops::Deref; +struct S(Vec); +impl Deref for S { + type Target = Vec; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl S { + fn foo(&self) { + // `self.clone()` returns `&S`, not `Vec` + for _ in self.clone().into_iter() {} //~ ERROR cannot move out of dereference of `S` + } +} +fn main() {} diff --git a/tests/ui/moves/needs-clone-through-deref.stderr b/tests/ui/moves/needs-clone-through-deref.stderr new file mode 100644 index 0000000000000..b6da6198af7a0 --- /dev/null +++ b/tests/ui/moves/needs-clone-through-deref.stderr @@ -0,0 +1,18 @@ +error[E0507]: cannot move out of dereference of `S` + --> $DIR/needs-clone-through-deref.rs:15:18 + | +LL | for _ in self.clone().into_iter() {} + | ^^^^^^^^^^^^ ----------- value moved due to this method call + | | + | move occurs because value has type `Vec`, which does not implement the `Copy` trait + | +note: `into_iter` takes ownership of the receiver `self`, which moves value + --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL +help: you can `clone` the value and consume it, but this might not be your desired behavior + | +LL | for _ in as Clone>::clone(&self.clone()).into_iter() {} + | ++++++++++++++++++++++++++++++ + + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0507`. diff --git a/tests/ui/moves/suggest-clone.fixed b/tests/ui/moves/suggest-clone.fixed index 204bfdb10b0b3..0c4a94d77e4be 100644 --- a/tests/ui/moves/suggest-clone.fixed +++ b/tests/ui/moves/suggest-clone.fixed @@ -7,5 +7,5 @@ impl Foo { } fn main() { let foo = &Foo; - foo.clone().foo(); //~ ERROR cannot move out + ::clone(&foo).foo(); //~ ERROR cannot move out } diff --git a/tests/ui/moves/suggest-clone.stderr b/tests/ui/moves/suggest-clone.stderr index 065acf904a435..f9ead06c8c810 100644 --- a/tests/ui/moves/suggest-clone.stderr +++ b/tests/ui/moves/suggest-clone.stderr @@ -13,8 +13,8 @@ LL | fn foo(self) {} | ^^^^ help: you can `clone` the value and consume it, but this might not be your desired behavior | -LL | foo.clone().foo(); - | ++++++++ +LL | ::clone(&foo).foo(); + | +++++++++++++++++++++++ + error: aborting due to previous error diff --git a/tests/ui/suggestions/option-content-move.stderr b/tests/ui/suggestions/option-content-move.stderr index 5060606d842a7..d4b73706fcae1 100644 --- a/tests/ui/suggestions/option-content-move.stderr +++ b/tests/ui/suggestions/option-content-move.stderr @@ -11,8 +11,8 @@ note: `Option::::unwrap` takes ownership of the receiver `self`, which moves --> $SRC_DIR/core/src/option.rs:LL:COL help: you can `clone` the value and consume it, but this might not be your desired behavior | -LL | if selection.1.clone().unwrap().contains(selection.0) { - | ++++++++ +LL | if as Clone>::clone(&selection.1).unwrap().contains(selection.0) { + | ++++++++++++++++++++++++++++++++++ + error[E0507]: cannot move out of `selection.1` which is behind a shared reference --> $DIR/option-content-move.rs:27:20 @@ -27,8 +27,8 @@ note: `Result::::unwrap` takes ownership of the receiver `self`, which mov --> $SRC_DIR/core/src/result.rs:LL:COL help: you can `clone` the value and consume it, but this might not be your desired behavior | -LL | if selection.1.clone().unwrap().contains(selection.0) { - | ++++++++ +LL | if as Clone>::clone(&selection.1).unwrap().contains(selection.0) { + | ++++++++++++++++++++++++++++++++++++++++++ + error: aborting due to 2 previous errors From a3f3f51581afab46cc3d70f4bba76afac9f90b7b 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 02/12] 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 e0f2777dd476f..26af9b7cc6bd8 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 0000000000000..4544eeb6e1966 --- /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 e25b4f727856e..81bfdf0fcc769 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 a585bff0c6543..d120adbc03bb3 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 bada08368fc6b..1e98006a9a71e 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 c91a8b5efacfc..0abfcd112ef90 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 0000000000000..a4e219e1c9bf0 --- /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 0000000000000..efe035ebae049 --- /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 0000000000000..0a8fdb72ce8fe --- /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 e2129b4502abe..09a40c25943cf 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 previous error From 294b5143cb78af9548a18fcbe8eb0fe44784fad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 20 Nov 2023 23:13:18 +0000 Subject: [PATCH 03/12] Tweak output on specific case --- .../rustc_borrowck/src/diagnostics/mod.rs | 22 ++++++++++++++++++- tests/ui/moves/move-fn-self-receiver.stderr | 2 +- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs index 26af9b7cc6bd8..af9fc3a9e603b 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mod.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs @@ -11,7 +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_infer::traits::{FulfillmentErrorCode, SelectionError, TraitEngine, TraitEngineExt}; use rustc_middle::mir::tcx::PlaceTy; use rustc_middle::mir::{ AggregateKind, CallSource, ConstOperand, FakeReadCause, Local, LocalInfo, LocalKind, Location, @@ -1211,7 +1211,27 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { .collect::>(); fulfill_cx .register_predicate_obligations(self.infcx, obligations); + // We also register the parent obligation for the type at hand + // implementing `Clone`, to account for bounds that also need + // to be evaluated, like ensuring that `Self: Clone`. + let trait_ref = ty::TraitRef::new(tcx, clone_trait, [ty]); + let obligation = Obligation::new( + tcx, + ObligationCause::dummy(), + self.param_env, + trait_ref, + ); + fulfill_cx + .register_predicate_obligation(self.infcx, obligation); let errors = fulfill_cx.select_all_or_error(self.infcx); + // We remove the last predicate failure, which corresponds to + // the top-level obligation, because most of the type we only + // care about the other ones, *except* when it is the only one. + // This seems to only be relevant for arbitrary self-types. + // Look at `tests/ui/moves/move-fn-self-receiver.rs`. + let errors = match &errors[..] { + errors @ [] | errors @ [_] | [errors @ .., _] => errors, + }; let msg = match &errors[..] { [] => "you can `clone` the value and consume it, but this \ might not be your desired behavior" diff --git a/tests/ui/moves/move-fn-self-receiver.stderr b/tests/ui/moves/move-fn-self-receiver.stderr index 0abfcd112ef90..462deacbe8d6e 100644 --- a/tests/ui/moves/move-fn-self-receiver.stderr +++ b/tests/ui/moves/move-fn-self-receiver.stderr @@ -55,7 +55,7 @@ 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 +help: you could `clone` the value and consume it, if the `Box: Clone` trait bound could be satisfied | LL | boxed_foo.clone().use_box_self(); | ++++++++ From ab2048f0c22527a6c55696ef5fbccb799c9573bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 20 Nov 2023 23:57:59 +0000 Subject: [PATCH 04/12] Mark more tests as `run-rustfix` --- ...ck-move-out-of-overloaded-auto-deref.fixed | 7 ++++ ...rowck-move-out-of-overloaded-auto-deref.rs | 1 + ...k-move-out-of-overloaded-auto-deref.stderr | 2 +- .../ui/suggestions/option-content-move.fixed | 38 +++++++++++++++++++ tests/ui/suggestions/option-content-move.rs | 1 + .../ui/suggestions/option-content-move.stderr | 4 +- 6 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.fixed create mode 100644 tests/ui/suggestions/option-content-move.fixed diff --git a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.fixed b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.fixed new file mode 100644 index 0000000000000..0b7551b97af94 --- /dev/null +++ b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.fixed @@ -0,0 +1,7 @@ +// run-rustfix +use std::rc::Rc; + +pub fn main() { + let _x = as Clone>::clone(&Rc::new(vec![1, 2])).into_iter(); + //~^ ERROR [E0507] +} diff --git a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.rs b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.rs index 0b9e7102cd54f..5cb8ceaca0866 100644 --- a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.rs +++ b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.rs @@ -1,3 +1,4 @@ +// run-rustfix use std::rc::Rc; pub fn main() { diff --git a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr index a86afab3a12db..3375ad298068f 100644 --- a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr +++ b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr @@ -1,5 +1,5 @@ error[E0507]: cannot move out of an `Rc` - --> $DIR/borrowck-move-out-of-overloaded-auto-deref.rs:4:14 + --> $DIR/borrowck-move-out-of-overloaded-auto-deref.rs:5:14 | LL | let _x = Rc::new(vec![1, 2]).into_iter(); | ^^^^^^^^^^^^^^^^^^^ ----------- value moved due to this method call diff --git a/tests/ui/suggestions/option-content-move.fixed b/tests/ui/suggestions/option-content-move.fixed new file mode 100644 index 0000000000000..5e79cf71d823f --- /dev/null +++ b/tests/ui/suggestions/option-content-move.fixed @@ -0,0 +1,38 @@ +// run-rustfix +pub struct LipogramCorpora { + selections: Vec<(char, Option)>, +} + +impl LipogramCorpora { + pub fn validate_all(&mut self) -> Result<(), char> { + for selection in &self.selections { + if selection.1.is_some() { + if as Clone>::clone(&selection.1).unwrap().contains(selection.0) { + //~^ ERROR cannot move out of `selection.1` + return Err(selection.0); + } + } + } + Ok(()) + } +} + +pub struct LipogramCorpora2 { + selections: Vec<(char, Result)>, +} + +impl LipogramCorpora2 { + pub fn validate_all(&mut self) -> Result<(), char> { + for selection in &self.selections { + if selection.1.is_ok() { + if as Clone>::clone(&selection.1).unwrap().contains(selection.0) { + //~^ ERROR cannot move out of `selection.1` + return Err(selection.0); + } + } + } + Ok(()) + } +} + +fn main() {} diff --git a/tests/ui/suggestions/option-content-move.rs b/tests/ui/suggestions/option-content-move.rs index 46c895b95f536..58efbe71f2786 100644 --- a/tests/ui/suggestions/option-content-move.rs +++ b/tests/ui/suggestions/option-content-move.rs @@ -1,3 +1,4 @@ +// run-rustfix pub struct LipogramCorpora { selections: Vec<(char, Option)>, } diff --git a/tests/ui/suggestions/option-content-move.stderr b/tests/ui/suggestions/option-content-move.stderr index d4b73706fcae1..e5de150275dbb 100644 --- a/tests/ui/suggestions/option-content-move.stderr +++ b/tests/ui/suggestions/option-content-move.stderr @@ -1,5 +1,5 @@ error[E0507]: cannot move out of `selection.1` which is behind a shared reference - --> $DIR/option-content-move.rs:9:20 + --> $DIR/option-content-move.rs:10:20 | LL | if selection.1.unwrap().contains(selection.0) { | ^^^^^^^^^^^ -------- `selection.1` moved due to this method call @@ -15,7 +15,7 @@ LL | if as Clone>::clone(&selection.1).unwrap(). | ++++++++++++++++++++++++++++++++++ + error[E0507]: cannot move out of `selection.1` which is behind a shared reference - --> $DIR/option-content-move.rs:27:20 + --> $DIR/option-content-move.rs:28:20 | LL | if selection.1.unwrap().contains(selection.0) { | ^^^^^^^^^^^ -------- `selection.1` moved due to this method call From 6b4c0cebba2587d85c0ab999a1d0fdd9f8c64c2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 21 Nov 2023 01:09:11 +0000 Subject: [PATCH 05/12] On "this .clone() is on the reference", provide more info When encountering a case where `let x: T = (val: &T).clone();` and `T: !Clone`, already mention that the reference is being cloned. We now also suggest `#[derive(Clone)]` not only on `T` but also on type parameters to satisfy blanket implementations. ``` error[E0308]: mismatched types --> $DIR/assignment-of-clone-call-on-ref-due-to-missing-bound.rs:17:39 | LL | let mut x: HashSet = v.clone(); | ------------ ^^^^^^^^^ expected `HashSet`, found `&HashSet` | | | expected due to this | = note: expected struct `HashSet` found reference `&HashSet` note: `HashSet` does not implement `Clone`, so `&HashSet` was cloned instead --> $DIR/assignment-of-clone-call-on-ref-due-to-missing-bound.rs:17:39 | LL | let mut x: HashSet = v.clone(); | ^ = help: `Clone` is not implemented because the trait bound `Day: Clone` is not satisfied help: consider annotating `Day` with `#[derive(Clone)]` | LL + #[derive(Clone)] LL | enum Day { | ``` Case taken from # #41825. --- .../src/fn_ctxt/suggestions.rs | 75 ++++++++++++++++++- ...one-call-on-ref-due-to-missing-bound.fixed | 30 ++++++++ ...-clone-call-on-ref-due-to-missing-bound.rs | 29 +++++++ ...ne-call-on-ref-due-to-missing-bound.stderr | 25 +++++++ 4 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.fixed create mode 100644 tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.rs create mode 100644 tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.stderr diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index a37a595e4a8da..4e863db5637b2 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -21,7 +21,7 @@ use rustc_hir::{ StmtKind, TyKind, WherePredicate, }; use rustc_hir_analysis::astconv::AstConv; -use rustc_infer::traits::{self, StatementAsExpression}; +use rustc_infer::traits::{self, StatementAsExpression, TraitEngineExt}; use rustc_middle::lint::in_external_macro; use rustc_middle::middle::stability::EvalResult; use rustc_middle::ty::print::with_no_trimmed_paths; @@ -34,6 +34,7 @@ use rustc_span::source_map::Spanned; use rustc_span::symbol::{sym, Ident}; use rustc_span::{Span, Symbol}; use rustc_trait_selection::infer::InferCtxtExt; +use rustc_trait_selection::solve::FulfillmentCtxt; use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt; use rustc_trait_selection::traits::error_reporting::DefIdOrName; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _; @@ -1603,6 +1604,78 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { None, ); } else { + self.infcx.probe(|_snapshot| { + if let ty::Adt(def, args) = expected_ty.kind() + && let Some((def_id, _imp)) = self + .tcx + .all_impls(clone_trait_did) + .filter_map(|def_id| { + self.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 = self + .tcx + .predicates_of(def_id) + .instantiate(self.tcx, args) + .into_iter() + .map(|(clause, span)| { + traits::Obligation::new( + self.tcx, + traits::ObligationCause::misc(span, self.body_id), + self.param_env, + clause, + ) + }) + .collect::>(); + fulfill_cx.register_predicate_obligations(&self.infcx, obligations); + let errors = fulfill_cx.select_all_or_error(&self.infcx); + match &errors[..] { + [] => {} + [error] => { + diag.help(format!( + "`Clone` is not implemented because the trait bound `{}` is \ + not satisfied", + error.obligation.predicate, + )); + } + [errors @ .., last] => { + diag.help(format!( + "`Clone` is not implemented because the following trait bounds \ + could not be satisfied: {} and `{}`", + errors + .iter() + .map(|e| format!("`{}`", e.obligation.predicate)) + .collect::>() + .join(", "), + last.obligation.predicate, + )); + } + } + for error in errors { + if let traits::FulfillmentErrorCode::CodeSelectionError( + traits::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, + diag, + error.obligation.predicate.kind().rebind(pred), + ); + } + } + } + }); self.suggest_derive(diag, &[(trait_ref.to_predicate(self.tcx), None, None)]); } } diff --git a/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.fixed b/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.fixed new file mode 100644 index 0000000000000..e88ca6079ece3 --- /dev/null +++ b/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.fixed @@ -0,0 +1,30 @@ +// run-rustfix +#![allow(unused_variables, dead_code)] +use std::collections::BTreeMap; +use std::collections::HashSet; + +#[derive(Debug,Eq,PartialEq,Hash)] +#[derive(Clone)] +enum Day { + Mon, +} + +struct Class { + days: BTreeMap> +} + +impl Class { + fn do_stuff(&self) { + for (_, v) in &self.days { + let mut x: HashSet = v.clone(); //~ ERROR + let y: Vec = x.drain().collect(); + println!("{:?}", x); + } + } +} + +fn fail() { + let c = Class { days: BTreeMap::new() }; + c.do_stuff(); +} +fn main() {} diff --git a/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.rs b/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.rs new file mode 100644 index 0000000000000..ba277c4a9c490 --- /dev/null +++ b/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.rs @@ -0,0 +1,29 @@ +// run-rustfix +#![allow(unused_variables, dead_code)] +use std::collections::BTreeMap; +use std::collections::HashSet; + +#[derive(Debug,Eq,PartialEq,Hash)] +enum Day { + Mon, +} + +struct Class { + days: BTreeMap> +} + +impl Class { + fn do_stuff(&self) { + for (_, v) in &self.days { + let mut x: HashSet = v.clone(); //~ ERROR + let y: Vec = x.drain().collect(); + println!("{:?}", x); + } + } +} + +fn fail() { + let c = Class { days: BTreeMap::new() }; + c.do_stuff(); +} +fn main() {} diff --git a/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.stderr b/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.stderr new file mode 100644 index 0000000000000..3ca4bffd6240e --- /dev/null +++ b/tests/ui/moves/assignment-of-clone-call-on-ref-due-to-missing-bound.stderr @@ -0,0 +1,25 @@ +error[E0308]: mismatched types + --> $DIR/assignment-of-clone-call-on-ref-due-to-missing-bound.rs:18:39 + | +LL | let mut x: HashSet = v.clone(); + | ------------ ^^^^^^^^^ expected `HashSet`, found `&HashSet` + | | + | expected due to this + | + = note: expected struct `HashSet` + found reference `&HashSet` +note: `HashSet` does not implement `Clone`, so `&HashSet` was cloned instead + --> $DIR/assignment-of-clone-call-on-ref-due-to-missing-bound.rs:18:39 + | +LL | let mut x: HashSet = v.clone(); + | ^ + = help: `Clone` is not implemented because the trait bound `Day: Clone` is not satisfied +help: consider annotating `Day` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | enum Day { + | + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0308`. From 05e6067b21364d68a53cb860cdb1cca09f7682a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 21 Nov 2023 21:24:51 +0000 Subject: [PATCH 06/12] Deduplicate some logic --- .../rustc_borrowck/src/diagnostics/mod.rs | 161 ++++++------------ .../src/fn_ctxt/suggestions.rs | 109 +++++------- compiler/rustc_trait_selection/src/infer.rs | 72 ++++++++ 3 files changed, 161 insertions(+), 181 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs index af9fc3a9e603b..891ea4cd5dbd2 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mod.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs @@ -11,7 +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, TraitEngine, TraitEngineExt}; +use rustc_infer::traits::{FulfillmentErrorCode, SelectionError}; use rustc_middle::mir::tcx::PlaceTy; use rustc_middle::mir::{ AggregateKind, CallSource, ConstOperand, FakeReadCause, Local, LocalInfo, LocalKind, Location, @@ -25,11 +25,9 @@ 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::solve::FulfillmentCtxt; +use rustc_trait_selection::infer::InferCtxtExt; use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt as _; -use rustc_trait_selection::traits::{ - type_known_to_meet_bound_modulo_regions, Obligation, ObligationCause, -}; +use rustc_trait_selection::traits::type_known_to_meet_bound_modulo_regions; use super::borrow_set::BorrowData; use super::MirBorrowckCtxt; @@ -1175,113 +1173,56 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { } else { vec![(move_span.shrink_to_hi(), ".clone()".to_string())] }; - 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); - // We also register the parent obligation for the type at hand - // implementing `Clone`, to account for bounds that also need - // to be evaluated, like ensuring that `Self: Clone`. - let trait_ref = ty::TraitRef::new(tcx, clone_trait, [ty]); - let obligation = Obligation::new( - tcx, - ObligationCause::dummy(), - self.param_env, - trait_ref, - ); - fulfill_cx - .register_predicate_obligation(self.infcx, obligation); - let errors = fulfill_cx.select_all_or_error(self.infcx); - // We remove the last predicate failure, which corresponds to - // the top-level obligation, because most of the type we only - // care about the other ones, *except* when it is the only one. - // This seems to only be relevant for arbitrary self-types. - // Look at `tests/ui/moves/move-fn-self-receiver.rs`. - let errors = match &errors[..] { - errors @ [] | errors @ [_] | [errors @ .., _] => errors, - }; - 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), - ); - } + if let Some(errors) = + self.infcx.could_impl_trait(clone_trait, ty, self.param_env) + && !has_sugg + { + 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), + ); } } - }); + } } } } diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index 4e863db5637b2..d68af226007fb 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -21,7 +21,7 @@ use rustc_hir::{ StmtKind, TyKind, WherePredicate, }; use rustc_hir_analysis::astconv::AstConv; -use rustc_infer::traits::{self, StatementAsExpression, TraitEngineExt}; +use rustc_infer::traits::{self, StatementAsExpression}; use rustc_middle::lint::in_external_macro; use rustc_middle::middle::stability::EvalResult; use rustc_middle::ty::print::with_no_trimmed_paths; @@ -34,7 +34,6 @@ use rustc_span::source_map::Spanned; use rustc_span::symbol::{sym, Ident}; use rustc_span::{Span, Symbol}; use rustc_trait_selection::infer::InferCtxtExt; -use rustc_trait_selection::solve::FulfillmentCtxt; use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt; use rustc_trait_selection::traits::error_reporting::DefIdOrName; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _; @@ -1604,78 +1603,46 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { None, ); } else { - self.infcx.probe(|_snapshot| { - if let ty::Adt(def, args) = expected_ty.kind() - && let Some((def_id, _imp)) = self - .tcx - .all_impls(clone_trait_did) - .filter_map(|def_id| { - self.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 = self - .tcx - .predicates_of(def_id) - .instantiate(self.tcx, args) - .into_iter() - .map(|(clause, span)| { - traits::Obligation::new( - self.tcx, - traits::ObligationCause::misc(span, self.body_id), - self.param_env, - clause, - ) - }) - .collect::>(); - fulfill_cx.register_predicate_obligations(&self.infcx, obligations); - let errors = fulfill_cx.select_all_or_error(&self.infcx); - match &errors[..] { - [] => {} - [error] => { - diag.help(format!( - "`Clone` is not implemented because the trait bound `{}` is \ - not satisfied", - error.obligation.predicate, - )); - } - [errors @ .., last] => { - diag.help(format!( - "`Clone` is not implemented because the following trait bounds \ - could not be satisfied: {} and `{}`", - errors - .iter() - .map(|e| format!("`{}`", e.obligation.predicate)) - .collect::>() - .join(", "), - last.obligation.predicate, - )); - } + if let Some(errors) = + self.could_impl_trait(clone_trait_did, expected_ty, self.param_env) + { + match &errors[..] { + [] => {} + [error] => { + diag.help(format!( + "`Clone` is not implemented because the trait bound `{}` is \ + not satisfied", + error.obligation.predicate, + )); } - for error in errors { - if let traits::FulfillmentErrorCode::CodeSelectionError( - traits::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, - diag, - error.obligation.predicate.kind().rebind(pred), - ); - } + [errors @ .., last] => { + diag.help(format!( + "`Clone` is not implemented because the following trait bounds \ + could not be satisfied: {} and `{}`", + errors + .iter() + .map(|e| format!("`{}`", e.obligation.predicate)) + .collect::>() + .join(", "), + last.obligation.predicate, + )); } } - }); + for error in errors { + if let traits::FulfillmentErrorCode::CodeSelectionError( + traits::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, + diag, + error.obligation.predicate.kind().rebind(pred), + ); + } + } + } self.suggest_derive(diag, &[(trait_ref.to_predicate(self.tcx), None, None)]); } } diff --git a/compiler/rustc_trait_selection/src/infer.rs b/compiler/rustc_trait_selection/src/infer.rs index 38153cccfdded..992bfd97e0e44 100644 --- a/compiler/rustc_trait_selection/src/infer.rs +++ b/compiler/rustc_trait_selection/src/infer.rs @@ -1,8 +1,10 @@ +use crate::solve::FulfillmentCtxt; use crate::traits::query::evaluate_obligation::InferCtxtExt as _; use crate::traits::{self, DefiningAnchor, ObligationCtxt}; use rustc_hir::def_id::DefId; use rustc_hir::lang_items::LangItem; +use rustc_infer::traits::{TraitEngine, TraitEngineExt}; use rustc_middle::arena::ArenaAllocatable; use rustc_middle::infer::canonical::{Canonical, CanonicalQueryResponse, QueryResponse}; use rustc_middle::traits::query::NoSolution; @@ -35,6 +37,13 @@ pub trait InferCtxtExt<'tcx> { params: impl IntoIterator>>, param_env: ty::ParamEnv<'tcx>, ) -> traits::EvaluationResult; + + fn could_impl_trait( + &self, + trait_def_id: DefId, + ty: Ty<'tcx>, + param_env: ty::ParamEnv<'tcx>, + ) -> Option>>; } impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> { @@ -76,6 +85,69 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> { }; self.evaluate_obligation(&obligation).unwrap_or(traits::EvaluationResult::EvaluatedToErr) } + + fn could_impl_trait( + &self, + trait_def_id: DefId, + ty: Ty<'tcx>, + param_env: ty::ParamEnv<'tcx>, + ) -> Option>> { + self.probe(|_snapshot| { + if let ty::Adt(def, args) = ty.kind() + && let Some((impl_def_id, _)) = self + .tcx + .all_impls(trait_def_id) + .filter_map(|impl_def_id| { + self.tcx.impl_trait_ref(impl_def_id).map(|r| (impl_def_id, r)) + }) + .map(|(impl_def_id, imp)| (impl_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); + // We get all obligations from the impl to talk about specific + // trait bounds. + let obligations = self + .tcx + .predicates_of(impl_def_id) + .instantiate(self.tcx, args) + .into_iter() + .map(|(clause, span)| { + traits::Obligation::new( + self.tcx, + traits::ObligationCause::dummy_with_span(span), + param_env, + clause, + ) + }) + .collect::>(); + fulfill_cx.register_predicate_obligations(self, obligations); + let trait_ref = ty::TraitRef::new(self.tcx, trait_def_id, [ty]); + let obligation = traits::Obligation::new( + self.tcx, + traits::ObligationCause::dummy(), + param_env, + trait_ref, + ); + fulfill_cx.register_predicate_obligation(self, obligation); + let mut errors = fulfill_cx.select_all_or_error(self); + // We remove the last predicate failure, which corresponds to + // the top-level obligation, because most of the type we only + // care about the other ones, *except* when it is the only one. + // This seems to only be relevant for arbitrary self-types. + // Look at `tests/ui/moves/move-fn-self-receiver.rs`. + if errors.len() > 1 { + errors.truncate(errors.len() - 1); + } + Some(errors) + } else { + None + } + }) + } } pub trait InferCtxtBuilderExt<'tcx> { From d42d3fd15f323797edc1251f858438c3f7668c8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 21 Nov 2023 23:41:50 +0000 Subject: [PATCH 07/12] Provide more suggestions for cloning immutable bindings When encountering multiple mutable borrows, suggest cloning and adding derive annotations as needed. ``` error[E0596]: cannot borrow `sm.x` as mutable, as it is behind a `&` reference --> $DIR/accidentally-cloning-ref-borrow-error.rs:32:9 | LL | foo(&mut sm.x); | ^^^^^^^^^ `sm` is a `&` reference, so the data it refers to cannot be borrowed as mutable | help: `Str` doesn't implement `Clone`, so this call clones the reference `&Str` --> $DIR/accidentally-cloning-ref-borrow-error.rs:31:21 | LL | let mut sm = sr.clone(); | ^^^^^^^ help: consider annotating `Str` with `#[derive(Clone)]` | LL + #[derive(Clone)] LL | struct Str { | help: consider specifying this binding's type | LL | let mut sm: &mut Str = sr.clone(); | ++++++++++ ``` ``` error[E0596]: cannot borrow `*inner` as mutable, as it is behind a `&` reference --> $DIR/issue-91206.rs:14:5 | LL | inner.clear(); | ^^^^^ `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable | help: you can `clone` the `Vec` value and consume it, but this might not be your desired behavior --> $DIR/issue-91206.rs:11:17 | LL | let inner = client.get_inner_ref(); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider specifying this binding's type | LL | let inner: &mut Vec = client.get_inner_ref(); | +++++++++++++++++ ``` --- .../src/diagnostics/mutability_errors.rs | 102 +++++++++++++++++- .../accidentally-cloning-ref-borrow-error.rs | 38 +++++++ ...cidentally-cloning-ref-borrow-error.stderr | 30 ++++++ tests/ui/borrowck/issue-85765-closure.rs | 1 + tests/ui/borrowck/issue-85765-closure.stderr | 13 ++- tests/ui/borrowck/issue-85765.rs | 1 + tests/ui/borrowck/issue-85765.stderr | 13 ++- tests/ui/borrowck/issue-91206.rs | 1 + tests/ui/borrowck/issue-91206.stderr | 7 +- 9 files changed, 196 insertions(+), 10 deletions(-) create mode 100644 tests/ui/borrowck/accidentally-cloning-ref-borrow-error.rs create mode 100644 tests/ui/borrowck/accidentally-cloning-ref-borrow-error.stderr diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index dde46eef6a0d3..129b3341f5f75 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -3,8 +3,9 @@ use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed use rustc_hir as hir; use rustc_hir::intravisit::Visitor; use rustc_hir::Node; +use rustc_infer::traits; use rustc_middle::mir::{Mutability, Place, PlaceRef, ProjectionElem}; -use rustc_middle::ty::{self, InstanceDef, Ty, TyCtxt}; +use rustc_middle::ty::{self, InstanceDef, ToPredicate, Ty, TyCtxt}; use rustc_middle::{ hir::place::PlaceBase, mir::{self, BindingForm, Local, LocalDecl, LocalInfo, LocalKind, Location}, @@ -12,6 +13,8 @@ use rustc_middle::{ use rustc_span::symbol::{kw, Symbol}; use rustc_span::{sym, BytePos, DesugaringKind, Span}; use rustc_target::abi::FieldIdx; +use rustc_trait_selection::infer::InferCtxtExt; +use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt; use crate::diagnostics::BorrowedContentSource; use crate::util::FindAssignments; @@ -1213,6 +1216,103 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> { if let Some(hir_id) = hir_id && let Some(hir::Node::Local(local)) = hir_map.find(hir_id) { + let tables = self.infcx.tcx.typeck(def_id.as_local().unwrap()); + if let Some(clone_trait) = self.infcx.tcx.lang_items().clone_trait() + && let Some(expr) = local.init + && let ty = tables.node_type_opt(expr.hir_id) + && let Some(ty) = ty + && let ty::Ref(..) = ty.kind() + { + match self + .infcx + .could_impl_trait(clone_trait, ty.peel_refs(), self.param_env) + .as_deref() + { + Some([]) => { + // The type implements Clone. + err.span_help( + expr.span, + format!( + "you can `clone` the `{}` value and consume it, but this \ + might not be your desired behavior", + ty.peel_refs(), + ), + ); + } + None => { + if let hir::ExprKind::MethodCall(segment, _rcvr, [], span) = + expr.kind + && segment.ident.name == sym::clone + { + err.span_help( + span, + format!( + "`{}` doesn't implement `Clone`, so this call clones \ + the reference `{ty}`", + ty.peel_refs(), + ), + ); + } + // The type doesn't implement Clone. + let trait_ref = ty::Binder::dummy(ty::TraitRef::new( + self.infcx.tcx, + clone_trait, + [ty.peel_refs()], + )); + let obligation = traits::Obligation::new( + self.infcx.tcx, + traits::ObligationCause::dummy(), + self.param_env, + trait_ref, + ); + self.infcx.err_ctxt().suggest_derive( + &obligation, + err, + trait_ref.to_predicate(self.infcx.tcx), + ); + } + Some(errors) => { + if let hir::ExprKind::MethodCall(segment, _rcvr, [], span) = + expr.kind + && segment.ident.name == sym::clone + { + err.span_help( + span, + format!( + "`{}` doesn't implement `Clone` because its \ + implementations trait bounds could not be met, so \ + this call clones the reference `{ty}`", + ty.peel_refs(), + ), + ); + err.note(format!( + "the following trait bounds weren't met: {}", + errors + .iter() + .map(|e| e.obligation.predicate.to_string()) + .collect::>() + .join("\n"), + )); + } + // The type doesn't implement Clone because of unmet obligations. + for error in errors { + if let traits::FulfillmentErrorCode::CodeSelectionError( + traits::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 (changing, span, sugg) = match local.ty { Some(ty) => ("changing", ty.span, message), None => { diff --git a/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.rs b/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.rs new file mode 100644 index 0000000000000..2b25a5b2348a4 --- /dev/null +++ b/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.rs @@ -0,0 +1,38 @@ +#[derive(Debug)] +struct X(T); + +impl Clone for X { + fn clone(&self) -> X { + X(self.0.clone()) + } +} + +#[derive(Debug)] +struct Y; + +#[derive(Debug)] +struct Str { + x: Option, +} + +fn foo(s: &mut Option) { + if s.is_none() { + *s = Some(0); + } + println!("{:?}", s); +} + +fn bar(s: &mut X) { + println!("{:?}", s); +} +fn main() { + let s = Str { x: None }; + let sr = &s; + let mut sm = sr.clone(); + foo(&mut sm.x); //~ ERROR cannot borrow `sm.x` as mutable, as it is behind a `&` reference + + let x = X(Y); + let xr = &x; + let mut xm = xr.clone(); + bar(&mut xm); //~ ERROR cannot borrow data in a `&` reference as mutable +} diff --git a/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.stderr b/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.stderr new file mode 100644 index 0000000000000..7e51a4819eef6 --- /dev/null +++ b/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.stderr @@ -0,0 +1,30 @@ +error[E0596]: cannot borrow `sm.x` as mutable, as it is behind a `&` reference + --> $DIR/accidentally-cloning-ref-borrow-error.rs:32:9 + | +LL | foo(&mut sm.x); + | ^^^^^^^^^ `sm` is a `&` reference, so the data it refers to cannot be borrowed as mutable + | +help: `Str` doesn't implement `Clone`, so this call clones the reference `&Str` + --> $DIR/accidentally-cloning-ref-borrow-error.rs:31:21 + | +LL | let mut sm = sr.clone(); + | ^^^^^^^ +help: consider annotating `Str` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | struct Str { + | +help: consider specifying this binding's type + | +LL | let mut sm: &mut Str = sr.clone(); + | ++++++++++ + +error[E0596]: cannot borrow data in a `&` reference as mutable + --> $DIR/accidentally-cloning-ref-borrow-error.rs:37:9 + | +LL | bar(&mut xm); + | ^^^^^^^ cannot borrow as mutable + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0596`. diff --git a/tests/ui/borrowck/issue-85765-closure.rs b/tests/ui/borrowck/issue-85765-closure.rs index f2d1dd0fbc3fb..edc9eeaffb5ac 100644 --- a/tests/ui/borrowck/issue-85765-closure.rs +++ b/tests/ui/borrowck/issue-85765-closure.rs @@ -3,6 +3,7 @@ fn main() { let mut test = Vec::new(); let rofl: &Vec> = &mut test; //~^ HELP consider changing this binding's type + //~| HELP you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior rofl.push(Vec::new()); //~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference //~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-85765-closure.stderr b/tests/ui/borrowck/issue-85765-closure.stderr index 936ddd67bcd81..4a6a0e94becd9 100644 --- a/tests/ui/borrowck/issue-85765-closure.stderr +++ b/tests/ui/borrowck/issue-85765-closure.stderr @@ -1,16 +1,21 @@ error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference - --> $DIR/issue-85765-closure.rs:6:9 + --> $DIR/issue-85765-closure.rs:7:9 | LL | rofl.push(Vec::new()); | ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable | +help: you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior + --> $DIR/issue-85765-closure.rs:4:36 + | +LL | let rofl: &Vec> = &mut test; + | ^^^^^^^^^ help: consider changing this binding's type | LL | let rofl: &mut Vec> = &mut test; | ~~~~~~~~~~~~~~~~~~ error[E0594]: cannot assign to `*r`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:13:9 + --> $DIR/issue-85765-closure.rs:14:9 | LL | *r = 0; | ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written @@ -21,7 +26,7 @@ LL | let r = &mut mutvar; | +++ error[E0594]: cannot assign to `*x`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:20:9 + --> $DIR/issue-85765-closure.rs:21:9 | LL | *x = 1; | ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written @@ -32,7 +37,7 @@ LL | let x: &mut usize = &mut{0}; | ~~~~~~~~~~ error[E0594]: cannot assign to `*y`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:27:9 + --> $DIR/issue-85765-closure.rs:28:9 | LL | *y = 1; | ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written diff --git a/tests/ui/borrowck/issue-85765.rs b/tests/ui/borrowck/issue-85765.rs index 76e0b51735416..ce5740bc0e7c1 100644 --- a/tests/ui/borrowck/issue-85765.rs +++ b/tests/ui/borrowck/issue-85765.rs @@ -2,6 +2,7 @@ fn main() { let mut test = Vec::new(); let rofl: &Vec> = &mut test; //~^ HELP consider changing this binding's type + //~| HELP you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior rofl.push(Vec::new()); //~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference //~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-85765.stderr b/tests/ui/borrowck/issue-85765.stderr index 57900bfb612e1..4889f774afa8e 100644 --- a/tests/ui/borrowck/issue-85765.stderr +++ b/tests/ui/borrowck/issue-85765.stderr @@ -1,16 +1,21 @@ error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference - --> $DIR/issue-85765.rs:5:5 + --> $DIR/issue-85765.rs:6:5 | LL | rofl.push(Vec::new()); | ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable | +help: you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior + --> $DIR/issue-85765.rs:3:32 + | +LL | let rofl: &Vec> = &mut test; + | ^^^^^^^^^ help: consider changing this binding's type | LL | let rofl: &mut Vec> = &mut test; | ~~~~~~~~~~~~~~~~~~ error[E0594]: cannot assign to `*r`, which is behind a `&` reference - --> $DIR/issue-85765.rs:12:5 + --> $DIR/issue-85765.rs:13:5 | LL | *r = 0; | ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written @@ -21,7 +26,7 @@ LL | let r = &mut mutvar; | +++ error[E0594]: cannot assign to `*x`, which is behind a `&` reference - --> $DIR/issue-85765.rs:19:5 + --> $DIR/issue-85765.rs:20:5 | LL | *x = 1; | ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written @@ -32,7 +37,7 @@ LL | let x: &mut usize = &mut{0}; | ~~~~~~~~~~ error[E0594]: cannot assign to `*y`, which is behind a `&` reference - --> $DIR/issue-85765.rs:26:5 + --> $DIR/issue-85765.rs:27:5 | LL | *y = 1; | ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written diff --git a/tests/ui/borrowck/issue-91206.rs b/tests/ui/borrowck/issue-91206.rs index e062a253767de..c60ac62fa3477 100644 --- a/tests/ui/borrowck/issue-91206.rs +++ b/tests/ui/borrowck/issue-91206.rs @@ -10,6 +10,7 @@ fn main() { let client = TestClient; let inner = client.get_inner_ref(); //~^ HELP consider specifying this binding's type + //~| HELP you can `clone` the `Vec` value and consume it, but this might not be your desired behavior inner.clear(); //~^ ERROR cannot borrow `*inner` as mutable, as it is behind a `&` reference [E0596] //~| NOTE `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-91206.stderr b/tests/ui/borrowck/issue-91206.stderr index 30f83656518dc..cfeb1b294b027 100644 --- a/tests/ui/borrowck/issue-91206.stderr +++ b/tests/ui/borrowck/issue-91206.stderr @@ -1,9 +1,14 @@ error[E0596]: cannot borrow `*inner` as mutable, as it is behind a `&` reference - --> $DIR/issue-91206.rs:13:5 + --> $DIR/issue-91206.rs:14:5 | LL | inner.clear(); | ^^^^^ `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable | +help: you can `clone` the `Vec` value and consume it, but this might not be your desired behavior + --> $DIR/issue-91206.rs:11:17 + | +LL | let inner = client.get_inner_ref(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider specifying this binding's type | LL | let inner: &mut Vec = client.get_inner_ref(); From 6da2b7dd8f9e32fe94969ebcf8f8745c2b2108aa Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Fri, 1 Dec 2023 11:04:34 -0700 Subject: [PATCH 08/12] rustdoc: do not escape quotes in body text Escaping quote marks is only needed in attributes, not text. ```console $ du -hs doc-old/ doc-new/ 670M doc-old/ 669M doc-new/ ``` --- src/librustdoc/html/escape.rs | 36 +++++++++++++++++++ src/librustdoc/html/highlight.rs | 12 +++++-- .../html/highlight/fixtures/dos_line.html | 2 +- .../html/highlight/fixtures/sample.html | 8 ++--- 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/librustdoc/html/escape.rs b/src/librustdoc/html/escape.rs index 4a19d0a44c365..ea4b573aeb955 100644 --- a/src/librustdoc/html/escape.rs +++ b/src/librustdoc/html/escape.rs @@ -38,3 +38,39 @@ impl<'a> fmt::Display for Escape<'a> { Ok(()) } } + +/// Wrapper struct which will emit the HTML-escaped version of the contained +/// string when passed to a format string. +/// +/// This is only safe to use for text nodes. If you need your output to be +/// safely contained in an attribute, use [`Escape`]. If you don't know the +/// difference, use [`Escape`]. +pub(crate) struct EscapeBodyText<'a>(pub &'a str); + +impl<'a> fmt::Display for EscapeBodyText<'a> { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + // Because the internet is always right, turns out there's not that many + // characters to escape: http://stackoverflow.com/questions/7381974 + let EscapeBodyText(s) = *self; + let pile_o_bits = s; + let mut last = 0; + for (i, ch) in s.char_indices() { + let s = match ch { + '>' => ">", + '<' => "<", + '&' => "&", + _ => continue, + }; + fmt.write_str(&pile_o_bits[last..i])?; + fmt.write_str(s)?; + // NOTE: we only expect single byte characters here - which is fine as long as we + // only match single byte characters + last = i + 1; + } + + if last < s.len() { + fmt.write_str(&pile_o_bits[last..])?; + } + Ok(()) + } +} diff --git a/src/librustdoc/html/highlight.rs b/src/librustdoc/html/highlight.rs index b762c8a1ce61c..1cdc792a819a8 100644 --- a/src/librustdoc/html/highlight.rs +++ b/src/librustdoc/html/highlight.rs @@ -6,7 +6,7 @@ //! Use the `render_with_highlighting` to highlight some rust code. use crate::clean::PrimitiveType; -use crate::html::escape::Escape; +use crate::html::escape::EscapeBodyText; use crate::html::render::{Context, LinkFromSrc}; use std::collections::VecDeque; @@ -189,7 +189,7 @@ impl<'a, 'tcx, F: Write> TokenHandler<'a, 'tcx, F> { && can_merge(current_class, Some(*parent_class), "") { for (text, class) in self.pending_elems.iter() { - string(self.out, Escape(text), *class, &self.href_context, false); + string(self.out, EscapeBodyText(text), *class, &self.href_context, false); } } else { // We only want to "open" the tag ourselves if we have more than one pending and if the @@ -202,7 +202,13 @@ impl<'a, 'tcx, F: Write> TokenHandler<'a, 'tcx, F> { None }; for (text, class) in self.pending_elems.iter() { - string(self.out, Escape(text), *class, &self.href_context, close_tag.is_none()); + string( + self.out, + EscapeBodyText(text), + *class, + &self.href_context, + close_tag.is_none(), + ); } if let Some(close_tag) = close_tag { exit_span(self.out, close_tag); diff --git a/src/librustdoc/html/highlight/fixtures/dos_line.html b/src/librustdoc/html/highlight/fixtures/dos_line.html index 30b50ca7c662c..b98e6712590c2 100644 --- a/src/librustdoc/html/highlight/fixtures/dos_line.html +++ b/src/librustdoc/html/highlight/fixtures/dos_line.html @@ -1,3 +1,3 @@ pub fn foo() { -println!("foo"); +println!("foo"); } diff --git a/src/librustdoc/html/highlight/fixtures/sample.html b/src/librustdoc/html/highlight/fixtures/sample.html index fced2eacd9e72..aa735e81597c7 100644 --- a/src/librustdoc/html/highlight/fixtures/sample.html +++ b/src/librustdoc/html/highlight/fixtures/sample.html @@ -8,12 +8,12 @@ .lifetime { color: #B76514; } .question-mark { color: #ff9011; } -
#![crate_type = "lib"]
+
#![crate_type = "lib"]
 
 use std::path::{Path, PathBuf};
 
-#[cfg(target_os = "linux")]
-#[cfg(target_os = "windows")]
+#[cfg(target_os = "linux")]
+#[cfg(target_os = "windows")]
 fn main() -> () {
     let foo = true && false || true;
     let _: *const () = 0;
@@ -22,7 +22,7 @@
     let _ = *foo;
     mac!(foo, &mut bar);
     assert!(self.length < N && index <= self.length);
-    ::std::env::var("gateau").is_ok();
+    ::std::env::var("gateau").is_ok();
     #[rustfmt::skip]
     let s:std::path::PathBuf = std::path::PathBuf::new();
     let mut s = String::new();

From edabd477f86c48929f12b4c2f5a15d58cde120a5 Mon Sep 17 00:00:00 2001
From: Ralf Jung 
Date: Sun, 3 Dec 2023 11:51:58 +0100
Subject: [PATCH 09/12] interpret: make numeric_intrinsic accessible from Miri

---
 .../src/interpret/intrinsics.rs               | 82 ++++++++-----------
 1 file changed, 36 insertions(+), 46 deletions(-)

diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs
index 80e14f5a884d7..c29f23b913f68 100644
--- a/compiler/rustc_const_eval/src/interpret/intrinsics.rs
+++ b/compiler/rustc_const_eval/src/interpret/intrinsics.rs
@@ -3,17 +3,22 @@
 //! and miri.
 
 use rustc_hir::def_id::DefId;
-use rustc_middle::mir::{
-    self,
-    interpret::{Allocation, ConstAllocation, GlobalId, InterpResult, PointerArithmetic, Scalar},
-    BinOp, ConstValue, NonDivergingIntrinsic,
-};
 use rustc_middle::ty;
 use rustc_middle::ty::layout::{LayoutOf as _, ValidityRequirement};
 use rustc_middle::ty::GenericArgsRef;
 use rustc_middle::ty::{Ty, TyCtxt};
+use rustc_middle::{
+    mir::{
+        self,
+        interpret::{
+            Allocation, ConstAllocation, GlobalId, InterpResult, PointerArithmetic, Scalar,
+        },
+        BinOp, ConstValue, NonDivergingIntrinsic,
+    },
+    ty::layout::TyAndLayout,
+};
 use rustc_span::symbol::{sym, Symbol};
-use rustc_target::abi::{Abi, Primitive, Size};
+use rustc_target::abi::Size;
 
 use super::{
     util::ensure_monomorphic_enough, CheckInAllocMsg, ImmTy, InterpCx, Machine, OpTy, PlaceTy,
@@ -22,23 +27,6 @@ use super::{
 
 use crate::fluent_generated as fluent;
 
-fn numeric_intrinsic(name: Symbol, bits: u128, kind: Primitive) -> Scalar {
-    let size = match kind {
-        Primitive::Int(integer, _) => integer.size(),
-        _ => bug!("invalid `{}` argument: {:?}", name, bits),
-    };
-    let extra = 128 - u128::from(size.bits());
-    let bits_out = match name {
-        sym::ctpop => u128::from(bits.count_ones()),
-        sym::ctlz => u128::from(bits.leading_zeros()) - extra,
-        sym::cttz => u128::from((bits << extra).trailing_zeros()) - extra,
-        sym::bswap => (bits << extra).swap_bytes(),
-        sym::bitreverse => (bits << extra).reverse_bits(),
-        _ => bug!("not a numeric intrinsic: {}", name),
-    };
-    Scalar::from_uint(bits_out, size)
-}
-
 /// Directly returns an `Allocation` containing an absolute path representation of the given type.
 pub(crate) fn alloc_type_name<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> ConstAllocation<'tcx> {
     let path = crate::util::type_name(tcx, ty);
@@ -179,30 +167,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             | sym::bswap
             | sym::bitreverse => {
                 let ty = instance_args.type_at(0);
-                let layout_of = self.layout_of(ty)?;
+                let layout = self.layout_of(ty)?;
                 let val = self.read_scalar(&args[0])?;
-                let bits = val.to_bits(layout_of.size)?;
-                let kind = match layout_of.abi {
-                    Abi::Scalar(scalar) => scalar.primitive(),
-                    _ => span_bug!(
-                        self.cur_span(),
-                        "{} called on invalid type {:?}",
-                        intrinsic_name,
-                        ty
-                    ),
-                };
-                let (nonzero, actual_intrinsic_name) = match intrinsic_name {
-                    sym::cttz_nonzero => (true, sym::cttz),
-                    sym::ctlz_nonzero => (true, sym::ctlz),
-                    other => (false, other),
-                };
-                if nonzero && bits == 0 {
-                    throw_ub_custom!(
-                        fluent::const_eval_call_nonzero_intrinsic,
-                        name = intrinsic_name,
-                    );
-                }
-                let out_val = numeric_intrinsic(actual_intrinsic_name, bits, kind);
+                let out_val = self.numeric_intrinsic(intrinsic_name, val, layout)?;
                 self.write_scalar(out_val, dest)?;
             }
             sym::saturating_add | sym::saturating_sub => {
@@ -493,6 +460,29 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         }
     }
 
+    pub fn numeric_intrinsic(
+        &self,
+        name: Symbol,
+        val: Scalar,
+        layout: TyAndLayout<'tcx>,
+    ) -> InterpResult<'tcx, Scalar> {
+        assert!(layout.ty.is_integral(), "invalid type for numeric intrinsic: {}", layout.ty);
+        let bits = val.to_bits(layout.size)?;
+        let extra = 128 - u128::from(layout.size.bits());
+        let bits_out = match name {
+            sym::ctpop => u128::from(bits.count_ones()),
+            sym::ctlz_nonzero | sym::cttz_nonzero if bits == 0 => {
+                throw_ub_custom!(fluent::const_eval_call_nonzero_intrinsic, name = name,);
+            }
+            sym::ctlz | sym::ctlz_nonzero => u128::from(bits.leading_zeros()) - extra,
+            sym::cttz | sym::cttz_nonzero => u128::from((bits << extra).trailing_zeros()) - extra,
+            sym::bswap => (bits << extra).swap_bytes(),
+            sym::bitreverse => (bits << extra).reverse_bits(),
+            _ => bug!("not a numeric intrinsic: {}", name),
+        };
+        Ok(Scalar::from_uint(bits_out, layout.size))
+    }
+
     pub fn exact_div(
         &mut self,
         a: &ImmTy<'tcx, M::Provenance>,

From bcfeaabaf3f942401c23e3edefa9de798ab2dc2f Mon Sep 17 00:00:00 2001
From: Ralf Jung 
Date: Mon, 4 Dec 2023 08:16:03 +0100
Subject: [PATCH 10/12] portable-simd: add missing feature gate

---
 library/portable-simd/crates/core_simd/tests/pointers.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/library/portable-simd/crates/core_simd/tests/pointers.rs b/library/portable-simd/crates/core_simd/tests/pointers.rs
index a90ff928cedcb..b9f32d16e01d1 100644
--- a/library/portable-simd/crates/core_simd/tests/pointers.rs
+++ b/library/portable-simd/crates/core_simd/tests/pointers.rs
@@ -1,4 +1,4 @@
-#![feature(portable_simd, strict_provenance)]
+#![feature(portable_simd, strict_provenance, exposed_provenance)]
 
 use core_simd::simd::{
     ptr::{SimdConstPtr, SimdMutPtr},

From 1c556bbed4a02a914c152c96c18c04835338ba83 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez 
Date: Mon, 4 Dec 2023 12:12:13 +0100
Subject: [PATCH 11/12] Don't generate the "Fields" heading if there is no
 field displayed

---
 src/librustdoc/html/render/print_item.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/librustdoc/html/render/print_item.rs b/src/librustdoc/html/render/print_item.rs
index 131b1d608e683..ff7ce01e807c4 100644
--- a/src/librustdoc/html/render/print_item.rs
+++ b/src/librustdoc/html/render/print_item.rs
@@ -1737,7 +1737,14 @@ fn item_variants(
         w.write_str("");
 
         let heading_and_fields = match &variant_data.kind {
-            clean::VariantKind::Struct(s) => Some(("Fields", &s.fields)),
+            clean::VariantKind::Struct(s) => {
+                // If there is no field to display, no need to add the heading.
+                if s.fields.iter().any(|f| !f.is_doc_hidden()) {
+                    Some(("Fields", &s.fields))
+                } else {
+                    None
+                }
+            }
             clean::VariantKind::Tuple(fields) => {
                 // Documentation on tuple variant fields is rare, so to reduce noise we only emit
                 // the section if at least one field is documented.

From 8e53edb2ece6cc6ab6715fef5b3332ae8ffee8a2 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez 
Date: Mon, 4 Dec 2023 12:13:24 +0100
Subject: [PATCH 12/12] Add regression test for #118195

---
 tests/rustdoc/enum-variant-fields-heading.rs   | 18 ++++++++++++++++++
 .../enum-variant-fields-heading.variants.html  |  3 +++
 2 files changed, 21 insertions(+)
 create mode 100644 tests/rustdoc/enum-variant-fields-heading.rs
 create mode 100644 tests/rustdoc/enum-variant-fields-heading.variants.html

diff --git a/tests/rustdoc/enum-variant-fields-heading.rs b/tests/rustdoc/enum-variant-fields-heading.rs
new file mode 100644
index 0000000000000..8a7c99a87359e
--- /dev/null
+++ b/tests/rustdoc/enum-variant-fields-heading.rs
@@ -0,0 +1,18 @@
+// This is a regression test for .
+// It ensures that the "Fields" heading is not generated if no field is displayed.
+
+#![crate_name = "foo"]
+
+// @has 'foo/enum.Foo.html'
+// @has - '//*[@id="variant.A"]' 'A'
+// @count - '//*[@id="variant.A.fields"]' 0
+// @has - '//*[@id="variant.B"]' 'B'
+// @count - '//*[@id="variant.B.fields"]' 0
+// @snapshot variants - '//*[@id="main-content"]/*[@class="variants"]'
+
+pub enum Foo {
+    /// A variant with no fields
+    A {},
+    /// A variant with hidden fields
+    B { #[doc(hidden)] a: u8 },
+}
diff --git a/tests/rustdoc/enum-variant-fields-heading.variants.html b/tests/rustdoc/enum-variant-fields-heading.variants.html
new file mode 100644
index 0000000000000..bcb36f7cf86f6
--- /dev/null
+++ b/tests/rustdoc/enum-variant-fields-heading.variants.html
@@ -0,0 +1,3 @@
+
§
A
A variant with no fields

+
§
B
A variant with hidden fields

+

\ No newline at end of file