From 526d794d807ca0e54d3411fed4c508b1c6f0f0e5 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Wed, 20 Aug 2025 01:34:05 +0200 Subject: [PATCH 1/4] create let-chain --- clippy_lints/src/non_canonical_impls.rs | 222 +++++++++++------------- 1 file changed, 106 insertions(+), 116 deletions(-) diff --git a/clippy_lints/src/non_canonical_impls.rs b/clippy_lints/src/non_canonical_impls.rs index ba67dc62abbd..c479f40707e9 100644 --- a/clippy_lints/src/non_canonical_impls.rs +++ b/clippy_lints/src/non_canonical_impls.rs @@ -114,136 +114,126 @@ declare_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL impl LateLintPass<'_> for NonCanonicalImpls { #[expect(clippy::too_many_lines)] fn check_impl_item<'tcx>(&mut self, cx: &LateContext<'tcx>, impl_item: &ImplItem<'tcx>) { - let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id()) else { - return; - }; - let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else { - return; - }; - if cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) { - return; - } - let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir_impl_item(impl_item.impl_item_id()).kind else { - return; - }; - let body = cx.tcx.hir_body(impl_item_id); - let ExprKind::Block(block, ..) = body.value.kind else { - return; - }; - if block.span.in_external_macro(cx.sess().source_map()) || is_from_proc_macro(cx, impl_item) { - return; - } - - let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id); - if trait_name == Some(sym::Clone) - && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) - && implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[]) + if let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id()) + && let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) + && !cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) + && let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir_impl_item(impl_item.impl_item_id()).kind + && let body = cx.tcx.hir_body(impl_item_id) + && let ExprKind::Block(block, ..) = body.value.kind + && !block.span.in_external_macro(cx.sess().source_map()) + && !is_from_proc_macro(cx, impl_item) { - if impl_item.ident.name == sym::clone { - if block.stmts.is_empty() - && let Some(expr) = block.expr - && let ExprKind::Unary(UnOp::Deref, deref) = expr.kind - && let ExprKind::Path(qpath) = deref.kind - && last_path_segment(&qpath).ident.name == kw::SelfLower - { - } else { + let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id); + if trait_name == Some(sym::Clone) + && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) + && implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[]) + { + if impl_item.ident.name == sym::clone { + if block.stmts.is_empty() + && let Some(expr) = block.expr + && let ExprKind::Unary(UnOp::Deref, deref) = expr.kind + && let ExprKind::Path(qpath) = deref.kind + && last_path_segment(&qpath).ident.name == kw::SelfLower + { + } else { + span_lint_and_sugg( + cx, + NON_CANONICAL_CLONE_IMPL, + block.span, + "non-canonical implementation of `clone` on a `Copy` type", + "change this to", + "{ *self }".to_owned(), + Applicability::MaybeIncorrect, + ); + + return; + } + } + + if impl_item.ident.name == sym::clone_from { span_lint_and_sugg( cx, NON_CANONICAL_CLONE_IMPL, - block.span, - "non-canonical implementation of `clone` on a `Copy` type", - "change this to", - "{ *self }".to_owned(), + impl_item.span, + "unnecessary implementation of `clone_from` on a `Copy` type", + "remove it", + String::new(), Applicability::MaybeIncorrect, ); + } + } else if trait_name == Some(sym::PartialOrd) + && impl_item.ident.name == sym::partial_cmp + && let Some(ord_def_id) = cx.tcx.get_diagnostic_item(sym::Ord) + && implements_trait(cx, trait_impl.self_ty(), ord_def_id, &[]) + { + // If the `cmp` call likely needs to be fully qualified in the suggestion + // (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't + // access `cmp_expr` in the suggestion without major changes, as we lint in `else`. + let mut needs_fully_qualified = false; + if block.stmts.is_empty() + && let Some(expr) = block.expr + && expr_is_cmp(cx, expr, impl_item, &mut needs_fully_qualified) + { + return; + } + // Fix #12683, allow [`needless_return`] here + else if block.expr.is_none() + && let Some(stmt) = block.stmts.first() + && let rustc_hir::StmtKind::Semi(Expr { + kind: ExprKind::Ret(Some(ret)), + .. + }) = stmt.kind + && expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified) + { + return; + } + // If `Self` and `Rhs` are not the same type, bail. This makes creating a valid + // suggestion tons more complex. + else if let [lhs, rhs, ..] = trait_impl.args.as_slice() + && lhs != rhs + { return; } - } - if impl_item.ident.name == sym::clone_from { - span_lint_and_sugg( + span_lint_and_then( cx, - NON_CANONICAL_CLONE_IMPL, - impl_item.span, - "unnecessary implementation of `clone_from` on a `Copy` type", - "remove it", - String::new(), - Applicability::MaybeIncorrect, - ); - } - } else if trait_name == Some(sym::PartialOrd) - && impl_item.ident.name == sym::partial_cmp - && let Some(ord_def_id) = cx.tcx.get_diagnostic_item(sym::Ord) - && implements_trait(cx, trait_impl.self_ty(), ord_def_id, &[]) - { - // If the `cmp` call likely needs to be fully qualified in the suggestion - // (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't - // access `cmp_expr` in the suggestion without major changes, as we lint in `else`. - let mut needs_fully_qualified = false; - - if block.stmts.is_empty() - && let Some(expr) = block.expr - && expr_is_cmp(cx, expr, impl_item, &mut needs_fully_qualified) - { - return; - } - // Fix #12683, allow [`needless_return`] here - else if block.expr.is_none() - && let Some(stmt) = block.stmts.first() - && let rustc_hir::StmtKind::Semi(Expr { - kind: ExprKind::Ret(Some(ret)), - .. - }) = stmt.kind - && expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified) - { - return; - } - // If `Self` and `Rhs` are not the same type, bail. This makes creating a valid - // suggestion tons more complex. - else if let [lhs, rhs, ..] = trait_impl.args.as_slice() - && lhs != rhs - { - return; - } - - span_lint_and_then( - cx, - NON_CANONICAL_PARTIAL_ORD_IMPL, - item.span, - "non-canonical implementation of `partial_cmp` on an `Ord` type", - |diag| { - let [_, other] = body.params else { - return; - }; - let Some(std_or_core) = std_or_core(cx) else { - return; - }; + NON_CANONICAL_PARTIAL_ORD_IMPL, + item.span, + "non-canonical implementation of `partial_cmp` on an `Ord` type", + |diag| { + let [_, other] = body.params else { + return; + }; + let Some(std_or_core) = std_or_core(cx) else { + return; + }; - let suggs = match (other.pat.simple_ident(), needs_fully_qualified) { - (Some(other_ident), true) => vec![( - block.span, - format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name), - )], - (Some(other_ident), false) => { - vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))] - }, - (None, true) => vec![ - ( + let suggs = match (other.pat.simple_ident(), needs_fully_qualified) { + (Some(other_ident), true) => vec![( block.span, - format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"), - ), - (other.pat.span, "other".to_owned()), - ], - (None, false) => vec![ - (block.span, "{ Some(self.cmp(other)) }".to_owned()), - (other.pat.span, "other".to_owned()), - ], - }; + format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name), + )], + (Some(other_ident), false) => { + vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))] + }, + (None, true) => vec![ + ( + block.span, + format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"), + ), + (other.pat.span, "other".to_owned()), + ], + (None, false) => vec![ + (block.span, "{ Some(self.cmp(other)) }".to_owned()), + (other.pat.span, "other".to_owned()), + ], + }; - diag.multipart_suggestion("change this to", suggs, Applicability::Unspecified); - }, - ); + diag.multipart_suggestion("change this to", suggs, Applicability::Unspecified); + }, + ); + } } } } From 54e28471d707c86476f7519571eea74c0016d3c8 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Wed, 20 Aug 2025 01:31:18 +0200 Subject: [PATCH 2/4] extract helper functions too many lines no more! --- clippy_lints/src/non_canonical_impls.rs | 214 +++++++++++++----------- 1 file changed, 113 insertions(+), 101 deletions(-) diff --git a/clippy_lints/src/non_canonical_impls.rs b/clippy_lints/src/non_canonical_impls.rs index c479f40707e9..5f2c388ff678 100644 --- a/clippy_lints/src/non_canonical_impls.rs +++ b/clippy_lints/src/non_canonical_impls.rs @@ -5,9 +5,9 @@ use clippy_utils::{ }; use rustc_errors::Applicability; use rustc_hir::def_id::LocalDefId; -use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, LangItem, Node, UnOp}; +use rustc_hir::{Block, Body, Expr, ExprKind, ImplItem, ImplItemKind, Item, LangItem, Node, UnOp}; use rustc_lint::{LateContext, LateLintPass, LintContext}; -use rustc_middle::ty::EarlyBinder; +use rustc_middle::ty::{EarlyBinder, TraitRef}; use rustc_session::declare_lint_pass; use rustc_span::sym; use rustc_span::symbol::kw; @@ -112,7 +112,6 @@ declare_clippy_lint! { declare_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL_PARTIAL_ORD_IMPL]); impl LateLintPass<'_> for NonCanonicalImpls { - #[expect(clippy::too_many_lines)] fn check_impl_item<'tcx>(&mut self, cx: &LateContext<'tcx>, impl_item: &ImplItem<'tcx>) { if let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id()) && let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) @@ -128,114 +127,127 @@ impl LateLintPass<'_> for NonCanonicalImpls { && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) && implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[]) { - if impl_item.ident.name == sym::clone { - if block.stmts.is_empty() - && let Some(expr) = block.expr - && let ExprKind::Unary(UnOp::Deref, deref) = expr.kind - && let ExprKind::Path(qpath) = deref.kind - && last_path_segment(&qpath).ident.name == kw::SelfLower - { - } else { - span_lint_and_sugg( - cx, - NON_CANONICAL_CLONE_IMPL, - block.span, - "non-canonical implementation of `clone` on a `Copy` type", - "change this to", - "{ *self }".to_owned(), - Applicability::MaybeIncorrect, - ); - - return; - } - } - - if impl_item.ident.name == sym::clone_from { - span_lint_and_sugg( - cx, - NON_CANONICAL_CLONE_IMPL, - impl_item.span, - "unnecessary implementation of `clone_from` on a `Copy` type", - "remove it", - String::new(), - Applicability::MaybeIncorrect, - ); - } + check_clone_on_copy(cx, impl_item, block); } else if trait_name == Some(sym::PartialOrd) && impl_item.ident.name == sym::partial_cmp && let Some(ord_def_id) = cx.tcx.get_diagnostic_item(sym::Ord) && implements_trait(cx, trait_impl.self_ty(), ord_def_id, &[]) { - // If the `cmp` call likely needs to be fully qualified in the suggestion - // (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't - // access `cmp_expr` in the suggestion without major changes, as we lint in `else`. - let mut needs_fully_qualified = false; + check_partial_ord_on_ord(cx, impl_item, item, &trait_impl, body, block); + } + } + } +} - if block.stmts.is_empty() - && let Some(expr) = block.expr - && expr_is_cmp(cx, expr, impl_item, &mut needs_fully_qualified) - { - return; - } - // Fix #12683, allow [`needless_return`] here - else if block.expr.is_none() - && let Some(stmt) = block.stmts.first() - && let rustc_hir::StmtKind::Semi(Expr { - kind: ExprKind::Ret(Some(ret)), - .. - }) = stmt.kind - && expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified) - { - return; - } - // If `Self` and `Rhs` are not the same type, bail. This makes creating a valid - // suggestion tons more complex. - else if let [lhs, rhs, ..] = trait_impl.args.as_slice() - && lhs != rhs - { - return; - } +fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &Block<'_>) { + if impl_item.ident.name == sym::clone { + if block.stmts.is_empty() + && let Some(expr) = block.expr + && let ExprKind::Unary(UnOp::Deref, deref) = expr.kind + && let ExprKind::Path(qpath) = deref.kind + && last_path_segment(&qpath).ident.name == kw::SelfLower + { + } else { + span_lint_and_sugg( + cx, + NON_CANONICAL_CLONE_IMPL, + block.span, + "non-canonical implementation of `clone` on a `Copy` type", + "change this to", + "{ *self }".to_owned(), + Applicability::MaybeIncorrect, + ); + } + } - span_lint_and_then( - cx, - NON_CANONICAL_PARTIAL_ORD_IMPL, - item.span, - "non-canonical implementation of `partial_cmp` on an `Ord` type", - |diag| { - let [_, other] = body.params else { - return; - }; - let Some(std_or_core) = std_or_core(cx) else { - return; - }; + if impl_item.ident.name == sym::clone_from { + span_lint_and_sugg( + cx, + NON_CANONICAL_CLONE_IMPL, + impl_item.span, + "unnecessary implementation of `clone_from` on a `Copy` type", + "remove it", + String::new(), + Applicability::MaybeIncorrect, + ); + } +} - let suggs = match (other.pat.simple_ident(), needs_fully_qualified) { - (Some(other_ident), true) => vec![( - block.span, - format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name), - )], - (Some(other_ident), false) => { - vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))] - }, - (None, true) => vec![ - ( - block.span, - format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"), - ), - (other.pat.span, "other".to_owned()), - ], - (None, false) => vec![ - (block.span, "{ Some(self.cmp(other)) }".to_owned()), - (other.pat.span, "other".to_owned()), - ], - }; +fn check_partial_ord_on_ord<'tcx>( + cx: &LateContext<'tcx>, + impl_item: &ImplItem<'_>, + item: &Item<'_>, + trait_impl: &TraitRef<'_>, + body: &Body<'_>, + block: &Block<'tcx>, +) { + // If the `cmp` call likely needs to be fully qualified in the suggestion + // (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't + // access `cmp_expr` in the suggestion without major changes, as we lint in `else`. - diag.multipart_suggestion("change this to", suggs, Applicability::Unspecified); - }, - ); - } - } + let mut needs_fully_qualified = false; + if block.stmts.is_empty() + && let Some(expr) = block.expr + && expr_is_cmp(cx, expr, impl_item, &mut needs_fully_qualified) + { + return; + } + // Fix #12683, allow [`needless_return`] here + else if block.expr.is_none() + && let Some(stmt) = block.stmts.first() + && let rustc_hir::StmtKind::Semi(Expr { + kind: ExprKind::Ret(Some(ret)), + .. + }) = stmt.kind + && expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified) + { + return; + } + // If `Self` and `Rhs` are not the same type, bail. This makes creating a valid + // suggestion tons more complex. + else if let [lhs, rhs, ..] = trait_impl.args.as_slice() + && lhs != rhs + { + return; } + + span_lint_and_then( + cx, + NON_CANONICAL_PARTIAL_ORD_IMPL, + item.span, + "non-canonical implementation of `partial_cmp` on an `Ord` type", + |diag| { + let [_, other] = body.params else { + return; + }; + let Some(std_or_core) = std_or_core(cx) else { + return; + }; + + let suggs = match (other.pat.simple_ident(), needs_fully_qualified) { + (Some(other_ident), true) => vec![( + block.span, + format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name), + )], + (Some(other_ident), false) => { + vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))] + }, + (None, true) => vec![ + ( + block.span, + format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"), + ), + (other.pat.span, "other".to_owned()), + ], + (None, false) => vec![ + (block.span, "{ Some(self.cmp(other)) }".to_owned()), + (other.pat.span, "other".to_owned()), + ], + }; + + diag.multipart_suggestion("change this to", suggs, Applicability::Unspecified); + }, + ); } /// Return true if `expr_kind` is a `cmp` call. From 34d9a2a084b42533d508cbfe3207b91b2f014eba Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Wed, 20 Aug 2025 23:17:32 +0200 Subject: [PATCH 3/4] use `match` imo it's a bit more clear than checking for the same thing (`expr.kind`) in multiple branches --- clippy_lints/src/non_canonical_impls.rs | 37 +++++++++++++------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/non_canonical_impls.rs b/clippy_lints/src/non_canonical_impls.rs index 5f2c388ff678..7dd515734030 100644 --- a/clippy_lints/src/non_canonical_impls.rs +++ b/clippy_lints/src/non_canonical_impls.rs @@ -258,26 +258,27 @@ fn expr_is_cmp<'tcx>( needs_fully_qualified: &mut bool, ) -> bool { let impl_item_did = impl_item.owner_id.def_id; - if let ExprKind::Call( - Expr { - kind: ExprKind::Path(some_path), - hir_id: some_hir_id, - .. - }, - [cmp_expr], - ) = expr.kind - { - is_res_lang_ctor(cx, cx.qpath_res(some_path, *some_hir_id), LangItem::OptionSome) + match expr.kind { + ExprKind::Call( + Expr { + kind: ExprKind::Path(some_path), + hir_id: some_hir_id, + .. + }, + [cmp_expr], + ) => { + is_res_lang_ctor(cx, cx.qpath_res(some_path, *some_hir_id), LangItem::OptionSome) // Fix #11178, allow `Self::cmp(self, ..)` too && self_cmp_call(cx, cmp_expr, impl_item_did, needs_fully_qualified) - } else if let ExprKind::MethodCall(_, recv, [], _) = expr.kind { - cx.tcx - .typeck(impl_item_did) - .type_dependent_def_id(expr.hir_id) - .is_some_and(|def_id| is_diag_trait_item(cx, def_id, sym::Into)) - && self_cmp_call(cx, recv, impl_item_did, needs_fully_qualified) - } else { - false + }, + ExprKind::MethodCall(_, recv, [], _) => { + cx.tcx + .typeck(impl_item_did) + .type_dependent_def_id(expr.hir_id) + .is_some_and(|def_id| is_diag_trait_item(cx, def_id, sym::Into)) + && self_cmp_call(cx, recv, impl_item_did, needs_fully_qualified) + }, + _ => false, } } From 21a5948ca17787961c6dcae8964086ddadb4f1b8 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Sun, 21 Sep 2025 22:04:25 +0200 Subject: [PATCH 4/4] incorporate review feedback --- clippy_lints/src/non_canonical_impls.rs | 30 ++++++++++++++----------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/non_canonical_impls.rs b/clippy_lints/src/non_canonical_impls.rs index 7dd515734030..e531f797272d 100644 --- a/clippy_lints/src/non_canonical_impls.rs +++ b/clippy_lints/src/non_canonical_impls.rs @@ -113,16 +113,18 @@ declare_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL impl LateLintPass<'_> for NonCanonicalImpls { fn check_impl_item<'tcx>(&mut self, cx: &LateContext<'tcx>, impl_item: &ImplItem<'tcx>) { - if let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id()) + if let ImplItemKind::Fn(_, impl_item_id) = impl_item.kind + && let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id()) && let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) + && let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id) + // NOTE: check this early to avoid expensive checks that come after this one + && matches!(trait_name, Some(sym::Clone | sym::PartialOrd)) && !cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) - && let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir_impl_item(impl_item.impl_item_id()).kind && let body = cx.tcx.hir_body(impl_item_id) && let ExprKind::Block(block, ..) = body.value.kind && !block.span.in_external_macro(cx.sess().source_map()) && !is_from_proc_macro(cx, impl_item) { - let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id); if trait_name == Some(sym::Clone) && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) && implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[]) @@ -147,17 +149,19 @@ fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &B && let ExprKind::Path(qpath) = deref.kind && last_path_segment(&qpath).ident.name == kw::SelfLower { - } else { - span_lint_and_sugg( - cx, - NON_CANONICAL_CLONE_IMPL, - block.span, - "non-canonical implementation of `clone` on a `Copy` type", - "change this to", - "{ *self }".to_owned(), - Applicability::MaybeIncorrect, - ); + // this is the canonical implementation, `fn clone(&self) -> Self { *self }` + return; } + + span_lint_and_sugg( + cx, + NON_CANONICAL_CLONE_IMPL, + block.span, + "non-canonical implementation of `clone` on a `Copy` type", + "change this to", + "{ *self }".to_owned(), + Applicability::MaybeIncorrect, + ); } if impl_item.ident.name == sym::clone_from {