From 2a1fd22f81fd1222a5ceaa554e41095dec34f785 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Mon, 15 May 2023 16:50:28 -0500 Subject: [PATCH 1/4] implement `manual_partial_ord_impl` first try at this --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 1 + clippy_lints/src/manual_partial_ord_impl.rs | 132 ++++++++++++++++++++ tests/ui/manual_partial_ord_impl.rs | 57 +++++++++ tests/ui/manual_partial_ord_impl.stderr | 28 +++++ 6 files changed, 220 insertions(+) create mode 100644 clippy_lints/src/manual_partial_ord_impl.rs create mode 100644 tests/ui/manual_partial_ord_impl.rs create mode 100644 tests/ui/manual_partial_ord_impl.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index da070adb8..840a46aa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4921,6 +4921,7 @@ Released 2018-09-13 [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or +[`manual_partial_ord_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_partial_ord_impl [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains [`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ca97db040..7764dd739 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -277,6 +277,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::manual_let_else::MANUAL_LET_ELSE_INFO, crate::manual_main_separator_str::MANUAL_MAIN_SEPARATOR_STR_INFO, crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO, + crate::manual_partial_ord_impl::MANUAL_PARTIAL_ORD_IMPL_INFO, crate::manual_range_patterns::MANUAL_RANGE_PATTERNS_INFO, crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO, crate::manual_retain::MANUAL_RETAIN_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 00d46025c..36a0cf860 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -188,6 +188,7 @@ mod manual_is_ascii_check; mod manual_let_else; mod manual_main_separator_str; mod manual_non_exhaustive; +mod manual_partial_ord_impl; mod manual_range_patterns; mod manual_rem_euclid; mod manual_retain; diff --git a/clippy_lints/src/manual_partial_ord_impl.rs b/clippy_lints/src/manual_partial_ord_impl.rs new file mode 100644 index 000000000..4c43b5e52 --- /dev/null +++ b/clippy_lints/src/manual_partial_ord_impl.rs @@ -0,0 +1,132 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::{ + def_path_def_ids, diagnostics::span_lint_and_sugg, get_trait_def_id, match_def_path, path_res, ty::implements_trait, +}; +use rustc_errors::Applicability; +use rustc_hir::def::Res; +use rustc_hir::*; +use rustc_hir_analysis::hir_ty_to_ty; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::def_id::DefId; +use std::cell::OnceCell; + +declare_clippy_lint! { + /// ### What it does + /// Checks for manual implementations of both `PartialOrd` and `Ord` when only `Ord` is + /// necessary. + /// + /// ### Why is this bad? + /// If both `PartialOrd` and `Ord` are implemented, `PartialOrd` will wrap the returned value of + /// `Ord::cmp` in `Some`. Not doing this may silently introduce an error. + /// + /// ### Example + /// ```rust + /// #[derive(Eq, PartialEq)] + /// struct A(u32); + /// + /// impl Ord for A { + /// fn cmp(&self, other: &Self) -> Ordering { + /// todo!(); + /// } + /// } + /// + /// impl PartialOrd for A { + /// fn partial_cmp(&self, other: &Self) -> Option { + /// todo!(); + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// #[derive(Eq, PartialEq)] + /// struct A(u32); + /// + /// impl Ord for A { + /// fn cmp(&self, other: &Self) -> Ordering { + /// todo!(); + /// } + /// } + /// + /// impl PartialOrd for A { + /// fn partial_cmp(&self, other: &Self) -> Option { + /// Some(self.cmp(other)) + /// } + /// } + /// ``` + #[clippy::version = "1.71.0"] + pub MANUAL_PARTIAL_ORD_IMPL, + nursery, + "default lint description" +} +impl_lint_pass!(ManualPartialOrdImpl => [MANUAL_PARTIAL_ORD_IMPL]); + +#[derive(Clone)] +pub struct ManualPartialOrdImpl { + pub ord_def_id: OnceCell, +} + +impl LateLintPass<'_> for ManualPartialOrdImpl { + fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + if_chain! { + if let ItemKind::Impl(imp) = &item.kind; + if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(item.owner_id); + if cx.tcx.is_diagnostic_item(sym!(PartialOrd), impl_trait_ref.skip_binder().def_id); + then { + lint_impl_body(self, cx, imp, item); + } + } + } +} + +fn lint_impl_body(conf: &mut ManualPartialOrdImpl, cx: &LateContext<'_>, imp: &Impl<'_>, item: &Item<'_>) { + for imp_item in imp.items { + if_chain! { + if imp_item.ident.name == sym!(partial_cmp); + if let ImplItemKind::Fn(_, id) = cx.tcx.hir().impl_item(imp_item.id).kind; + then { + let body = cx.tcx.hir().body(id); + let ord_def_id = conf.ord_def_id.get_or_init(|| get_trait_def_id(cx, &["core", "cmp", "Ord"]).unwrap()); + if let ExprKind::Block(block, ..) + = body.value.kind && implements_trait(cx, hir_ty_to_ty(cx.tcx, imp.self_ty), *ord_def_id, &[]) + { + if_chain! { + if block.stmts.is_empty(); + if let Some(expr) = block.expr; + if let ExprKind::Call(Expr { kind: ExprKind::Path(path), ..}, [cmp_expr]) = expr.kind; + if let QPath::Resolved(_, some_path) = path; + if let Some(some_seg_one) = some_path.segments.get(0); + if some_seg_one.ident.name == sym!(Some); + if let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind; + if cmp_path.ident.name == sym!(cmp); + if let Res::Local(..) = path_res(cx, other_expr); + then {} + else { + span_lint_and_then( + cx, + MANUAL_PARTIAL_ORD_IMPL, + item.span, + "manual implementation of `PartialOrd` when `Ord` is already implemented", + |diag| { + if let Some(param) = body.params.get(1) + && let PatKind::Binding(_, _, param_ident, ..) = param.pat.kind + { + diag.span_suggestion( + block.span, + "change this to", + format!("{{ Some(self.cmp({})) }}", + param_ident.name), + Applicability::MaybeIncorrect + ); + } else { + diag.help("return the value of `self.cmp` wrapped in `Some` instead"); + }; + } + ); + } + } + } + } + } + } +} diff --git a/tests/ui/manual_partial_ord_impl.rs b/tests/ui/manual_partial_ord_impl.rs new file mode 100644 index 000000000..9e2c7e2ec --- /dev/null +++ b/tests/ui/manual_partial_ord_impl.rs @@ -0,0 +1,57 @@ +#![allow(unused)] +#![warn(clippy::manual_partial_ord_impl)] +#![no_main] + +use std::cmp::Ordering; + +// lint + +#[derive(Eq, PartialEq)] +struct A(u32); + +impl Ord for A { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for A { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} + +// do not lint + +#[derive(Eq, PartialEq)] +struct B(u32); + +impl Ord for B { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for B { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +// lint, but we cannot give a suggestion since &Self is not named + +#[derive(Eq, PartialEq)] +struct C(u32); + +impl Ord for C { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for C { + fn partial_cmp(&self, _: &Self) -> Option { + todo!(); + } +} + diff --git a/tests/ui/manual_partial_ord_impl.stderr b/tests/ui/manual_partial_ord_impl.stderr new file mode 100644 index 000000000..100ddb46e --- /dev/null +++ b/tests/ui/manual_partial_ord_impl.stderr @@ -0,0 +1,28 @@ +error: manual implementation of `PartialOrd` when `Ord` is already implemented + --> $DIR/manual_partial_ord_impl.rs:18:1 + | +LL | / impl PartialOrd for A { +LL | | fn partial_cmp(&self, other: &Self) -> Option { + | | _____________________________________________________________- +LL | || todo!(); +LL | || } + | ||_____- help: change this to: `{ Some(self.cmp(other)) }` +LL | | } + | |__^ + | + = note: `-D clippy::manual-partial-ord-impl` implied by `-D warnings` + +error: manual implementation of `PartialOrd` when `Ord` is already implemented + --> $DIR/manual_partial_ord_impl.rs:52:1 + | +LL | / impl PartialOrd for C { +LL | | fn partial_cmp(&self, _: &Self) -> Option { +LL | | todo!(); +LL | | } +LL | | } + | |_^ + | + = help: return the value of `self.cmp` wrapped in `Some` instead + +error: aborting due to 2 previous errors + From 004e89d4cfde749e9cb33d5ac26bdbe3001a1f3d Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Mon, 15 May 2023 18:41:41 -0500 Subject: [PATCH 2/4] rename to `manual_partial_ord_and_ord_impl` cargo dev fmt cargo test passes cargo test passes refactor a lil Update bool_comparison.stderr heavily refactor + bump `clippy::version` refactor refactor check bounds to increase accuracy, and add todos --- CHANGELOG.md | 2 +- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/manual_partial_ord_impl.rs | 132 ---------------- clippy_lints/src/needless_impls.rs | 145 ++++++++++++++++++ tests/ui/bool_comparison.fixed | 1 + tests/ui/bool_comparison.rs | 1 + tests/ui/bool_comparison.stderr | 44 +++--- tests/ui/derive_ord_xor_partial_ord.rs | 1 + tests/ui/derive_ord_xor_partial_ord.stderr | 16 +- tests/ui/manual_partial_ord_impl.stderr | 28 ---- tests/ui/needless_partial_ord_impl.fixed | 85 ++++++++++ ...d_impl.rs => needless_partial_ord_impl.rs} | 36 ++++- tests/ui/needless_partial_ord_impl.stderr | 28 ++++ 14 files changed, 328 insertions(+), 195 deletions(-) delete mode 100644 clippy_lints/src/manual_partial_ord_impl.rs create mode 100644 clippy_lints/src/needless_impls.rs delete mode 100644 tests/ui/manual_partial_ord_impl.stderr create mode 100644 tests/ui/needless_partial_ord_impl.fixed rename tests/ui/{manual_partial_ord_impl.rs => needless_partial_ord_impl.rs} (52%) create mode 100644 tests/ui/needless_partial_ord_impl.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 840a46aa8..fea11bba6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4921,7 +4921,6 @@ Released 2018-09-13 [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or -[`manual_partial_ord_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_partial_ord_impl [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains [`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid @@ -5021,6 +5020,7 @@ Released 2018-09-13 [`needless_option_as_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_as_deref [`needless_option_take`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_take [`needless_parens_on_range_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_parens_on_range_literals +[`needless_partial_ord_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_partial_ord_impl [`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value [`needless_pub_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pub_self [`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7764dd739..a2231b8fb 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -277,7 +277,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::manual_let_else::MANUAL_LET_ELSE_INFO, crate::manual_main_separator_str::MANUAL_MAIN_SEPARATOR_STR_INFO, crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO, - crate::manual_partial_ord_impl::MANUAL_PARTIAL_ORD_IMPL_INFO, crate::manual_range_patterns::MANUAL_RANGE_PATTERNS_INFO, crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO, crate::manual_retain::MANUAL_RETAIN_INFO, @@ -470,6 +469,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::needless_else::NEEDLESS_ELSE_INFO, crate::needless_for_each::NEEDLESS_FOR_EACH_INFO, crate::needless_if::NEEDLESS_IF_INFO, + crate::needless_impls::NEEDLESS_PARTIAL_ORD_IMPL_INFO, crate::needless_late_init::NEEDLESS_LATE_INIT_INFO, crate::needless_parens_on_range_literals::NEEDLESS_PARENS_ON_RANGE_LITERALS_INFO, crate::needless_pass_by_value::NEEDLESS_PASS_BY_VALUE_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 36a0cf860..1ec5b2e8b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -188,7 +188,6 @@ mod manual_is_ascii_check; mod manual_let_else; mod manual_main_separator_str; mod manual_non_exhaustive; -mod manual_partial_ord_impl; mod manual_range_patterns; mod manual_rem_euclid; mod manual_retain; @@ -228,6 +227,7 @@ mod needless_continue; mod needless_else; mod needless_for_each; mod needless_if; +mod needless_impls; mod needless_late_init; mod needless_parens_on_range_literals; mod needless_pass_by_value; diff --git a/clippy_lints/src/manual_partial_ord_impl.rs b/clippy_lints/src/manual_partial_ord_impl.rs deleted file mode 100644 index 4c43b5e52..000000000 --- a/clippy_lints/src/manual_partial_ord_impl.rs +++ /dev/null @@ -1,132 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::{ - def_path_def_ids, diagnostics::span_lint_and_sugg, get_trait_def_id, match_def_path, path_res, ty::implements_trait, -}; -use rustc_errors::Applicability; -use rustc_hir::def::Res; -use rustc_hir::*; -use rustc_hir_analysis::hir_ty_to_ty; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::def_id::DefId; -use std::cell::OnceCell; - -declare_clippy_lint! { - /// ### What it does - /// Checks for manual implementations of both `PartialOrd` and `Ord` when only `Ord` is - /// necessary. - /// - /// ### Why is this bad? - /// If both `PartialOrd` and `Ord` are implemented, `PartialOrd` will wrap the returned value of - /// `Ord::cmp` in `Some`. Not doing this may silently introduce an error. - /// - /// ### Example - /// ```rust - /// #[derive(Eq, PartialEq)] - /// struct A(u32); - /// - /// impl Ord for A { - /// fn cmp(&self, other: &Self) -> Ordering { - /// todo!(); - /// } - /// } - /// - /// impl PartialOrd for A { - /// fn partial_cmp(&self, other: &Self) -> Option { - /// todo!(); - /// } - /// } - /// ``` - /// Use instead: - /// ```rust - /// #[derive(Eq, PartialEq)] - /// struct A(u32); - /// - /// impl Ord for A { - /// fn cmp(&self, other: &Self) -> Ordering { - /// todo!(); - /// } - /// } - /// - /// impl PartialOrd for A { - /// fn partial_cmp(&self, other: &Self) -> Option { - /// Some(self.cmp(other)) - /// } - /// } - /// ``` - #[clippy::version = "1.71.0"] - pub MANUAL_PARTIAL_ORD_IMPL, - nursery, - "default lint description" -} -impl_lint_pass!(ManualPartialOrdImpl => [MANUAL_PARTIAL_ORD_IMPL]); - -#[derive(Clone)] -pub struct ManualPartialOrdImpl { - pub ord_def_id: OnceCell, -} - -impl LateLintPass<'_> for ManualPartialOrdImpl { - fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { - if_chain! { - if let ItemKind::Impl(imp) = &item.kind; - if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(item.owner_id); - if cx.tcx.is_diagnostic_item(sym!(PartialOrd), impl_trait_ref.skip_binder().def_id); - then { - lint_impl_body(self, cx, imp, item); - } - } - } -} - -fn lint_impl_body(conf: &mut ManualPartialOrdImpl, cx: &LateContext<'_>, imp: &Impl<'_>, item: &Item<'_>) { - for imp_item in imp.items { - if_chain! { - if imp_item.ident.name == sym!(partial_cmp); - if let ImplItemKind::Fn(_, id) = cx.tcx.hir().impl_item(imp_item.id).kind; - then { - let body = cx.tcx.hir().body(id); - let ord_def_id = conf.ord_def_id.get_or_init(|| get_trait_def_id(cx, &["core", "cmp", "Ord"]).unwrap()); - if let ExprKind::Block(block, ..) - = body.value.kind && implements_trait(cx, hir_ty_to_ty(cx.tcx, imp.self_ty), *ord_def_id, &[]) - { - if_chain! { - if block.stmts.is_empty(); - if let Some(expr) = block.expr; - if let ExprKind::Call(Expr { kind: ExprKind::Path(path), ..}, [cmp_expr]) = expr.kind; - if let QPath::Resolved(_, some_path) = path; - if let Some(some_seg_one) = some_path.segments.get(0); - if some_seg_one.ident.name == sym!(Some); - if let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind; - if cmp_path.ident.name == sym!(cmp); - if let Res::Local(..) = path_res(cx, other_expr); - then {} - else { - span_lint_and_then( - cx, - MANUAL_PARTIAL_ORD_IMPL, - item.span, - "manual implementation of `PartialOrd` when `Ord` is already implemented", - |diag| { - if let Some(param) = body.params.get(1) - && let PatKind::Binding(_, _, param_ident, ..) = param.pat.kind - { - diag.span_suggestion( - block.span, - "change this to", - format!("{{ Some(self.cmp({})) }}", - param_ident.name), - Applicability::MaybeIncorrect - ); - } else { - diag.help("return the value of `self.cmp` wrapped in `Some` instead"); - }; - } - ); - } - } - } - } - } - } -} diff --git a/clippy_lints/src/needless_impls.rs b/clippy_lints/src/needless_impls.rs new file mode 100644 index 000000000..4f8741bf3 --- /dev/null +++ b/clippy_lints/src/needless_impls.rs @@ -0,0 +1,145 @@ +use clippy_utils::{ + diagnostics::span_lint_and_then, get_parent_node, is_res_lang_ctor, path_res, ty::implements_trait, +}; +use rustc_errors::Applicability; +use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, PatKind}; +use rustc_hir_analysis::hir_ty_to_ty; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::EarlyBinder; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; +use std::borrow::Cow; + +declare_clippy_lint! { + /// ### What it does + /// Checks for manual implementations of both `PartialOrd` and `Ord` when only `Ord` is + /// necessary. + /// + /// ### Why is this bad? + /// If both `PartialOrd` and `Ord` are implemented, they must agree. This is commonly done by + /// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently + /// introduce an error upon refactoring. + /// + /// ### Example + /// ```rust,ignore + /// #[derive(Eq, PartialEq)] + /// struct A(u32); + /// + /// impl Ord for A { + /// fn cmp(&self, other: &Self) -> Ordering { + /// todo!(); + /// } + /// } + /// + /// impl PartialOrd for A { + /// fn partial_cmp(&self, other: &Self) -> Option { + /// todo!(); + /// } + /// } + /// ``` + /// Use instead: + /// ```rust,ignore + /// #[derive(Eq, PartialEq)] + /// struct A(u32); + /// + /// impl Ord for A { + /// fn cmp(&self, other: &Self) -> Ordering { + /// todo!(); + /// } + /// } + /// + /// impl PartialOrd for A { + /// fn partial_cmp(&self, other: &Self) -> Option { + /// Some(self.cmp(other)) + /// } + /// } + /// ``` + #[clippy::version = "1.72.0"] + pub NEEDLESS_PARTIAL_ORD_IMPL, + correctness, + "manual implementation of `PartialOrd` when `Ord` is already implemented" +} +declare_lint_pass!(NeedlessImpls => [NEEDLESS_PARTIAL_ORD_IMPL]); + +impl LateLintPass<'_> for NeedlessImpls { + fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { + let node = get_parent_node(cx.tcx, impl_item.hir_id()); + let Some(Node::Item(item)) = node else { + return; + }; + let ItemKind::Impl(imp) = item.kind else { + return; + }; + let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else { + return; + }; + let trait_impl_def_id = trait_impl.def_id; + 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 cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl_def_id) + && impl_item.ident.name == sym::partial_cmp + && let Some(ord_def_id) = cx + .tcx + .diagnostic_items(trait_impl.def_id.krate) + .name_to_id + .get(&sym::Ord) + && implements_trait( + cx, + hir_ty_to_ty(cx.tcx, imp.self_ty), + *ord_def_id, + trait_impl.substs, + ) + { + if block.stmts.is_empty() + && let Some(expr) = block.expr + && 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) + && let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind + && cmp_path.ident.name == sym::cmp + && let Res::Local(..) = path_res(cx, other_expr) + {} else { + span_lint_and_then( + cx, + NEEDLESS_PARTIAL_ORD_IMPL, + item.span, + "manual implementation of `PartialOrd` when `Ord` is already implemented", + |diag| { + let (help, app) = if let Some(other) = body.params.get(0) + && let PatKind::Binding(_, _, other_ident, ..) = other.pat.kind + { + ( + Cow::Owned(format!("{{ Some(self.cmp({})) }}", other_ident.name)), + Applicability::Unspecified, + ) + } else { + (Cow::Borrowed("{ Some(self.cmp(...)) }"), Applicability::HasPlaceholders) + }; + + diag.span_suggestion( + block.span, + "change this to", + help, + app, + ); + } + ); + } + } + } +} diff --git a/tests/ui/bool_comparison.fixed b/tests/ui/bool_comparison.fixed index d6774c035..33221c777 100644 --- a/tests/ui/bool_comparison.fixed +++ b/tests/ui/bool_comparison.fixed @@ -2,6 +2,7 @@ #![allow(clippy::needless_if)] #![warn(clippy::bool_comparison)] +#![allow(clippy::needless_partial_ord_impl)] fn main() { let x = true; diff --git a/tests/ui/bool_comparison.rs b/tests/ui/bool_comparison.rs index c0483fd73..5f909bd2a 100644 --- a/tests/ui/bool_comparison.rs +++ b/tests/ui/bool_comparison.rs @@ -2,6 +2,7 @@ #![allow(clippy::needless_if)] #![warn(clippy::bool_comparison)] +#![allow(clippy::needless_partial_ord_impl)] fn main() { let x = true; diff --git a/tests/ui/bool_comparison.stderr b/tests/ui/bool_comparison.stderr index f4dded365..19bdf3013 100644 --- a/tests/ui/bool_comparison.stderr +++ b/tests/ui/bool_comparison.stderr @@ -1,5 +1,5 @@ error: equality checks against true are unnecessary - --> $DIR/bool_comparison.rs:8:8 + --> $DIR/bool_comparison.rs:9:8 | LL | if x == true { | ^^^^^^^^^ help: try simplifying it as shown: `x` @@ -7,127 +7,127 @@ LL | if x == true { = note: `-D clippy::bool-comparison` implied by `-D warnings` error: equality checks against false can be replaced by a negation - --> $DIR/bool_comparison.rs:13:8 + --> $DIR/bool_comparison.rs:14:8 | LL | if x == false { | ^^^^^^^^^^ help: try simplifying it as shown: `!x` error: equality checks against true are unnecessary - --> $DIR/bool_comparison.rs:18:8 + --> $DIR/bool_comparison.rs:19:8 | LL | if true == x { | ^^^^^^^^^ help: try simplifying it as shown: `x` error: equality checks against false can be replaced by a negation - --> $DIR/bool_comparison.rs:23:8 + --> $DIR/bool_comparison.rs:24:8 | LL | if false == x { | ^^^^^^^^^^ help: try simplifying it as shown: `!x` error: inequality checks against true can be replaced by a negation - --> $DIR/bool_comparison.rs:28:8 + --> $DIR/bool_comparison.rs:29:8 | LL | if x != true { | ^^^^^^^^^ help: try simplifying it as shown: `!x` error: inequality checks against false are unnecessary - --> $DIR/bool_comparison.rs:33:8 + --> $DIR/bool_comparison.rs:34:8 | LL | if x != false { | ^^^^^^^^^^ help: try simplifying it as shown: `x` error: inequality checks against true can be replaced by a negation - --> $DIR/bool_comparison.rs:38:8 + --> $DIR/bool_comparison.rs:39:8 | LL | if true != x { | ^^^^^^^^^ help: try simplifying it as shown: `!x` error: inequality checks against false are unnecessary - --> $DIR/bool_comparison.rs:43:8 + --> $DIR/bool_comparison.rs:44:8 | LL | if false != x { | ^^^^^^^^^^ help: try simplifying it as shown: `x` error: less than comparison against true can be replaced by a negation - --> $DIR/bool_comparison.rs:48:8 + --> $DIR/bool_comparison.rs:49:8 | LL | if x < true { | ^^^^^^^^ help: try simplifying it as shown: `!x` error: greater than checks against false are unnecessary - --> $DIR/bool_comparison.rs:53:8 + --> $DIR/bool_comparison.rs:54:8 | LL | if false < x { | ^^^^^^^^^ help: try simplifying it as shown: `x` error: greater than checks against false are unnecessary - --> $DIR/bool_comparison.rs:58:8 + --> $DIR/bool_comparison.rs:59:8 | LL | if x > false { | ^^^^^^^^^ help: try simplifying it as shown: `x` error: less than comparison against true can be replaced by a negation - --> $DIR/bool_comparison.rs:63:8 + --> $DIR/bool_comparison.rs:64:8 | LL | if true > x { | ^^^^^^^^ help: try simplifying it as shown: `!x` error: order comparisons between booleans can be simplified - --> $DIR/bool_comparison.rs:69:8 + --> $DIR/bool_comparison.rs:70:8 | LL | if x < y { | ^^^^^ help: try simplifying it as shown: `!x & y` error: order comparisons between booleans can be simplified - --> $DIR/bool_comparison.rs:74:8 + --> $DIR/bool_comparison.rs:75:8 | LL | if x > y { | ^^^^^ help: try simplifying it as shown: `x & !y` error: this comparison might be written more concisely - --> $DIR/bool_comparison.rs:122:8 + --> $DIR/bool_comparison.rs:123:8 | LL | if a == !b {}; | ^^^^^^^ help: try simplifying it as shown: `a != b` error: this comparison might be written more concisely - --> $DIR/bool_comparison.rs:123:8 + --> $DIR/bool_comparison.rs:124:8 | LL | if !a == b {}; | ^^^^^^^ help: try simplifying it as shown: `a != b` error: this comparison might be written more concisely - --> $DIR/bool_comparison.rs:127:8 + --> $DIR/bool_comparison.rs:128:8 | LL | if b == !a {}; | ^^^^^^^ help: try simplifying it as shown: `b != a` error: this comparison might be written more concisely - --> $DIR/bool_comparison.rs:128:8 + --> $DIR/bool_comparison.rs:129:8 | LL | if !b == a {}; | ^^^^^^^ help: try simplifying it as shown: `b != a` error: equality checks against false can be replaced by a negation - --> $DIR/bool_comparison.rs:152:8 + --> $DIR/bool_comparison.rs:153:8 | LL | if false == m!(func) {} | ^^^^^^^^^^^^^^^^^ help: try simplifying it as shown: `!m!(func)` error: equality checks against false can be replaced by a negation - --> $DIR/bool_comparison.rs:153:8 + --> $DIR/bool_comparison.rs:154:8 | LL | if m!(func) == false {} | ^^^^^^^^^^^^^^^^^ help: try simplifying it as shown: `!m!(func)` error: equality checks against true are unnecessary - --> $DIR/bool_comparison.rs:154:8 + --> $DIR/bool_comparison.rs:155:8 | LL | if true == m!(func) {} | ^^^^^^^^^^^^^^^^ help: try simplifying it as shown: `m!(func)` error: equality checks against true are unnecessary - --> $DIR/bool_comparison.rs:155:8 + --> $DIR/bool_comparison.rs:156:8 | LL | if m!(func) == true {} | ^^^^^^^^^^^^^^^^ help: try simplifying it as shown: `m!(func)` diff --git a/tests/ui/derive_ord_xor_partial_ord.rs b/tests/ui/derive_ord_xor_partial_ord.rs index 6f12d36d7..5d1a69332 100644 --- a/tests/ui/derive_ord_xor_partial_ord.rs +++ b/tests/ui/derive_ord_xor_partial_ord.rs @@ -1,5 +1,6 @@ #![warn(clippy::derive_ord_xor_partial_ord)] #![allow(clippy::unnecessary_wraps)] +#![allow(clippy::needless_partial_ord_impl)] use std::cmp::Ordering; diff --git a/tests/ui/derive_ord_xor_partial_ord.stderr b/tests/ui/derive_ord_xor_partial_ord.stderr index 58efbb854..bd1488348 100644 --- a/tests/ui/derive_ord_xor_partial_ord.stderr +++ b/tests/ui/derive_ord_xor_partial_ord.stderr @@ -1,11 +1,11 @@ error: you are deriving `Ord` but have implemented `PartialOrd` explicitly - --> $DIR/derive_ord_xor_partial_ord.rs:21:10 + --> $DIR/derive_ord_xor_partial_ord.rs:22:10 | LL | #[derive(Ord, PartialEq, Eq)] | ^^^ | note: `PartialOrd` implemented here - --> $DIR/derive_ord_xor_partial_ord.rs:24:1 + --> $DIR/derive_ord_xor_partial_ord.rs:25:1 | LL | impl PartialOrd for DeriveOrd { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -13,20 +13,20 @@ LL | impl PartialOrd for DeriveOrd { = note: this error originates in the derive macro `Ord` (in Nightly builds, run with -Z macro-backtrace for more info) error: you are deriving `Ord` but have implemented `PartialOrd` explicitly - --> $DIR/derive_ord_xor_partial_ord.rs:30:10 + --> $DIR/derive_ord_xor_partial_ord.rs:31:10 | LL | #[derive(Ord, PartialEq, Eq)] | ^^^ | note: `PartialOrd` implemented here - --> $DIR/derive_ord_xor_partial_ord.rs:33:1 + --> $DIR/derive_ord_xor_partial_ord.rs:34:1 | LL | impl PartialOrd for DeriveOrdWithExplicitTypeVariable { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: this error originates in the derive macro `Ord` (in Nightly builds, run with -Z macro-backtrace for more info) error: you are implementing `Ord` explicitly but have derived `PartialOrd` - --> $DIR/derive_ord_xor_partial_ord.rs:42:1 + --> $DIR/derive_ord_xor_partial_ord.rs:43:1 | LL | / impl std::cmp::Ord for DerivePartialOrd { LL | | fn cmp(&self, other: &Self) -> Ordering { @@ -36,14 +36,14 @@ LL | | } | |_^ | note: `PartialOrd` implemented here - --> $DIR/derive_ord_xor_partial_ord.rs:39:10 + --> $DIR/derive_ord_xor_partial_ord.rs:40:10 | LL | #[derive(PartialOrd, PartialEq, Eq)] | ^^^^^^^^^^ = note: this error originates in the derive macro `PartialOrd` (in Nightly builds, run with -Z macro-backtrace for more info) error: you are implementing `Ord` explicitly but have derived `PartialOrd` - --> $DIR/derive_ord_xor_partial_ord.rs:62:5 + --> $DIR/derive_ord_xor_partial_ord.rs:63:5 | LL | / impl Ord for DerivePartialOrdInUseOrd { LL | | fn cmp(&self, other: &Self) -> Ordering { @@ -53,7 +53,7 @@ LL | | } | |_____^ | note: `PartialOrd` implemented here - --> $DIR/derive_ord_xor_partial_ord.rs:59:14 + --> $DIR/derive_ord_xor_partial_ord.rs:60:14 | LL | #[derive(PartialOrd, PartialEq, Eq)] | ^^^^^^^^^^ diff --git a/tests/ui/manual_partial_ord_impl.stderr b/tests/ui/manual_partial_ord_impl.stderr deleted file mode 100644 index 100ddb46e..000000000 --- a/tests/ui/manual_partial_ord_impl.stderr +++ /dev/null @@ -1,28 +0,0 @@ -error: manual implementation of `PartialOrd` when `Ord` is already implemented - --> $DIR/manual_partial_ord_impl.rs:18:1 - | -LL | / impl PartialOrd for A { -LL | | fn partial_cmp(&self, other: &Self) -> Option { - | | _____________________________________________________________- -LL | || todo!(); -LL | || } - | ||_____- help: change this to: `{ Some(self.cmp(other)) }` -LL | | } - | |__^ - | - = note: `-D clippy::manual-partial-ord-impl` implied by `-D warnings` - -error: manual implementation of `PartialOrd` when `Ord` is already implemented - --> $DIR/manual_partial_ord_impl.rs:52:1 - | -LL | / impl PartialOrd for C { -LL | | fn partial_cmp(&self, _: &Self) -> Option { -LL | | todo!(); -LL | | } -LL | | } - | |_^ - | - = help: return the value of `self.cmp` wrapped in `Some` instead - -error: aborting due to 2 previous errors - diff --git a/tests/ui/needless_partial_ord_impl.fixed b/tests/ui/needless_partial_ord_impl.fixed new file mode 100644 index 000000000..7b47773ad --- /dev/null +++ b/tests/ui/needless_partial_ord_impl.fixed @@ -0,0 +1,85 @@ +//@run-rustfix +#![allow(unused)] +#![no_main] + +use std::cmp::Ordering; + +// lint + +#[derive(Eq, PartialEq)] +struct A(u32); + +impl Ord for A { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for A { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(self)) } +} + +// do not lint + +#[derive(Eq, PartialEq)] +struct B(u32); + +impl Ord for B { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for B { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +// lint, but we can't give a suggestion since &Self is not named + +#[derive(Eq, PartialEq)] +struct C(u32); + +impl Ord for C { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for C { + fn partial_cmp(&self, _: &Self) -> Option { Some(self.cmp(self)) } +} + +// do not lint derived + +#[derive(Eq, Ord, PartialEq, PartialOrd)] +struct D(u32); + +// do not lint if ord is not manually implemented + +#[derive(Eq, PartialEq)] +struct E(u32); + +impl PartialOrd for E { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} + +// do not lint since ord has more restrictive bounds + +#[derive(Eq, PartialEq)] +struct Uwu(A); + +impl Ord for Uwu { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for Uwu { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} diff --git a/tests/ui/manual_partial_ord_impl.rs b/tests/ui/needless_partial_ord_impl.rs similarity index 52% rename from tests/ui/manual_partial_ord_impl.rs rename to tests/ui/needless_partial_ord_impl.rs index 9e2c7e2ec..8f5c13373 100644 --- a/tests/ui/manual_partial_ord_impl.rs +++ b/tests/ui/needless_partial_ord_impl.rs @@ -1,5 +1,5 @@ +//@run-rustfix #![allow(unused)] -#![warn(clippy::manual_partial_ord_impl)] #![no_main] use std::cmp::Ordering; @@ -38,7 +38,7 @@ impl PartialOrd for B { } } -// lint, but we cannot give a suggestion since &Self is not named +// lint, but we can't give a suggestion since &Self is not named #[derive(Eq, PartialEq)] struct C(u32); @@ -55,3 +55,35 @@ impl PartialOrd for C { } } +// do not lint derived + +#[derive(Eq, Ord, PartialEq, PartialOrd)] +struct D(u32); + +// do not lint if ord is not manually implemented + +#[derive(Eq, PartialEq)] +struct E(u32); + +impl PartialOrd for E { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} + +// do not lint since ord has more restrictive bounds + +#[derive(Eq, PartialEq)] +struct Uwu(A); + +impl Ord for Uwu { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for Uwu { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} diff --git a/tests/ui/needless_partial_ord_impl.stderr b/tests/ui/needless_partial_ord_impl.stderr new file mode 100644 index 000000000..b8978462c --- /dev/null +++ b/tests/ui/needless_partial_ord_impl.stderr @@ -0,0 +1,28 @@ +error: manual implementation of `PartialOrd` when `Ord` is already implemented + --> $DIR/needless_partial_ord_impl.rs:18:1 + | +LL | / impl PartialOrd for A { +LL | | fn partial_cmp(&self, other: &Self) -> Option { + | | _____________________________________________________________- +LL | || todo!(); +LL | || } + | ||_____- help: change this to: `{ Some(self.cmp(self)) }` +LL | | } + | |__^ + | + = note: `#[deny(clippy::needless_partial_ord_impl)]` on by default + +error: manual implementation of `PartialOrd` when `Ord` is already implemented + --> $DIR/needless_partial_ord_impl.rs:52:1 + | +LL | / impl PartialOrd for C { +LL | | fn partial_cmp(&self, _: &Self) -> Option { + | | _________________________________________________________- +LL | || todo!(); +LL | || } + | ||_____- help: change this to: `{ Some(self.cmp(self)) }` +LL | | } + | |__^ + +error: aborting due to 2 previous errors + From 844afbfebad063d335e30f01e7eab287535f6470 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Thu, 15 Jun 2023 07:11:25 -0500 Subject: [PATCH 3/4] use `other` instead of `self` --- CHANGELOG.md | 2 +- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/incorrect_impls.rs | 130 +++++++++++++++- clippy_lints/src/lib.rs | 1 - clippy_lints/src/needless_impls.rs | 145 ------------------ tests/ui/bool_comparison.fixed | 2 +- tests/ui/bool_comparison.rs | 2 +- tests/ui/derive.rs | 6 +- tests/ui/derive.stderr | 20 +-- tests/ui/derive_ord_xor_partial_ord.rs | 2 +- ...incorrect_partial_ord_impl_on_ord_type.rs} | 32 +++- ...orrect_partial_ord_impl_on_ord_type.stderr | 28 ++++ tests/ui/needless_partial_ord_impl.fixed | 85 ---------- tests/ui/needless_partial_ord_impl.stderr | 28 ---- 14 files changed, 202 insertions(+), 283 deletions(-) delete mode 100644 clippy_lints/src/needless_impls.rs rename tests/ui/{needless_partial_ord_impl.rs => incorrect_partial_ord_impl_on_ord_type.rs} (72%) create mode 100644 tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr delete mode 100644 tests/ui/needless_partial_ord_impl.fixed delete mode 100644 tests/ui/needless_partial_ord_impl.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index fea11bba6..57322887b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4834,6 +4834,7 @@ Released 2018-09-13 [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping [`inconsistent_struct_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor [`incorrect_clone_impl_on_copy_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#incorrect_clone_impl_on_copy_type +[`incorrect_partial_ord_impl_on_ord_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#incorrect_partial_ord_impl_on_ord_type [`index_refutable_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#index_refutable_slice [`indexing_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing [`ineffective_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_bit_mask @@ -5020,7 +5021,6 @@ Released 2018-09-13 [`needless_option_as_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_as_deref [`needless_option_take`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_take [`needless_parens_on_range_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_parens_on_range_literals -[`needless_partial_ord_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_partial_ord_impl [`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value [`needless_pub_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pub_self [`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index a2231b8fb..caff1c4b3 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -207,6 +207,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::implicit_saturating_sub::IMPLICIT_SATURATING_SUB_INFO, crate::inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR_INFO, crate::incorrect_impls::INCORRECT_CLONE_IMPL_ON_COPY_TYPE_INFO, + crate::incorrect_impls::INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE_INFO, crate::index_refutable_slice::INDEX_REFUTABLE_SLICE_INFO, crate::indexing_slicing::INDEXING_SLICING_INFO, crate::indexing_slicing::OUT_OF_BOUNDS_INDEXING_INFO, @@ -469,7 +470,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::needless_else::NEEDLESS_ELSE_INFO, crate::needless_for_each::NEEDLESS_FOR_EACH_INFO, crate::needless_if::NEEDLESS_IF_INFO, - crate::needless_impls::NEEDLESS_PARTIAL_ORD_IMPL_INFO, crate::needless_late_init::NEEDLESS_LATE_INIT_INFO, crate::needless_parens_on_range_literals::NEEDLESS_PARENS_ON_RANGE_LITERALS_INFO, crate::needless_pass_by_value::NEEDLESS_PASS_BY_VALUE_INFO, diff --git a/clippy_lints/src/incorrect_impls.rs b/clippy_lints/src/incorrect_impls.rs index ed21df4ff..f7894f9c1 100644 --- a/clippy_lints/src/incorrect_impls.rs +++ b/clippy_lints/src/incorrect_impls.rs @@ -1,10 +1,16 @@ -use clippy_utils::{diagnostics::span_lint_and_sugg, get_parent_node, last_path_segment, ty::implements_trait}; +use clippy_utils::{ + diagnostics::{span_lint_and_sugg, span_lint_and_then}, + get_parent_node, is_res_lang_ctor, last_path_segment, path_res, + ty::implements_trait, +}; use rustc_errors::Applicability; +use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, PatKind, UnOp}; use rustc_hir::{ExprKind, ImplItem, ImplItemKind, Node, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::EarlyBinder; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{sym, symbol}; +use std::borrow::Cow; declare_clippy_lint! { /// ### What it does @@ -45,10 +51,59 @@ declare_clippy_lint! { correctness, "manual implementation of `Clone` on a `Copy` type" } -declare_lint_pass!(IncorrectImpls => [INCORRECT_CLONE_IMPL_ON_COPY_TYPE]); +declare_clippy_lint! { + /// ### What it does + /// Checks for manual implementations of both `PartialOrd` and `Ord` when only `Ord` is + /// necessary. + /// + /// ### Why is this bad? + /// If both `PartialOrd` and `Ord` are implemented, they must agree. This is commonly done by + /// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently + /// introduce an error upon refactoring. + /// + /// ### Example + /// ```rust,ignore + /// #[derive(Eq, PartialEq)] + /// struct A(u32); + /// + /// impl Ord for A { + /// fn cmp(&self, other: &Self) -> Ordering { + /// todo!(); + /// } + /// } + /// + /// impl PartialOrd for A { + /// fn partial_cmp(&self, other: &Self) -> Option { + /// todo!(); + /// } + /// } + /// ``` + /// Use instead: + /// ```rust,ignore + /// #[derive(Eq, PartialEq)] + /// struct A(u32); + /// + /// impl Ord for A { + /// fn cmp(&self, other: &Self) -> Ordering { + /// todo!(); + /// } + /// } + /// + /// impl PartialOrd for A { + /// fn partial_cmp(&self, other: &Self) -> Option { + /// Some(self.cmp(other)) + /// } + /// } + /// ``` + #[clippy::version = "1.72.0"] + pub INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE, + correctness, + "manual implementation of `PartialOrd` when `Ord` is already implemented" +} +declare_lint_pass!(IncorrectImpls => [INCORRECT_CLONE_IMPL_ON_COPY_TYPE, INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE]); impl LateLintPass<'_> for IncorrectImpls { - #[expect(clippy::needless_return)] + #[expect(clippy::too_many_lines)] fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { let node = get_parent_node(cx.tcx, impl_item.hir_id()); let Some(Node::Item(item)) = node else { @@ -68,10 +123,7 @@ impl LateLintPass<'_> for IncorrectImpls { let ExprKind::Block(block, ..) = body.value.kind else { return; }; - // Above is duplicated from the `duplicate_manual_partial_ord_impl` branch. - // Remove it while solving conflicts once that PR is merged. - // Actual implementation; remove this comment once aforementioned PR is merged if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl_def_id) && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) && implements_trait( @@ -116,5 +168,71 @@ impl LateLintPass<'_> for IncorrectImpls { return; } } + + if cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl_def_id) + && impl_item.ident.name == sym::partial_cmp + && let Some(ord_def_id) = cx + .tcx + .diagnostic_items(trait_impl.def_id.krate) + .name_to_id + .get(&sym::Ord) + && implements_trait( + cx, + hir_ty_to_ty(cx.tcx, imp.self_ty), + *ord_def_id, + trait_impl.substs, + ) + { + if block.stmts.is_empty() + && let Some(expr) = block.expr + && 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) + && let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind + && cmp_path.ident.name == sym::cmp + && let Res::Local(..) = path_res(cx, other_expr) + {} else { + // If lhs and rhs are not the same type, bail. This makes creating a valid + // suggestion tons more complex. + if let Some(lhs) = trait_impl.substs.get(0) + && let Some(rhs) = trait_impl.substs.get(1) + && lhs != rhs + { + return; + } + + span_lint_and_then( + cx, + INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE, + item.span, + "incorrect implementation of `partial_cmp` on an `Ord` type", + |diag| { + let (help, app) = if let Some(other) = body.params.get(1) + && let PatKind::Binding(_, _, other_ident, ..) = other.pat.kind + { + ( + Cow::Owned(format!("{{ Some(self.cmp({})) }}", other_ident.name)), + Applicability::Unspecified, + ) + } else { + (Cow::Borrowed("{ Some(self.cmp(...)) }"), Applicability::HasPlaceholders) + }; + + diag.span_suggestion( + block.span, + "change this to", + help, + app, + ); + } + ); + } + } } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1ec5b2e8b..00d46025c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -227,7 +227,6 @@ mod needless_continue; mod needless_else; mod needless_for_each; mod needless_if; -mod needless_impls; mod needless_late_init; mod needless_parens_on_range_literals; mod needless_pass_by_value; diff --git a/clippy_lints/src/needless_impls.rs b/clippy_lints/src/needless_impls.rs deleted file mode 100644 index 4f8741bf3..000000000 --- a/clippy_lints/src/needless_impls.rs +++ /dev/null @@ -1,145 +0,0 @@ -use clippy_utils::{ - diagnostics::span_lint_and_then, get_parent_node, is_res_lang_ctor, path_res, ty::implements_trait, -}; -use rustc_errors::Applicability; -use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, PatKind}; -use rustc_hir_analysis::hir_ty_to_ty; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::EarlyBinder; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::sym; -use std::borrow::Cow; - -declare_clippy_lint! { - /// ### What it does - /// Checks for manual implementations of both `PartialOrd` and `Ord` when only `Ord` is - /// necessary. - /// - /// ### Why is this bad? - /// If both `PartialOrd` and `Ord` are implemented, they must agree. This is commonly done by - /// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently - /// introduce an error upon refactoring. - /// - /// ### Example - /// ```rust,ignore - /// #[derive(Eq, PartialEq)] - /// struct A(u32); - /// - /// impl Ord for A { - /// fn cmp(&self, other: &Self) -> Ordering { - /// todo!(); - /// } - /// } - /// - /// impl PartialOrd for A { - /// fn partial_cmp(&self, other: &Self) -> Option { - /// todo!(); - /// } - /// } - /// ``` - /// Use instead: - /// ```rust,ignore - /// #[derive(Eq, PartialEq)] - /// struct A(u32); - /// - /// impl Ord for A { - /// fn cmp(&self, other: &Self) -> Ordering { - /// todo!(); - /// } - /// } - /// - /// impl PartialOrd for A { - /// fn partial_cmp(&self, other: &Self) -> Option { - /// Some(self.cmp(other)) - /// } - /// } - /// ``` - #[clippy::version = "1.72.0"] - pub NEEDLESS_PARTIAL_ORD_IMPL, - correctness, - "manual implementation of `PartialOrd` when `Ord` is already implemented" -} -declare_lint_pass!(NeedlessImpls => [NEEDLESS_PARTIAL_ORD_IMPL]); - -impl LateLintPass<'_> for NeedlessImpls { - fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { - let node = get_parent_node(cx.tcx, impl_item.hir_id()); - let Some(Node::Item(item)) = node else { - return; - }; - let ItemKind::Impl(imp) = item.kind else { - return; - }; - let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else { - return; - }; - let trait_impl_def_id = trait_impl.def_id; - 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 cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl_def_id) - && impl_item.ident.name == sym::partial_cmp - && let Some(ord_def_id) = cx - .tcx - .diagnostic_items(trait_impl.def_id.krate) - .name_to_id - .get(&sym::Ord) - && implements_trait( - cx, - hir_ty_to_ty(cx.tcx, imp.self_ty), - *ord_def_id, - trait_impl.substs, - ) - { - if block.stmts.is_empty() - && let Some(expr) = block.expr - && 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) - && let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind - && cmp_path.ident.name == sym::cmp - && let Res::Local(..) = path_res(cx, other_expr) - {} else { - span_lint_and_then( - cx, - NEEDLESS_PARTIAL_ORD_IMPL, - item.span, - "manual implementation of `PartialOrd` when `Ord` is already implemented", - |diag| { - let (help, app) = if let Some(other) = body.params.get(0) - && let PatKind::Binding(_, _, other_ident, ..) = other.pat.kind - { - ( - Cow::Owned(format!("{{ Some(self.cmp({})) }}", other_ident.name)), - Applicability::Unspecified, - ) - } else { - (Cow::Borrowed("{ Some(self.cmp(...)) }"), Applicability::HasPlaceholders) - }; - - diag.span_suggestion( - block.span, - "change this to", - help, - app, - ); - } - ); - } - } - } -} diff --git a/tests/ui/bool_comparison.fixed b/tests/ui/bool_comparison.fixed index 33221c777..8689f89d2 100644 --- a/tests/ui/bool_comparison.fixed +++ b/tests/ui/bool_comparison.fixed @@ -2,7 +2,7 @@ #![allow(clippy::needless_if)] #![warn(clippy::bool_comparison)] -#![allow(clippy::needless_partial_ord_impl)] +#![allow(clippy::incorrect_partial_ord_impl_on_ord_type)] fn main() { let x = true; diff --git a/tests/ui/bool_comparison.rs b/tests/ui/bool_comparison.rs index 5f909bd2a..a1c94aff9 100644 --- a/tests/ui/bool_comparison.rs +++ b/tests/ui/bool_comparison.rs @@ -2,7 +2,7 @@ #![allow(clippy::needless_if)] #![warn(clippy::bool_comparison)] -#![allow(clippy::needless_partial_ord_impl)] +#![allow(clippy::incorrect_partial_ord_impl_on_ord_type)] fn main() { let x = true; diff --git a/tests/ui/derive.rs b/tests/ui/derive.rs index 44e18bff8..ff4dcbfa2 100644 --- a/tests/ui/derive.rs +++ b/tests/ui/derive.rs @@ -1,4 +1,8 @@ -#![allow(clippy::incorrect_clone_impl_on_copy_type, dead_code)] +#![allow( + clippy::incorrect_clone_impl_on_copy_type, + clippy::incorrect_partial_ord_impl_on_ord_type, + dead_code +)] #![warn(clippy::expl_impl_clone_on_copy)] #[derive(Copy)] diff --git a/tests/ui/derive.stderr b/tests/ui/derive.stderr index d37f7fa73..f7948e044 100644 --- a/tests/ui/derive.stderr +++ b/tests/ui/derive.stderr @@ -1,5 +1,5 @@ error: you are implementing `Clone` explicitly on a `Copy` type - --> $DIR/derive.rs:7:1 + --> $DIR/derive.rs:11:1 | LL | / impl Clone for Qux { LL | | fn clone(&self) -> Self { @@ -9,7 +9,7 @@ LL | | } | |_^ | note: consider deriving `Clone` or removing `Copy` - --> $DIR/derive.rs:7:1 + --> $DIR/derive.rs:11:1 | LL | / impl Clone for Qux { LL | | fn clone(&self) -> Self { @@ -20,7 +20,7 @@ LL | | } = note: `-D clippy::expl-impl-clone-on-copy` implied by `-D warnings` error: you are implementing `Clone` explicitly on a `Copy` type - --> $DIR/derive.rs:31:1 + --> $DIR/derive.rs:35:1 | LL | / impl<'a> Clone for Lt<'a> { LL | | fn clone(&self) -> Self { @@ -30,7 +30,7 @@ LL | | } | |_^ | note: consider deriving `Clone` or removing `Copy` - --> $DIR/derive.rs:31:1 + --> $DIR/derive.rs:35:1 | LL | / impl<'a> Clone for Lt<'a> { LL | | fn clone(&self) -> Self { @@ -40,7 +40,7 @@ LL | | } | |_^ error: you are implementing `Clone` explicitly on a `Copy` type - --> $DIR/derive.rs:42:1 + --> $DIR/derive.rs:46:1 | LL | / impl Clone for BigArray { LL | | fn clone(&self) -> Self { @@ -50,7 +50,7 @@ LL | | } | |_^ | note: consider deriving `Clone` or removing `Copy` - --> $DIR/derive.rs:42:1 + --> $DIR/derive.rs:46:1 | LL | / impl Clone for BigArray { LL | | fn clone(&self) -> Self { @@ -60,7 +60,7 @@ LL | | } | |_^ error: you are implementing `Clone` explicitly on a `Copy` type - --> $DIR/derive.rs:53:1 + --> $DIR/derive.rs:57:1 | LL | / impl Clone for FnPtr { LL | | fn clone(&self) -> Self { @@ -70,7 +70,7 @@ LL | | } | |_^ | note: consider deriving `Clone` or removing `Copy` - --> $DIR/derive.rs:53:1 + --> $DIR/derive.rs:57:1 | LL | / impl Clone for FnPtr { LL | | fn clone(&self) -> Self { @@ -80,7 +80,7 @@ LL | | } | |_^ error: you are implementing `Clone` explicitly on a `Copy` type - --> $DIR/derive.rs:73:1 + --> $DIR/derive.rs:77:1 | LL | / impl Clone for Generic2 { LL | | fn clone(&self) -> Self { @@ -90,7 +90,7 @@ LL | | } | |_^ | note: consider deriving `Clone` or removing `Copy` - --> $DIR/derive.rs:73:1 + --> $DIR/derive.rs:77:1 | LL | / impl Clone for Generic2 { LL | | fn clone(&self) -> Self { diff --git a/tests/ui/derive_ord_xor_partial_ord.rs b/tests/ui/derive_ord_xor_partial_ord.rs index 5d1a69332..1fb3d51c4 100644 --- a/tests/ui/derive_ord_xor_partial_ord.rs +++ b/tests/ui/derive_ord_xor_partial_ord.rs @@ -1,6 +1,6 @@ #![warn(clippy::derive_ord_xor_partial_ord)] #![allow(clippy::unnecessary_wraps)] -#![allow(clippy::needless_partial_ord_impl)] +#![allow(clippy::incorrect_partial_ord_impl_on_ord_type)] use std::cmp::Ordering; diff --git a/tests/ui/needless_partial_ord_impl.rs b/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs similarity index 72% rename from tests/ui/needless_partial_ord_impl.rs rename to tests/ui/incorrect_partial_ord_impl_on_ord_type.rs index 8f5c13373..25fe909a8 100644 --- a/tests/ui/needless_partial_ord_impl.rs +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs @@ -1,4 +1,3 @@ -//@run-rustfix #![allow(unused)] #![no_main] @@ -51,7 +50,7 @@ impl Ord for C { impl PartialOrd for C { fn partial_cmp(&self, _: &Self) -> Option { - todo!(); + todo!(); // don't run rustfix, or else this will cause it to fail to compile } } @@ -87,3 +86,32 @@ impl PartialOrd for Uwu { todo!(); } } + +// do not lint since `Rhs` is not `Self` + +#[derive(Eq, PartialEq)] +struct F(u32); + +impl Ord for F { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for F { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl PartialEq for F { + fn eq(&self, other: &u32) -> bool { + todo!(); + } +} + +impl PartialOrd for F { + fn partial_cmp(&self, other: &u32) -> Option { + todo!(); + } +} diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr b/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr new file mode 100644 index 000000000..86b3d3724 --- /dev/null +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr @@ -0,0 +1,28 @@ +error: incorrect implementation of `partial_cmp` on an `Ord` type + --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:17:1 + | +LL | / impl PartialOrd for A { +LL | | fn partial_cmp(&self, other: &Self) -> Option { + | | _____________________________________________________________- +LL | || todo!(); +LL | || } + | ||_____- help: change this to: `{ Some(self.cmp(other)) }` +LL | | } + | |__^ + | + = note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default + +error: incorrect implementation of `partial_cmp` on an `Ord` type + --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:51:1 + | +LL | / impl PartialOrd for C { +LL | | fn partial_cmp(&self, _: &Self) -> Option { + | | _________________________________________________________- +LL | || todo!(); // don't run rustfix, or else this will cause it to fail to compile +LL | || } + | ||_____- help: change this to: `{ Some(self.cmp(...)) }` +LL | | } + | |__^ + +error: aborting due to 2 previous errors + diff --git a/tests/ui/needless_partial_ord_impl.fixed b/tests/ui/needless_partial_ord_impl.fixed deleted file mode 100644 index 7b47773ad..000000000 --- a/tests/ui/needless_partial_ord_impl.fixed +++ /dev/null @@ -1,85 +0,0 @@ -//@run-rustfix -#![allow(unused)] -#![no_main] - -use std::cmp::Ordering; - -// lint - -#[derive(Eq, PartialEq)] -struct A(u32); - -impl Ord for A { - fn cmp(&self, other: &Self) -> Ordering { - todo!(); - } -} - -impl PartialOrd for A { - fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(self)) } -} - -// do not lint - -#[derive(Eq, PartialEq)] -struct B(u32); - -impl Ord for B { - fn cmp(&self, other: &Self) -> Ordering { - todo!(); - } -} - -impl PartialOrd for B { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -// lint, but we can't give a suggestion since &Self is not named - -#[derive(Eq, PartialEq)] -struct C(u32); - -impl Ord for C { - fn cmp(&self, other: &Self) -> Ordering { - todo!(); - } -} - -impl PartialOrd for C { - fn partial_cmp(&self, _: &Self) -> Option { Some(self.cmp(self)) } -} - -// do not lint derived - -#[derive(Eq, Ord, PartialEq, PartialOrd)] -struct D(u32); - -// do not lint if ord is not manually implemented - -#[derive(Eq, PartialEq)] -struct E(u32); - -impl PartialOrd for E { - fn partial_cmp(&self, other: &Self) -> Option { - todo!(); - } -} - -// do not lint since ord has more restrictive bounds - -#[derive(Eq, PartialEq)] -struct Uwu(A); - -impl Ord for Uwu { - fn cmp(&self, other: &Self) -> Ordering { - todo!(); - } -} - -impl PartialOrd for Uwu { - fn partial_cmp(&self, other: &Self) -> Option { - todo!(); - } -} diff --git a/tests/ui/needless_partial_ord_impl.stderr b/tests/ui/needless_partial_ord_impl.stderr deleted file mode 100644 index b8978462c..000000000 --- a/tests/ui/needless_partial_ord_impl.stderr +++ /dev/null @@ -1,28 +0,0 @@ -error: manual implementation of `PartialOrd` when `Ord` is already implemented - --> $DIR/needless_partial_ord_impl.rs:18:1 - | -LL | / impl PartialOrd for A { -LL | | fn partial_cmp(&self, other: &Self) -> Option { - | | _____________________________________________________________- -LL | || todo!(); -LL | || } - | ||_____- help: change this to: `{ Some(self.cmp(self)) }` -LL | | } - | |__^ - | - = note: `#[deny(clippy::needless_partial_ord_impl)]` on by default - -error: manual implementation of `PartialOrd` when `Ord` is already implemented - --> $DIR/needless_partial_ord_impl.rs:52:1 - | -LL | / impl PartialOrd for C { -LL | | fn partial_cmp(&self, _: &Self) -> Option { - | | _________________________________________________________- -LL | || todo!(); -LL | || } - | ||_____- help: change this to: `{ Some(self.cmp(self)) }` -LL | | } - | |__^ - -error: aborting due to 2 previous errors - From a5dfb68491505d8ad2aceb8cb14e1bd7272dcc2a Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Fri, 30 Jun 2023 15:16:56 -0500 Subject: [PATCH 4/4] refactor --- clippy_lints/src/incorrect_impls.rs | 78 ++++++------ .../incorrect_clone_impl_on_copy_type.stderr | 4 +- ...correct_partial_ord_impl_on_ord_type.fixed | 114 ++++++++++++++++++ .../incorrect_partial_ord_impl_on_ord_type.rs | 5 +- ...orrect_partial_ord_impl_on_ord_type.stderr | 23 ++-- 5 files changed, 174 insertions(+), 50 deletions(-) create mode 100644 tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed diff --git a/clippy_lints/src/incorrect_impls.rs b/clippy_lints/src/incorrect_impls.rs index f7894f9c1..25e1cb36b 100644 --- a/clippy_lints/src/incorrect_impls.rs +++ b/clippy_lints/src/incorrect_impls.rs @@ -4,13 +4,12 @@ use clippy_utils::{ ty::implements_trait, }; use rustc_errors::Applicability; -use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, PatKind, UnOp}; -use rustc_hir::{ExprKind, ImplItem, ImplItemKind, Node, UnOp}; +use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, UnOp}; +use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::EarlyBinder; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::{sym, symbol}; -use std::borrow::Cow; +use rustc_span::{sym, symbol::kw}; declare_clippy_lint! { /// ### What it does @@ -61,31 +60,39 @@ declare_clippy_lint! { /// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently /// introduce an error upon refactoring. /// + /// ### Limitations + /// Will not lint if `Self` and `Rhs` do not have the same type. + /// /// ### Example - /// ```rust,ignore + /// ```rust + /// # use std::cmp::Ordering; /// #[derive(Eq, PartialEq)] /// struct A(u32); /// /// impl Ord for A { /// fn cmp(&self, other: &Self) -> Ordering { - /// todo!(); + /// // ... + /// # todo!(); /// } /// } /// /// impl PartialOrd for A { /// fn partial_cmp(&self, other: &Self) -> Option { - /// todo!(); + /// // ... + /// # todo!(); /// } /// } /// ``` /// Use instead: - /// ```rust,ignore + /// ```rust + /// # use std::cmp::Ordering; /// #[derive(Eq, PartialEq)] /// struct A(u32); /// /// impl Ord for A { /// fn cmp(&self, other: &Self) -> Ordering { - /// todo!(); + /// // ... + /// # todo!(); /// } /// } /// @@ -105,17 +112,18 @@ declare_lint_pass!(IncorrectImpls => [INCORRECT_CLONE_IMPL_ON_COPY_TYPE, INCORRE impl LateLintPass<'_> for IncorrectImpls { #[expect(clippy::too_many_lines)] fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { - let node = get_parent_node(cx.tcx, impl_item.hir_id()); - let Some(Node::Item(item)) = node else { + let Some(Node::Item(item)) = get_parent_node(cx.tcx, impl_item.hir_id()) else { return; }; let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else { return; }; - let trait_impl_def_id = trait_impl.def_id; if cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) { return; } + let ItemKind::Impl(imp) = item.kind else { + return; + }; let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir().impl_item(impl_item.impl_item_id()).kind else { return; }; @@ -124,7 +132,7 @@ impl LateLintPass<'_> for IncorrectImpls { return; }; - if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl_def_id) + if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl.def_id) && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) && implements_trait( cx, @@ -136,9 +144,9 @@ impl LateLintPass<'_> for IncorrectImpls { if impl_item.ident.name == sym::clone { if block.stmts.is_empty() && let Some(expr) = block.expr - && let ExprKind::Unary(UnOp::Deref, inner) = expr.kind - && let ExprKind::Path(qpath) = inner.kind - && last_path_segment(&qpath).ident.name == symbol::kw::SelfLower + && 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, @@ -160,7 +168,7 @@ impl LateLintPass<'_> for IncorrectImpls { INCORRECT_CLONE_IMPL_ON_COPY_TYPE, impl_item.span, "incorrect implementation of `clone_from` on a `Copy` type", - "remove this", + "remove it", String::new(), Applicability::MaybeIncorrect, ); @@ -169,7 +177,7 @@ impl LateLintPass<'_> for IncorrectImpls { } } - if cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl_def_id) + if cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl.def_id) && impl_item.ident.name == sym::partial_cmp && let Some(ord_def_id) = cx .tcx @@ -198,12 +206,9 @@ impl LateLintPass<'_> for IncorrectImpls { && cmp_path.ident.name == sym::cmp && let Res::Local(..) = path_res(cx, other_expr) {} else { - // If lhs and rhs are not the same type, bail. This makes creating a valid + // If `Self` and `Rhs` are not the same type, bail. This makes creating a valid // suggestion tons more complex. - if let Some(lhs) = trait_impl.substs.get(0) - && let Some(rhs) = trait_impl.substs.get(1) - && lhs != rhs - { + if let [lhs, rhs, ..] = trait_impl.substs.as_slice() && lhs != rhs { return; } @@ -213,22 +218,23 @@ impl LateLintPass<'_> for IncorrectImpls { item.span, "incorrect implementation of `partial_cmp` on an `Ord` type", |diag| { - let (help, app) = if let Some(other) = body.params.get(1) - && let PatKind::Binding(_, _, other_ident, ..) = other.pat.kind - { - ( - Cow::Owned(format!("{{ Some(self.cmp({})) }}", other_ident.name)), - Applicability::Unspecified, - ) - } else { - (Cow::Borrowed("{ Some(self.cmp(...)) }"), Applicability::HasPlaceholders) + let [_, other] = body.params else { + return; }; - diag.span_suggestion( - block.span, + let suggs = if let Some(other_ident) = other.pat.simple_ident() { + vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))] + } else { + vec![ + (block.span, "{ Some(self.cmp(other)) }".to_owned()), + (other.pat.span, "other".to_owned()), + ] + }; + + diag.multipart_suggestion( "change this to", - help, - app, + suggs, + Applicability::Unspecified, ); } ); diff --git a/tests/ui/incorrect_clone_impl_on_copy_type.stderr b/tests/ui/incorrect_clone_impl_on_copy_type.stderr index 0021841aa..7bcba8ba4 100644 --- a/tests/ui/incorrect_clone_impl_on_copy_type.stderr +++ b/tests/ui/incorrect_clone_impl_on_copy_type.stderr @@ -16,7 +16,7 @@ LL | / fn clone_from(&mut self, source: &Self) { LL | | source.clone(); LL | | *self = source.clone(); LL | | } - | |_____^ help: remove this + | |_____^ help: remove it error: incorrect implementation of `clone` on a `Copy` type --> $DIR/incorrect_clone_impl_on_copy_type.rs:81:29 @@ -34,7 +34,7 @@ LL | / fn clone_from(&mut self, source: &Self) { LL | | source.clone(); LL | | *self = source.clone(); LL | | } - | |_____^ help: remove this + | |_____^ help: remove it error: aborting due to 4 previous errors diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed b/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed new file mode 100644 index 000000000..dd4fdd988 --- /dev/null +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed @@ -0,0 +1,114 @@ +//@run-rustfix +#![allow(unused)] +#![no_main] + +use std::cmp::Ordering; + +// lint + +#[derive(Eq, PartialEq)] +struct A(u32); + +impl Ord for A { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for A { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } +} + +// do not lint + +#[derive(Eq, PartialEq)] +struct B(u32); + +impl Ord for B { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for B { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +// lint, and give `_` a name + +#[derive(Eq, PartialEq)] +struct C(u32); + +impl Ord for C { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for C { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } +} + +// do not lint derived + +#[derive(Eq, Ord, PartialEq, PartialOrd)] +struct D(u32); + +// do not lint if ord is not manually implemented + +#[derive(Eq, PartialEq)] +struct E(u32); + +impl PartialOrd for E { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} + +// do not lint since ord has more restrictive bounds + +#[derive(Eq, PartialEq)] +struct Uwu(A); + +impl Ord for Uwu { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for Uwu { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +} + +// do not lint since `Rhs` is not `Self` + +#[derive(Eq, PartialEq)] +struct F(u32); + +impl Ord for F { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for F { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl PartialEq for F { + fn eq(&self, other: &u32) -> bool { + todo!(); + } +} + +impl PartialOrd for F { + fn partial_cmp(&self, other: &u32) -> Option { + todo!(); + } +} diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs b/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs index 25fe909a8..522e82299 100644 --- a/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs @@ -1,3 +1,4 @@ +//@run-rustfix #![allow(unused)] #![no_main] @@ -37,7 +38,7 @@ impl PartialOrd for B { } } -// lint, but we can't give a suggestion since &Self is not named +// lint, and give `_` a name #[derive(Eq, PartialEq)] struct C(u32); @@ -50,7 +51,7 @@ impl Ord for C { impl PartialOrd for C { fn partial_cmp(&self, _: &Self) -> Option { - todo!(); // don't run rustfix, or else this will cause it to fail to compile + todo!(); } } diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr b/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr index 86b3d3724..0e477798c 100644 --- a/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr @@ -1,5 +1,5 @@ error: incorrect implementation of `partial_cmp` on an `Ord` type - --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:17:1 + --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:18:1 | LL | / impl PartialOrd for A { LL | | fn partial_cmp(&self, other: &Self) -> Option { @@ -13,16 +13,19 @@ LL | | } = note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default error: incorrect implementation of `partial_cmp` on an `Ord` type - --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:51:1 + --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:52:1 | -LL | / impl PartialOrd for C { -LL | | fn partial_cmp(&self, _: &Self) -> Option { - | | _________________________________________________________- -LL | || todo!(); // don't run rustfix, or else this will cause it to fail to compile -LL | || } - | ||_____- help: change this to: `{ Some(self.cmp(...)) }` -LL | | } - | |__^ +LL | / impl PartialOrd for C { +LL | | fn partial_cmp(&self, _: &Self) -> Option { +LL | | todo!(); +LL | | } +LL | | } + | |_^ + | +help: change this to + | +LL | fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } + | ~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~ error: aborting due to 2 previous errors