From e7a777805a63266292bb715e283db4107fb6e394 Mon Sep 17 00:00:00 2001 From: yukang Date: Tue, 3 Jan 2023 13:27:49 +0800 Subject: [PATCH 1/5] Suggest possible clone when we have &T --- compiler/rustc_hir_typeck/src/demand.rs | 1 + .../src/fn_ctxt/suggestions.rs | 29 ++++++++ .../src/traits/error_reporting/mod.rs | 5 ++ .../src/traits/error_reporting/suggestions.rs | 72 ++++++++++++++++++- .../issue-106443-sugg-clone-for-arg.rs | 23 ++++++ .../issue-106443-sugg-clone-for-arg.stderr | 35 +++++++++ .../issue-106443-sugg-clone-for-bound.rs | 20 ++++++ .../issue-106443-sugg-clone-for-bound.stderr | 29 ++++++++ 8 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/suggestions/issue-106443-sugg-clone-for-arg.rs create mode 100644 src/test/ui/suggestions/issue-106443-sugg-clone-for-arg.stderr create mode 100644 src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.rs create mode 100644 src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.stderr diff --git a/compiler/rustc_hir_typeck/src/demand.rs b/compiler/rustc_hir_typeck/src/demand.rs index 0f191c21a0a63..52778747987d3 100644 --- a/compiler/rustc_hir_typeck/src/demand.rs +++ b/compiler/rustc_hir_typeck/src/demand.rs @@ -57,6 +57,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { || self.suggest_boxing_when_appropriate(err, expr, expected, expr_ty) || self.suggest_block_to_brackets_peeling_refs(err, expr, expr_ty, expected) || self.suggest_copied_or_cloned(err, expr, expr_ty, expected) + || self.suggest_clone_for_ref(err, expr, expr_ty, expected) || self.suggest_into(err, expr, expr_ty, expected) || self.suggest_floating_point_literal(err, expr, expected); if !suggested { diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index 066e98c74578f..991b0399542bf 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -1014,6 +1014,35 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } + pub(crate) fn suggest_clone_for_ref( + &self, + diag: &mut Diagnostic, + expr: &hir::Expr<'_>, + expr_ty: Ty<'tcx>, + expected_ty: Ty<'tcx>, + ) -> bool { + if let ty::Ref(_, inner_ty, hir::Mutability::Not) = expr_ty.kind() && + let Some(clone_trait_def) = self.tcx.lang_items().clone_trait() && + expected_ty == *inner_ty && + self + .infcx + .type_implements_trait( + clone_trait_def, + [self.tcx.erase_regions(expected_ty)], + self.param_env + ) + .must_apply_modulo_regions() { + diag.span_suggestion_verbose( + expr.span.shrink_to_hi(), + "consider using clone here", + ".clone()", + Applicability::MachineApplicable, + ); + return true; + } + false + } + pub(crate) fn suggest_copied_or_cloned( &self, diag: &mut Diagnostic, diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs index 17331a5010552..9c098e1a2fc12 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs @@ -873,6 +873,11 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> { ); } + if self.suggest_add_clone_to_arg(&obligation, &mut err, trait_predicate) { + err.emit(); + return; + } + if self.suggest_impl_trait(&mut err, span, &obligation, trait_predicate) { err.emit(); return; diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index 28f76b141469d..d15196a9577aa 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -10,7 +10,7 @@ use crate::infer::InferCtxt; use crate::traits::{NormalizeExt, ObligationCtxt}; use hir::def::CtorOf; -use hir::HirId; +use hir::{Expr, HirId}; use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::stack::ensure_sufficient_stack; use rustc_errors::{ @@ -206,11 +206,15 @@ pub trait TypeErrCtxtExt<'tcx> { trait_pred: ty::PolyTraitPredicate<'tcx>, ); - fn suggest_add_reference_to_arg( + fn suggest_add_clone_to_arg( &self, obligation: &PredicateObligation<'tcx>, err: &mut Diagnostic, trait_pred: ty::PolyTraitPredicate<'tcx>, + ); + + fn suggest_add_reference_to_arg( + err: &mut Diagnostic, has_custom_message: bool, ) -> bool; @@ -1102,6 +1106,70 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> { } } + fn suggest_add_clone_to_arg( + &self, + obligation: &PredicateObligation<'tcx>, + err: &mut Diagnostic, + trait_pred: ty::PolyTraitPredicate<'tcx>, + ) -> bool { + let span = obligation.cause.span; + let body_id = obligation.cause.body_id; + let self_ty = self.resolve_vars_if_possible(trait_pred.self_ty()); + let ty = self.tcx.erase_late_bound_regions(self_ty); + let owner = self.tcx.hir().get_parent_item(body_id); + if let ObligationCauseCode::FunctionArgumentObligation { arg_hir_id, .. } = obligation.cause.code() && + let arg_node = self.tcx.hir().get(*arg_hir_id) && + let Node::Expr(Expr { kind: hir::ExprKind::Path(_), ..}) = arg_node && + let Some(generics) = self.tcx.hir().get_generics(owner.def_id) && + let ty::Ref(_, inner_ty, hir::Mutability::Not) = ty.kind() && + let ty::Param(param) = inner_ty.kind() && + let Some(generic_param) = + generics.params.iter().find(|p| p.name.ident().as_str() == param.name.as_str()) + { + let clone_trait = self.tcx.require_lang_item(LangItem::Clone, None); + let has_clone = self + .type_implements_trait(clone_trait, [ty], obligation.param_env) + .must_apply_modulo_regions(); + + let trait_pred_and_suggested_ty = + trait_pred.map_bound(|trait_pred| (trait_pred, *inner_ty)); + let new_obligation = self.mk_trait_obligation_with_new_self_ty( + obligation.param_env, + trait_pred_and_suggested_ty, + ); + + if has_clone && self.predicate_may_hold(&new_obligation) { + let clone_bound = generics.bounds_for_param(generic_param.def_id) + .flat_map(|bp| bp.bounds) + .any(|bound| { + if let hir::GenericBound::Trait( hir::PolyTraitRef { trait_ref, ..}, ..) = bound { + Some(clone_trait) == trait_ref.trait_def_id() + } else { + false + } + }); + if !clone_bound { + suggest_constraining_type_param( + self.tcx, + generics, + err, + param.name.as_str(), + "Clone", + Some(clone_trait) + ); + } + err.span_suggestion_verbose( + span.shrink_to_hi(), + "consider using clone here", + ".clone()".to_string(), + Applicability::MaybeIncorrect, + ); + return true; + } + } + false + } + fn suggest_add_reference_to_arg( &self, obligation: &PredicateObligation<'tcx>, diff --git a/src/test/ui/suggestions/issue-106443-sugg-clone-for-arg.rs b/src/test/ui/suggestions/issue-106443-sugg-clone-for-arg.rs new file mode 100644 index 0000000000000..48efdb82c46ca --- /dev/null +++ b/src/test/ui/suggestions/issue-106443-sugg-clone-for-arg.rs @@ -0,0 +1,23 @@ +#[derive(Clone)] +struct S; + +// without Clone +struct T; + +fn foo(_: S) {} + +fn test1() { + let s = &S; + foo(s); //~ ERROR mismatched types +} + +fn bar(_: T) {} +fn test2() { + let t = &T; + bar(t); //~ ERROR mismatched types +} + +fn main() { + test1(); + test2(); +} diff --git a/src/test/ui/suggestions/issue-106443-sugg-clone-for-arg.stderr b/src/test/ui/suggestions/issue-106443-sugg-clone-for-arg.stderr new file mode 100644 index 0000000000000..1e66fe3af2414 --- /dev/null +++ b/src/test/ui/suggestions/issue-106443-sugg-clone-for-arg.stderr @@ -0,0 +1,35 @@ +error[E0308]: mismatched types + --> $DIR/issue-106443-sugg-clone-for-arg.rs:11:9 + | +LL | foo(s); + | --- ^ expected struct `S`, found `&S` + | | + | arguments to this function are incorrect + | +note: function defined here + --> $DIR/issue-106443-sugg-clone-for-arg.rs:7:4 + | +LL | fn foo(_: S) {} + | ^^^ ---- +help: consider using clone here + | +LL | foo(s.clone()); + | ++++++++ + +error[E0308]: mismatched types + --> $DIR/issue-106443-sugg-clone-for-arg.rs:17:9 + | +LL | bar(t); + | --- ^ expected struct `T`, found `&T` + | | + | arguments to this function are incorrect + | +note: function defined here + --> $DIR/issue-106443-sugg-clone-for-arg.rs:14:4 + | +LL | fn bar(_: T) {} + | ^^^ ---- + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0308`. diff --git a/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.rs b/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.rs new file mode 100644 index 0000000000000..4e4d4c044c986 --- /dev/null +++ b/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.rs @@ -0,0 +1,20 @@ +#[derive(Clone)] +struct S; + +trait X {} + +impl X for S {} + +fn foo(_: T) {} +fn bar(s: &T) { + foo(s); //~ ERROR the trait bound `&T: X` is not satisfied +} + +fn bar_with_clone(s: &T) { + foo(s); //~ ERROR the trait bound `&T: X` is not satisfied +} + +fn main() { + let s = &S; + bar(s); +} diff --git a/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.stderr b/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.stderr new file mode 100644 index 0000000000000..22bafc6cc8b90 --- /dev/null +++ b/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.stderr @@ -0,0 +1,29 @@ +error[E0277]: the trait bound `&T: X` is not satisfied + --> $DIR/issue-106443-sugg-clone-for-bound.rs:10:9 + | +LL | foo(s); + | ^ the trait `X` is not implemented for `&T` + | +help: consider further restricting this bound + | +LL | fn bar(s: &T) { + | +++++++ +help: consider using clone here + | +LL | foo(s.clone()); + | ++++++++ + +error[E0277]: the trait bound `&T: X` is not satisfied + --> $DIR/issue-106443-sugg-clone-for-bound.rs:14:9 + | +LL | foo(s); + | ^ the trait `X` is not implemented for `&T` + | +help: consider using clone here + | +LL | foo(s.clone()); + | ++++++++ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0277`. From 1a0a6132a8f4f3d54971c11c74035c5872a72a1f Mon Sep 17 00:00:00 2001 From: Yukang Date: Fri, 6 Jan 2023 15:03:05 +0800 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Esteban Kuber --- .../src/fn_ctxt/suggestions.rs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index 991b0399542bf..236bdc60e677d 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -1021,25 +1021,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { expr_ty: Ty<'tcx>, expected_ty: Ty<'tcx>, ) -> bool { - if let ty::Ref(_, inner_ty, hir::Mutability::Not) = expr_ty.kind() && - let Some(clone_trait_def) = self.tcx.lang_items().clone_trait() && - expected_ty == *inner_ty && - self + if let ty::Ref(_, inner_ty, hir::Mutability::Not) = expr_ty.kind() + && let Some(clone_trait_def) = self.tcx.lang_items().clone_trait() + && expected_ty == *inner_ty + && self .infcx .type_implements_trait( clone_trait_def, [self.tcx.erase_regions(expected_ty)], self.param_env ) - .must_apply_modulo_regions() { - diag.span_suggestion_verbose( - expr.span.shrink_to_hi(), - "consider using clone here", - ".clone()", - Applicability::MachineApplicable, - ); - return true; - } + .must_apply_modulo_regions() + { + diag.span_suggestion_verbose( + expr.span.shrink_to_hi(), + "consider using clone here", + ".clone()", + Applicability::MachineApplicable, + ); + return true; + } false } From ce4afed2efdaaceb2624021df3a8d8fff892415a Mon Sep 17 00:00:00 2001 From: yukang Date: Fri, 6 Jan 2023 07:44:10 +0800 Subject: [PATCH 3/5] comments feedback --- .../src/traits/error_reporting/suggestions.rs | 101 +++++++++--------- 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index d15196a9577aa..727bdc391bb41 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -211,10 +211,13 @@ pub trait TypeErrCtxtExt<'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut Diagnostic, trait_pred: ty::PolyTraitPredicate<'tcx>, - ); + ) -> bool; fn suggest_add_reference_to_arg( + &self, + obligation: &PredicateObligation<'tcx>, err: &mut Diagnostic, + trait_pred: ty::PolyTraitPredicate<'tcx>, has_custom_message: bool, ) -> bool; @@ -1112,60 +1115,58 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> { err: &mut Diagnostic, trait_pred: ty::PolyTraitPredicate<'tcx>, ) -> bool { - let span = obligation.cause.span; - let body_id = obligation.cause.body_id; let self_ty = self.resolve_vars_if_possible(trait_pred.self_ty()); let ty = self.tcx.erase_late_bound_regions(self_ty); - let owner = self.tcx.hir().get_parent_item(body_id); - if let ObligationCauseCode::FunctionArgumentObligation { arg_hir_id, .. } = obligation.cause.code() && - let arg_node = self.tcx.hir().get(*arg_hir_id) && - let Node::Expr(Expr { kind: hir::ExprKind::Path(_), ..}) = arg_node && - let Some(generics) = self.tcx.hir().get_generics(owner.def_id) && - let ty::Ref(_, inner_ty, hir::Mutability::Not) = ty.kind() && - let ty::Param(param) = inner_ty.kind() && - let Some(generic_param) = - generics.params.iter().find(|p| p.name.ident().as_str() == param.name.as_str()) - { - let clone_trait = self.tcx.require_lang_item(LangItem::Clone, None); - let has_clone = self - .type_implements_trait(clone_trait, [ty], obligation.param_env) - .must_apply_modulo_regions(); - - let trait_pred_and_suggested_ty = - trait_pred.map_bound(|trait_pred| (trait_pred, *inner_ty)); - let new_obligation = self.mk_trait_obligation_with_new_self_ty( - obligation.param_env, - trait_pred_and_suggested_ty, - ); + let owner = self.tcx.hir().get_parent_item(obligation.cause.body_id); + let Some(generics) = self.tcx.hir().get_generics(owner.def_id) else { return false }; + let ty::Ref(_, inner_ty, hir::Mutability::Not) = ty.kind() else { return false }; + let ty::Param(param) = inner_ty.kind() else { return false }; + let Some(generic_param) = generics.get_named(param.name) else { return false }; + let ObligationCauseCode::FunctionArgumentObligation { arg_hir_id, .. } = obligation.cause.code() else { return false }; + let arg_node = self.tcx.hir().get(*arg_hir_id); + let Node::Expr(Expr { kind: hir::ExprKind::Path(_), ..}) = arg_node else { return false }; + + let clone_trait = self.tcx.require_lang_item(LangItem::Clone, None); + let has_clone = self + .type_implements_trait(clone_trait, [ty], obligation.param_env) + .must_apply_modulo_regions(); + + let trait_pred_and_suggested_ty = + trait_pred.map_bound(|trait_pred| (trait_pred, *inner_ty)); + let new_obligation = self.mk_trait_obligation_with_new_self_ty( + obligation.param_env, + trait_pred_and_suggested_ty, + ); - if has_clone && self.predicate_may_hold(&new_obligation) { - let clone_bound = generics.bounds_for_param(generic_param.def_id) - .flat_map(|bp| bp.bounds) - .any(|bound| { - if let hir::GenericBound::Trait( hir::PolyTraitRef { trait_ref, ..}, ..) = bound { - Some(clone_trait) == trait_ref.trait_def_id() - } else { - false - } - }); - if !clone_bound { - suggest_constraining_type_param( - self.tcx, - generics, - err, - param.name.as_str(), - "Clone", - Some(clone_trait) - ); - } - err.span_suggestion_verbose( - span.shrink_to_hi(), - "consider using clone here", - ".clone()".to_string(), - Applicability::MaybeIncorrect, + if has_clone && self.predicate_may_hold(&new_obligation) { + let clone_bound = generics + .bounds_for_param(generic_param.def_id) + .flat_map(|bp| bp.bounds) + .any(|bound| { + if let hir::GenericBound::Trait(hir::PolyTraitRef { trait_ref, .. }, ..) = bound + { + Some(clone_trait) == trait_ref.trait_def_id() + } else { + false + } + }); + if !clone_bound { + suggest_constraining_type_param( + self.tcx, + generics, + err, + param.name.as_str(), + "Clone", + Some(clone_trait), ); - return true; } + err.span_suggestion_verbose( + obligation.cause.span.shrink_to_hi(), + "consider using clone here", + ".clone()".to_string(), + Applicability::MaybeIncorrect, + ); + return true; } false } From 6082729646e3530ecdd522921837806db83b27e3 Mon Sep 17 00:00:00 2001 From: yukang Date: Sat, 7 Jan 2023 07:16:48 +0800 Subject: [PATCH 4/5] use type_implements_trait to check Param clone --- .../src/traits/error_reporting/suggestions.rs | 27 +++++-------------- .../issue-106443-sugg-clone-for-bound.rs | 4 +-- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index 727bdc391bb41..c52365ae3b7c7 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -1121,36 +1121,23 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> { let Some(generics) = self.tcx.hir().get_generics(owner.def_id) else { return false }; let ty::Ref(_, inner_ty, hir::Mutability::Not) = ty.kind() else { return false }; let ty::Param(param) = inner_ty.kind() else { return false }; - let Some(generic_param) = generics.get_named(param.name) else { return false }; let ObligationCauseCode::FunctionArgumentObligation { arg_hir_id, .. } = obligation.cause.code() else { return false }; let arg_node = self.tcx.hir().get(*arg_hir_id); let Node::Expr(Expr { kind: hir::ExprKind::Path(_), ..}) = arg_node else { return false }; let clone_trait = self.tcx.require_lang_item(LangItem::Clone, None); - let has_clone = self - .type_implements_trait(clone_trait, [ty], obligation.param_env) - .must_apply_modulo_regions(); + let has_clone = |ty| { + self.type_implements_trait(clone_trait, [ty], obligation.param_env) + .must_apply_modulo_regions() + }; - let trait_pred_and_suggested_ty = - trait_pred.map_bound(|trait_pred| (trait_pred, *inner_ty)); let new_obligation = self.mk_trait_obligation_with_new_self_ty( obligation.param_env, - trait_pred_and_suggested_ty, + trait_pred.map_bound(|trait_pred| (trait_pred, *inner_ty)), ); - if has_clone && self.predicate_may_hold(&new_obligation) { - let clone_bound = generics - .bounds_for_param(generic_param.def_id) - .flat_map(|bp| bp.bounds) - .any(|bound| { - if let hir::GenericBound::Trait(hir::PolyTraitRef { trait_ref, .. }, ..) = bound - { - Some(clone_trait) == trait_ref.trait_def_id() - } else { - false - } - }); - if !clone_bound { + if self.predicate_may_hold(&new_obligation) && has_clone(ty) { + if !has_clone(param.to_ty(self.tcx)) { suggest_constraining_type_param( self.tcx, generics, diff --git a/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.rs b/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.rs index 4e4d4c044c986..3b2e316b2961e 100644 --- a/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.rs +++ b/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.rs @@ -6,11 +6,11 @@ trait X {} impl X for S {} fn foo(_: T) {} -fn bar(s: &T) { +fn bar(s: &T) { foo(s); //~ ERROR the trait bound `&T: X` is not satisfied } -fn bar_with_clone(s: &T) { +fn bar_with_clone(s: &T) { foo(s); //~ ERROR the trait bound `&T: X` is not satisfied } From 0e570e58f2cf465f254b7e6c56584ec732a77dbc Mon Sep 17 00:00:00 2001 From: Yukang Date: Sun, 8 Jan 2023 22:51:42 +0800 Subject: [PATCH 5/5] Remove extra space --- .../ui/suggestions/issue-106443-sugg-clone-for-bound.stderr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.stderr b/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.stderr index 22bafc6cc8b90..8607917ede6bf 100644 --- a/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.stderr +++ b/src/test/ui/suggestions/issue-106443-sugg-clone-for-bound.stderr @@ -6,7 +6,7 @@ LL | foo(s); | help: consider further restricting this bound | -LL | fn bar(s: &T) { +LL | fn bar(s: &T) { | +++++++ help: consider using clone here |