From f69946ac6497495315ff56052d6f41d5532216b9 Mon Sep 17 00:00:00 2001 From: yuk1ty Date: Sat, 21 Mar 2026 23:12:46 +0900 Subject: [PATCH] Avoid redundant clone suggestions in borrowck diagnostics --- .../src/diagnostics/conflict_errors.rs | 4 +- .../rustc_borrowck/src/diagnostics/mod.rs | 121 ++++++++++++------ .../src/diagnostics/move_errors.rs | 62 +++++---- ...ck-move-out-of-overloaded-auto-deref.fixed | 2 +- ...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 - tests/ui/moves/suggest-clone.fixed | 2 +- tests/ui/moves/suggest-clone.stderr | 4 - .../ui/suggestions/option-content-move.fixed | 4 +- .../ui/suggestions/option-content-move.stderr | 8 -- 13 files changed, 129 insertions(+), 94 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 2735c800c4a3..639cb75fdd28 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -254,7 +254,9 @@ pub(crate) fn report_use_of_moved_or_uninitialized( maybe_reinitialized_locations_is_empty: maybe_reinitialized_locations .is_empty(), }; - self.explain_captures( + // The return value indicates whether a clone suggestion was emitted; + // redundancy is already handled via the `FnSelfUse` check below. + let _ = self.explain_captures( &mut err, span, move_span, diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs index a9697950e38b..3ee0369c9da6 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mod.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs @@ -1002,6 +1002,12 @@ struct CapturedMessageOpt { maybe_reinitialized_locations_is_empty: bool, } +/// Tracks which suggestions were emitted by [`MirBorrowckCtxt::explain_captures`], +/// so callers can avoid emitting redundant suggestions downstream. +struct ExplainCapturesResult { + clone_suggestion: bool, +} + impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { /// Finds the spans associated to a move or copy of move_place at location. pub(super) fn move_spans( @@ -1226,7 +1232,7 @@ fn explain_captures( move_spans: UseSpans<'tcx>, moved_place: Place<'tcx>, msg_opt: CapturedMessageOpt, - ) { + ) -> ExplainCapturesResult { let CapturedMessageOpt { is_partial_move: is_partial, is_loop_message, @@ -1235,6 +1241,7 @@ fn explain_captures( has_suggest_reborrow, maybe_reinitialized_locations_is_empty, } = msg_opt; + let mut suggested_cloning = false; if let UseSpans::FnSelfUse { var_span, fn_call_span, fn_span, kind } = move_spans { let place_name = self .describe_place(moved_place.as_ref()) @@ -1461,10 +1468,26 @@ fn explain_captures( has_sugg = true; } if let Some(clone_trait) = tcx.lang_items().clone_trait() { - let sugg = if moved_place + // Check whether the deref is from a custom Deref impl + // (e.g. Rc, Box) or a built-in reference deref. + // For built-in derefs with Clone fully satisfied, we skip + // the UFCS suggestion here and let `suggest_cloning` + // downstream emit a simpler `.clone()` suggestion instead. + let has_overloaded_deref = + moved_place.iter_projections().any(|(place, elem)| { + matches!(elem, ProjectionElem::Deref) + && matches!( + self.borrowed_content_source(place), + BorrowedContentSource::OverloadedDeref(_) + | BorrowedContentSource::OverloadedIndex(_) + ) + }); + + let has_deref = moved_place .iter_projections() - .any(|(_, elem)| matches!(elem, ProjectionElem::Deref)) - { + .any(|(_, elem)| matches!(elem, ProjectionElem::Deref)); + + let sugg = if has_deref { let (start, end) = if let Some(expr) = self.find_expr(move_span) && let Some(_) = self.clone_on_reference(expr) && let hir::ExprKind::MethodCall(_, rcvr, _, _) = expr.kind @@ -1490,43 +1513,58 @@ fn explain_captures( self.infcx.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, - ) - } - _ => { - format!( - "you could `clone` the value and consume it, if the \ - following trait bounds could be satisfied: {}", - listify(&errors, |e: &FulfillmentError<'tcx>| format!( - "`{}`", - e.obligation.predicate - )) - .unwrap(), - ) - } - }; - err.multipart_suggestion(msg, sugg, Applicability::MaybeIncorrect); - for error in errors { - if let FulfillmentErrorCode::Select( - 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 skip_for_simple_clone = + has_deref && !has_overloaded_deref && errors.is_empty(); + if !skip_for_simple_clone { + 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, + ) + } + _ => { + format!( + "you could `clone` the value and consume it, if \ + the following trait bounds could be satisfied: \ + {}", + listify( + &errors, + |e: &FulfillmentError<'tcx>| format!( + "`{}`", + e.obligation.predicate + ) + ) + .unwrap(), + ) + } + }; + err.multipart_suggestion( + msg, + sugg, + Applicability::MaybeIncorrect, + ); + + suggested_cloning = errors.is_empty(); + + for error in errors { + if let FulfillmentErrorCode::Select( + 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), + ); + } } } } @@ -1558,6 +1596,7 @@ fn explain_captures( }) } } + ExplainCapturesResult { clone_suggestion: suggested_cloning } } /// Skip over locals that begin with an underscore or have no name diff --git a/compiler/rustc_borrowck/src/diagnostics/move_errors.rs b/compiler/rustc_borrowck/src/diagnostics/move_errors.rs index 0d0147e5eb51..1dfd83956e49 100644 --- a/compiler/rustc_borrowck/src/diagnostics/move_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/move_errors.rs @@ -269,14 +269,19 @@ fn report(&mut self, error: GroupedMoveError<'tcx>) { .span_delayed_bug(span, "Type may implement copy, but there is no other error."); return; } + + let mut has_clone_suggestion = false; let mut err = match kind { - &IllegalMoveOriginKind::BorrowedContent { target_place } => self - .report_cannot_move_from_borrowed_content( + &IllegalMoveOriginKind::BorrowedContent { target_place } => { + let (diag, clone_sugg) = self.report_cannot_move_from_borrowed_content( original_path, target_place, span, use_spans, - ), + ); + has_clone_suggestion = clone_sugg; + diag + } &IllegalMoveOriginKind::InteriorOfTypeWithDestructor { container_ty: ty } => { self.cannot_move_out_of_interior_of_drop(span, ty) } @@ -285,7 +290,7 @@ fn report(&mut self, error: GroupedMoveError<'tcx>) { } }; - self.add_move_hints(error, &mut err, span); + self.add_move_hints(error, &mut err, span, has_clone_suggestion); self.buffer_error(err); } @@ -426,7 +431,7 @@ fn report_cannot_move_from_borrowed_content( deref_target_place: Place<'tcx>, span: Span, use_spans: Option>, - ) -> Diag<'infcx> { + ) -> (Diag<'infcx>, bool) { let tcx = self.infcx.tcx; // Inspect the type of the content behind the // borrow to provide feedback about why this @@ -447,8 +452,8 @@ fn report_cannot_move_from_borrowed_content( let decl = &self.body.local_decls[local]; let local_name = self.local_name(local).map(|sym| format!("`{sym}`")); if decl.is_ref_for_guard() { - return self - .cannot_move_out_of( + return ( + self.cannot_move_out_of( span, &format!( "{} in pattern guard", @@ -458,9 +463,11 @@ fn report_cannot_move_from_borrowed_content( .with_note( "variables bound in patterns cannot be moved from \ until after the end of the pattern guard", - ); + ), + false, + ); } else if decl.is_ref_to_static() { - return self.report_cannot_move_from_static(move_place, span); + return (self.report_cannot_move_from_static(move_place, span), false); } } @@ -539,10 +546,13 @@ fn report_cannot_move_from_borrowed_content( has_suggest_reborrow: false, maybe_reinitialized_locations_is_empty: true, }; - if let Some(use_spans) = use_spans { - self.explain_captures(&mut err, span, span, use_spans, move_place, msg_opt); - } - err + let suggested_cloning = if let Some(use_spans) = use_spans { + self.explain_captures(&mut err, span, span, use_spans, move_place, msg_opt) + .clone_suggestion + } else { + false + }; + (err, suggested_cloning) } fn report_closure_move_error( @@ -676,7 +686,13 @@ fn closure_clause_kind( } } - fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span: Span) { + fn add_move_hints( + &self, + error: GroupedMoveError<'tcx>, + err: &mut Diag<'_>, + span: Span, + has_clone_suggestion: bool, + ) { match error { GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => { self.add_borrow_suggestions(err, span, !binds_to.is_empty()); @@ -719,14 +735,16 @@ fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span None => "value".to_string(), }; - if let Some(expr) = self.find_expr(use_span) { - self.suggest_cloning( - err, - original_path.as_ref(), - place_ty, - expr, - Some(use_spans), - ); + if !has_clone_suggestion { + if let Some(expr) = self.find_expr(use_span) { + self.suggest_cloning( + err, + original_path.as_ref(), + place_ty, + expr, + Some(use_spans), + ); + } } if let Some(upvar_field) = self 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 index a19db7e5cd32..8d5ebbc77440 100644 --- a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.fixed +++ b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.fixed @@ -2,6 +2,6 @@ use std::rc::Rc; pub fn main() { - let _x = as Clone>::clone(&Rc::new(vec![1, 2]).clone()).into_iter(); + 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.stderr b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr index 577c2de38be0..076f0ce3440a 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 @@ -12,10 +12,6 @@ help: you can `clone` the value and consume it, but this might not be your desir | LL | let _x = as Clone>::clone(&Rc::new(vec![1, 2])).into_iter(); | ++++++++++++++++++++++++++++ + -help: consider cloning the value if the performance cost is acceptable - | -LL | let _x = Rc::new(vec![1, 2]).clone().into_iter(); - | ++++++++ error: aborting due to 1 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 59a162e72c17..86ce083da65e 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; - ::clone(&foo.clone()).foo(); //~ ERROR cannot move out + foo.clone().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 c2c63f949436..4cf1de52a4a4 100644 --- a/tests/ui/borrowck/clone-span-on-try-operator.stderr +++ b/tests/ui/borrowck/clone-span-on-try-operator.stderr @@ -11,10 +11,6 @@ note: `Foo::foo` takes ownership of the receiver `self`, which moves `*foo` | LL | fn foo(self) {} | ^^^^ -help: you can `clone` the value and consume it, but this might not be your desired behavior - | -LL | ::clone(&(*foo)).foo(); - | +++++++++++++++++++++++ + help: consider cloning the value if the performance cost is acceptable | LL - (*foo).foo(); 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 3b4f7c8465cf..c7ddef0f7bc7 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(|| { - as Clone>::clone(&y.clone()).into_iter(); + y.clone().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 69c366749163..08cf52883983 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 @@ -17,10 +17,6 @@ LL | fn call(f: F) where F : Fn() { | ^^^^ 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 | as Clone>::clone(&y).into_iter(); - | +++++++++++++++++++++++++++++++ + help: consider cloning the value if the performance cost is acceptable | LL | y.clone().into_iter(); diff --git a/tests/ui/moves/suggest-clone.fixed b/tests/ui/moves/suggest-clone.fixed index 59a162e72c17..86ce083da65e 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; - ::clone(&foo.clone()).foo(); //~ ERROR cannot move out + foo.clone().foo(); //~ ERROR cannot move out } diff --git a/tests/ui/moves/suggest-clone.stderr b/tests/ui/moves/suggest-clone.stderr index f8e0ccdfceff..ec18d2abdec0 100644 --- a/tests/ui/moves/suggest-clone.stderr +++ b/tests/ui/moves/suggest-clone.stderr @@ -11,10 +11,6 @@ note: `Foo::foo` takes ownership of the receiver `self`, which moves `*foo` | LL | fn foo(self) {} | ^^^^ -help: you can `clone` the value and consume it, but this might not be your desired behavior - | -LL | ::clone(&foo).foo(); - | +++++++++++++++++++++++ + help: consider cloning the value if the performance cost is acceptable | LL | foo.clone().foo(); diff --git a/tests/ui/suggestions/option-content-move.fixed b/tests/ui/suggestions/option-content-move.fixed index 0720146280f9..f36c36d7d645 100644 --- a/tests/ui/suggestions/option-content-move.fixed +++ b/tests/ui/suggestions/option-content-move.fixed @@ -7,7 +7,7 @@ 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.clone()).as_mut().as_ref().unwrap().contains(selection.0) { + if selection.1.clone().as_mut().as_ref().unwrap().contains(selection.0) { //~^ ERROR cannot move out of `selection.1` return Err(selection.0); } @@ -25,7 +25,7 @@ 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.clone()).as_mut().as_ref().unwrap().contains(selection.0) { + if selection.1.clone().as_mut().as_ref().unwrap().contains(selection.0) { //~^ ERROR cannot move out of `selection.1` return Err(selection.0); } diff --git a/tests/ui/suggestions/option-content-move.stderr b/tests/ui/suggestions/option-content-move.stderr index 09c6f39fd803..b514622699e7 100644 --- a/tests/ui/suggestions/option-content-move.stderr +++ b/tests/ui/suggestions/option-content-move.stderr @@ -16,10 +16,6 @@ help: consider calling `.as_mut()` to mutably borrow the value's contents | LL | if selection.1.as_mut().unwrap().contains(selection.0) { | +++++++++ -help: you can `clone` the value and consume it, but this might not be your desired behavior - | -LL | if as Clone>::clone(&selection.1).unwrap().contains(selection.0) { - | ++++++++++++++++++++++++++++++++++ + help: consider cloning the value if the performance cost is acceptable | LL | if selection.1.clone().unwrap().contains(selection.0) { @@ -43,10 +39,6 @@ help: consider calling `.as_mut()` to mutably borrow the value's contents | LL | if selection.1.as_mut().unwrap().contains(selection.0) { | +++++++++ -help: you can `clone` the value and consume it, but this might not be your desired behavior - | -LL | if as Clone>::clone(&selection.1).unwrap().contains(selection.0) { - | ++++++++++++++++++++++++++++++++++++++++++ + help: consider cloning the value if the performance cost is acceptable | LL | if selection.1.clone().unwrap().contains(selection.0) {