Auto merge of #11358 - Alexendoo:incorrect-to-manual-impls, r=Jarcho

Rename incorrect_impls to non_canonical_impls, move them to warn by default

The wording/category of these feel too strong to me, I would expect most of the time it's linting the implementations aren't going to be *incorrect*, just unnecessary

changelog: rename `incorrect_clone_impl_on_copy_type` to [`non_canonical_clone_impl`]
changelog: rename `incorrect_partial_ord_impl_on_ord_type` to [`non_canonical_partial_ord_impl`]
changelog: Move [`non_canonical_clone_impl`], [`non_canonical_partial_ord_impl`] to suspicious

CHANGELOG.md

@@ -5206,6 +5206,8 @@ Released 2018-09-13
 [`no_effect_underscore_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect_underscore_binding
 [`no_mangle_with_rust_abi`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi
 [`non_ascii_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_ascii_literal
+[`non_canonical_clone_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
+[`non_canonical_partial_ord_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_partial_ord_impl
 [`non_minimal_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_minimal_cfg
 [`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions
 [`non_send_fields_in_send_ty`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_send_fields_in_send_ty

clippy_lints/src/declared_lints.rs

@@ -211,8 +211,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::implicit_saturating_sub::IMPLICIT_SATURATING_SUB_INFO,
     crate::implied_bounds_in_impls::IMPLIED_BOUNDS_IN_IMPLS_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,
@@ -498,6 +496,8 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::no_effect::NO_EFFECT_UNDERSCORE_BINDING_INFO,
     crate::no_effect::UNNECESSARY_OPERATION_INFO,
     crate::no_mangle_with_rust_abi::NO_MANGLE_WITH_RUST_ABI_INFO,
+    crate::non_canonical_impls::NON_CANONICAL_CLONE_IMPL_INFO,
+    crate::non_canonical_impls::NON_CANONICAL_PARTIAL_ORD_IMPL_INFO,
     crate::non_copy_const::BORROW_INTERIOR_MUTABLE_CONST_INFO,
     crate::non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST_INFO,
     crate::non_expressive_names::JUST_UNDERSCORES_AND_DIGITS_INFO,

clippy_lints/src/lib.rs

@@ -154,7 +154,6 @@ mod implicit_saturating_add;
 mod implicit_saturating_sub;
 mod implied_bounds_in_impls;
 mod inconsistent_struct_constructor;
-mod incorrect_impls;
 mod index_refutable_slice;
 mod indexing_slicing;
 mod infinite_iter;
@@ -244,6 +243,7 @@ mod neg_multiply;
 mod new_without_default;
 mod no_effect;
 mod no_mangle_with_rust_abi;
+mod non_canonical_impls;
 mod non_copy_const;
 mod non_expressive_names;
 mod non_octal_unix_permissions;
@@ -1070,7 +1070,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
             avoid_breaking_exported_api,
         ))
     });
