diff --git a/CHANGELOG.md b/CHANGELOG.md index 8929341bab83..6d13fb3c8247 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4833,6 +4833,7 @@ Released 2018-09-13 [`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops [`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_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 @@ -5017,7 +5018,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_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark [`needless_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0f4821a79af1..05fd7b6a54e9 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -206,6 +206,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::implicit_saturating_add::IMPLICIT_SATURATING_ADD_INFO, crate::implicit_saturating_sub::IMPLICIT_SATURATING_SUB_INFO, crate::inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR_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, @@ -463,7 +464,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/needless_impls.rs b/clippy_lints/src/incorrect_impls.rs similarity index 87% rename from clippy_lints/src/needless_impls.rs rename to clippy_lints/src/incorrect_impls.rs index 4f8741bf3708..a3bcd3c7d55e 100644 --- a/clippy_lints/src/needless_impls.rs +++ b/clippy_lints/src/incorrect_impls.rs @@ -55,13 +55,13 @@ declare_clippy_lint! { /// } /// ``` #[clippy::version = "1.72.0"] - pub NEEDLESS_PARTIAL_ORD_IMPL, + pub INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE, correctness, "manual implementation of `PartialOrd` when `Ord` is already implemented" } -declare_lint_pass!(NeedlessImpls => [NEEDLESS_PARTIAL_ORD_IMPL]); +declare_lint_pass!(IncorrectImpls => [INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE]); -impl LateLintPass<'_> for NeedlessImpls { +impl LateLintPass<'_> for IncorrectImpls { 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 { @@ -114,13 +114,22 @@ impl LateLintPass<'_> for NeedlessImpls { && 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, - NEEDLESS_PARTIAL_ORD_IMPL, + INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE, item.span, - "manual implementation of `PartialOrd` when `Ord` is already implemented", + "incorrect implementation of `partial_cmp` on an `Ord` type", |diag| { - let (help, app) = if let Some(other) = body.params.get(0) + let (help, app) = if let Some(other) = body.params.get(1) && let PatKind::Binding(_, _, other_ident, ..) = other.pat.kind { ( diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6fe9a0551285..b254c05d48bf 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -150,6 +150,7 @@ mod implicit_return; mod implicit_saturating_add; mod implicit_saturating_sub; mod inconsistent_struct_constructor; +mod incorrect_impls; mod index_refutable_slice; mod indexing_slicing; mod infinite_iter; @@ -226,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; @@ -1048,6 +1048,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let stack_size_threshold = conf.stack_size_threshold; store.register_late_pass(move |_| Box::new(large_stack_frames::LargeStackFrames::new(stack_size_threshold))); store.register_late_pass(|_| Box::new(single_range_in_vec_init::SingleRangeInVecInit)); + store.register_late_pass(|_| Box::new(incorrect_impls::IncorrectImpls)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/bool_comparison.fixed b/tests/ui/bool_comparison.fixed index 33221c7777ea..8689f89d2c33 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 5f909bd2a53f..a1c94aff94b2 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 402550513fe8..05177146e270 100644 --- a/tests/ui/derive.rs +++ b/tests/ui/derive.rs @@ -1,4 +1,4 @@ -#![allow(clippy::needless_partial_ord_impl, dead_code)] +#![allow(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_ord_xor_partial_ord.rs b/tests/ui/derive_ord_xor_partial_ord.rs index 5d1a693321e5..1fb3d51c46dc 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 8f5c1337369b..25fe909a8d39 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 000000000000..86b3d37241a7 --- /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 7b47773ad766..000000000000 --- 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 b8978462cc0c..000000000000 --- 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 -