-    store.register_late_pass(|_| Box::new(incorrect_impls::IncorrectImpls));
+    store.register_late_pass(|_| Box::new(non_canonical_impls::NonCanonicalImpls));
     store.register_late_pass(move |_| {
         Box::new(single_call_fn::SingleCallFn {
             avoid_breaking_exported_api,

clippy_lints/src/incorrect_impls.rs → clippy_lints/src/non_canonical_impls.rs

@@ -14,11 +14,12 @@ use rustc_span::symbol::kw;
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for manual implementations of `Clone` when `Copy` is already implemented.
+    /// Checks for non-canonical implementations of `Clone` when `Copy` is already implemented.
     ///
     /// ### Why is this bad?
-    /// If both `Clone` and `Copy` are implemented, they must agree. This is done by dereferencing
-    /// `self` in `Clone`'s implementation. Anything else is incorrect.
+    /// If both `Clone` and `Copy` are implemented, they must agree. This can done by dereferencing
+    /// `self` in `Clone`'s implementation, which will avoid any possibility of the implementations
+    /// becoming out of sync.
     ///
     /// ### Example
     /// ```rust,ignore
@@ -47,26 +48,22 @@ declare_clippy_lint! {
     /// impl Copy for A {}
     /// ```
     #[clippy::version = "1.72.0"]
-    pub INCORRECT_CLONE_IMPL_ON_COPY_TYPE,
-    correctness,
-    "manual implementation of `Clone` on a `Copy` type"
+    pub NON_CANONICAL_CLONE_IMPL,
+    suspicious,
+    "non-canonical implementation of `Clone` on a `Copy` type"
 }
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for manual implementations of both `PartialOrd` and `Ord` when only `Ord` is
-    /// necessary.
+    /// Checks for non-canonical implementations of `PartialOrd` when `Ord` is already implemented.
     ///
     /// ### 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.
     ///
     /// ### Known issues
-    /// Code that calls the `.into()` method instead will be flagged as incorrect, despite `.into()`
-    /// wrapping it in `Some`.
-    ///
-    /// ### Limitations
-    /// Will not lint if `Self` and `Rhs` do not have the same type.
+    /// Code that calls the `.into()` method instead will be flagged, despite `.into()` wrapping it
+    /// in `Some`.
     ///
     /// ### Example
     /// ```rust
@@ -108,13 +105,13 @@ declare_clippy_lint! {
     /// }
     /// ```
     #[clippy::version = "1.72.0"]
-    pub INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE,
-    correctness,
-    "manual implementation of `PartialOrd` when `Ord` is already implemented"
+    pub NON_CANONICAL_PARTIAL_ORD_IMPL,
+    suspicious,
+    "non-canonical implementation of `PartialOrd` on an `Ord` type"
 }
-declare_lint_pass!(IncorrectImpls => [INCORRECT_CLONE_IMPL_ON_COPY_TYPE, INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE]);
+declare_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL_PARTIAL_ORD_IMPL]);
 
-impl LateLintPass<'_> for IncorrectImpls {
+impl LateLintPass<'_> for NonCanonicalImpls {
     #[expect(clippy::too_many_lines)]
     fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
         let Some(Node::Item(item)) = get_parent_node(cx.tcx, impl_item.hir_id()) else {
@@ -155,9 +152,9 @@ impl LateLintPass<'_> for IncorrectImpls {
                 {} else {
                     span_lint_and_sugg(
                         cx,
-                        INCORRECT_CLONE_IMPL_ON_COPY_TYPE,
+                        NON_CANONICAL_CLONE_IMPL,
                         block.span,
-                        "incorrect implementation of `clone` on a `Copy` type",
+                        "non-canonical implementation of `clone` on a `Copy` type",
                         "change this to",
                         "{ *self }".to_owned(),
                         Applicability::MaybeIncorrect,
@@ -170,9 +167,9 @@ impl LateLintPass<'_> for IncorrectImpls {
             if impl_item.ident.name == sym::clone_from {
                 span_lint_and_sugg(
                     cx,
-                    INCORRECT_CLONE_IMPL_ON_COPY_TYPE,
+                    NON_CANONICAL_CLONE_IMPL,
                     impl_item.span,
-                    "incorrect implementation of `clone_from` on a `Copy` type",
+                    "unnecessary implementation of `clone_from` on a `Copy` type",
                     "remove it",
                     String::new(),
                     Applicability::MaybeIncorrect,
@@ -218,9 +215,9 @@ impl LateLintPass<'_> for IncorrectImpls {
 
                 span_lint_and_then(
                     cx,
-                    INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE,
+                    NON_CANONICAL_PARTIAL_ORD_IMPL,
                     item.span,
-                    "incorrect implementation of `partial_cmp` on an `Ord` type",
+                    "non-canonical implementation of `partial_cmp` on an `Ord` type",
                     |diag| {
                         let [_, other] = body.params else {
                             return;

clippy_lints/src/renamed_lints.rs

@@ -15,6 +15,8 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
     ("clippy::eval_order_dependence", "clippy::mixed_read_write_in_expression"),
     ("clippy::identity_conversion", "clippy::useless_conversion"),
     ("clippy::if_let_some_result", "clippy::match_result_ok"),
+    ("clippy::incorrect_clone_impl_on_copy_type", "clippy::non_canonical_clone_impl"),
+    ("clippy::incorrect_partial_ord_impl_on_ord_type", "clippy::non_canonical_partial_ord_impl"),
     ("clippy::integer_arithmetic", "clippy::arithmetic_side_effects"),
     ("clippy::logic_bug", "clippy::overly_complex_bool_expr"),
     ("clippy::new_without_default_derive", "clippy::new_without_default"),
@@ -38,12 +40,12 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
     ("clippy::drop_bounds", "drop_bounds"),
     ("clippy::drop_copy", "dropping_copy_types"),
     ("clippy::drop_ref", "dropping_references"),
+    ("clippy::fn_null_check", "useless_ptr_null_checks"),
     ("clippy::for_loop_over_option", "for_loops_over_fallibles"),
     ("clippy::for_loop_over_result", "for_loops_over_fallibles"),
     ("clippy::for_loops_over_fallibles", "for_loops_over_fallibles"),
     ("clippy::forget_copy", "forgetting_copy_types"),
     ("clippy::forget_ref", "forgetting_references"),
-    ("clippy::fn_null_check", "useless_ptr_null_checks"),
     ("clippy::into_iter_on_array", "array_into_iter"),
     ("clippy::invalid_atomic_ordering", "invalid_atomic_ordering"),
     ("clippy::invalid_ref", "invalid_value"),

tests/ui/bool_comparison.fixed

@@ -1,6 +1,6 @@
 #![allow(clippy::needless_if)]
 #![warn(clippy::bool_comparison)]
-#![allow(clippy::incorrect_partial_ord_impl_on_ord_type)]
+#![allow(clippy::non_canonical_partial_ord_impl)]
 
 fn main() {
     let x = true;

tests/ui/bool_comparison.rs

@@ -1,6 +1,6 @@
 #![allow(clippy::needless_if)]
 #![warn(clippy::bool_comparison)]
-#![allow(clippy::incorrect_partial_ord_impl_on_ord_type)]
+#![allow(clippy::non_canonical_partial_ord_impl)]
 
 fn main() {
     let x = true;

tests/ui/clone_on_copy_impl.rs

@@ -1,4 +1,4 @@
-#![allow(clippy::incorrect_clone_impl_on_copy_type)]
+#![allow(clippy::non_canonical_clone_impl)]
 
 use std::fmt;
 use std::marker::PhantomData;

tests/ui/derive.rs

@@ -1,8 +1,4 @@
-#![allow(
-    clippy::incorrect_clone_impl_on_copy_type,
-    clippy::incorrect_partial_ord_impl_on_ord_type,
-    dead_code
-)]
+#![allow(clippy::non_canonical_clone_impl, clippy::non_canonical_partial_ord_impl, dead_code)]
 #![warn(clippy::expl_impl_clone_on_copy)]
 
 #[derive(Copy)]

tests/ui/derive.stderr

@@ -1,5 +1,5 @@
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> $DIR/derive.rs:11:1
+  --> $DIR/derive.rs:7:1
    |
 LL | / impl Clone for Qux {
 LL | |
@@ -10,7 +10,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> $DIR/derive.rs:11:1
+  --> $DIR/derive.rs:7:1
    |
 LL | / impl Clone for Qux {
 LL | |
@@ -23,7 +23,7 @@ LL | | }
    = help: to override `-D warnings` add `#[allow(clippy::expl_impl_clone_on_copy)]`
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> $DIR/derive.rs:36:1
+  --> $DIR/derive.rs:32:1
    |
 LL | / impl<'a> Clone for Lt<'a> {
 LL | |
@@ -34,7 +34,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> $DIR/derive.rs:36:1
+  --> $DIR/derive.rs:32:1
    |
 LL | / impl<'a> Clone for Lt<'a> {
 LL | |
@@ -45,7 +45,7 @@ LL | | }
    | |_^
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> $DIR/derive.rs:48:1
+  --> $DIR/derive.rs:44:1
    |
 LL | / impl Clone for BigArray {
 LL | |
@@ -56,7 +56,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> $DIR/derive.rs:48:1
+  --> $DIR/derive.rs:44:1
    |
 LL | / impl Clone for BigArray {
 LL | |
@@ -67,7 +67,7 @@ LL | | }
    | |_^
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> $DIR/derive.rs:60:1
+  --> $DIR/derive.rs:56:1
    |
 LL | / impl Clone for FnPtr {
 LL | |
@@ -78,7 +78,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> $DIR/derive.rs:60:1
+  --> $DIR/derive.rs:56:1
    |
 LL | / impl Clone for FnPtr {
 LL | |
@@ -89,7 +89,7 @@ LL | | }
    | |_^
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> $DIR/derive.rs:81:1
+  --> $DIR/derive.rs:77:1
    |
 LL | / impl<T: Clone> Clone for Generic2<T> {
 LL | |
@@ -100,7 +100,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> $DIR/derive.rs:81:1
+  --> $DIR/derive.rs:77:1
    |
 LL | / impl<T: Clone> Clone for Generic2<T> {
 LL | |

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::incorrect_partial_ord_impl_on_ord_type)]
+#![allow(clippy::non_canonical_partial_ord_impl)]
 
 use std::cmp::Ordering;
 

tests/ui/incorrect_clone_impl_on_copy_type.stderr → tests/ui/non_canonical_clone_impl.stderr

@@ -1,34 +1,35 @@
-error: incorrect implementation of `clone` on a `Copy` type
-  --> $DIR/incorrect_clone_impl_on_copy_type.rs:9:29
+error: non-canonical implementation of `clone` on a `Copy` type
+  --> $DIR/non_canonical_clone_impl.rs:9:29
    |
 LL |       fn clone(&self) -> Self {
    |  _____________________________^
 LL | |         Self(self.0)
 LL | |     }
    | |_____^ help: change this to: `{ *self }`
    |
-   = note: `#[deny(clippy::incorrect_clone_impl_on_copy_type)]` on by default
+   = note: `-D clippy::non-canonical-clone-impl` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::non_canonical_clone_impl)]`
 
-error: incorrect implementation of `clone_from` on a `Copy` type
-  --> $DIR/incorrect_clone_impl_on_copy_type.rs:13:5
+error: unnecessary implementation of `clone_from` on a `Copy` type
+  --> $DIR/non_canonical_clone_impl.rs:13:5
    |
 LL | /     fn clone_from(&mut self, source: &Self) {
 LL | |         source.clone();
 LL | |         *self = source.clone();
 LL | |     }
    | |_____^ help: remove it
 
-error: incorrect implementation of `clone` on a `Copy` type
-  --> $DIR/incorrect_clone_impl_on_copy_type.rs:80:29
+error: non-canonical implementation of `clone` on a `Copy` type
+  --> $DIR/non_canonical_clone_impl.rs:80:29
    |
 LL |       fn clone(&self) -> Self {
    |  _____________________________^
 LL | |         Self(self.0)
 LL | |     }
    | |_____^ help: change this to: `{ *self }`
 
-error: incorrect implementation of `clone_from` on a `Copy` type
-  --> $DIR/incorrect_clone_impl_on_copy_type.rs:84:5
+error: unnecessary implementation of `clone_from` on a `Copy` type
+  --> $DIR/non_canonical_clone_impl.rs:84:5
    |
 LL | /     fn clone_from(&mut self, source: &Self) {
 LL | |         source.clone();

tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr → tests/ui/non_canonical_partial_ord_impl.stderr

@@ -1,5 +1,5 @@
-error: incorrect implementation of `partial_cmp` on an `Ord` type
-  --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:16:1
+error: non-canonical implementation of `partial_cmp` on an `Ord` type
+  --> $DIR/non_canonical_partial_ord_impl.rs:16:1
    |
 LL | /  impl PartialOrd for A {
 LL | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
@@ -10,10 +10,11 @@ LL | ||     }
 LL | |  }
    | |__^
    |
-   = note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default
+   = note: `-D clippy::non-canonical-partial-ord-impl` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::non_canonical_partial_ord_impl)]`
 
-error: incorrect implementation of `partial_cmp` on an `Ord` type
-  --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:50:1
+error: non-canonical implementation of `partial_cmp` on an `Ord` type
+  --> $DIR/non_canonical_partial_ord_impl.rs:50:1
    |
 LL | / impl PartialOrd for C {
 LL | |     fn partial_cmp(&self, _: &Self) -> Option<Ordering> {

tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs → tests/ui/non_canonical_partial_ord_impl_fully_qual.rs

@@ -21,8 +21,6 @@ impl cmp::Ord for A {
 }
 
 impl PartialOrd for A {
-    //~^ ERROR: incorrect implementation of `partial_cmp` on an `Ord` type
-    //~| NOTE: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         // NOTE: This suggestion is wrong, as `Ord` is not in scope. But this should be fine as it isn't
         // automatically applied
@@ -46,7 +44,6 @@ impl cmp::Ord for B {
 }
 
 impl PartialOrd for B {
-    //~^ ERROR: incorrect implementation of `partial_cmp` on an `Ord` type
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         // This calls `B.cmp`, not `Ord::cmp`!
         Some(self.cmp(other))