refactor

clippy_lints/src/incorrect_impls.rs

@@ -4,12 +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::{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 rustc_span::{sym, symbol::kw};
 use std::borrow::Cow;
 
 declare_clippy_lint! {
@@ -61,31 +61,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<Ordering> {
- /// 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,8 +113,7 @@ 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 ItemKind::Impl(imp) = item.kind else {
@@ -115,7 +122,6 @@ impl LateLintPass<'_> for IncorrectImpls {
  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;
  }
@@ -127,7 +133,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,
@@ -139,9 +145,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,
@@ -163,7 +169,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,
  );
@@ -172,7 +178,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
@@ -203,10 +209,7 @@ impl LateLintPass<'_> for IncorrectImpls {
  {} 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
- {
+ if let [lhs, rhs, ..] = trait_impl.substs.as_slice() && lhs != rhs {
  return;
  }
 
@@ -216,22 +219,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,
- )
+ let [_, other] = body.params else {
+ return;
+ };
+
+ let suggs = if let Some(other_ident) = other.pat.simple_ident() {
+ vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))]
  } else {
- (Cow::Borrowed("{ Some(self.cmp(...)) }"), Applicability::HasPlaceholders)
+ vec![
+ (block.span, "{ Some(self.cmp(other)) }".to_owned()),
+ (other.pat.span, "other".to_owned()),
+ ]
  };
 
- diag.span_suggestion(
- block.span,
+ diag.multipart_suggestion(
  "change this to",
- help,
- app,
+ suggs,
+ Applicability::Unspecified,
  );
  }
  );

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
 

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<Ordering> { 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<Ordering> {
+ 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<Ordering> { 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<Ordering> {
+ todo!();
+ }
+}
+
+// do not lint since ord has more restrictive bounds
+
+#[derive(Eq, PartialEq)]
+struct Uwu<A>(A);
+
+impl<A: std::fmt::Debug + Ord + PartialOrd> Ord for Uwu<A> {
+ fn cmp(&self, other: &Self) -> Ordering {
+ todo!();
+ }
+}
+
+impl<A: Ord + PartialOrd> PartialOrd for Uwu<A> {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+ 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<Ordering> {
+ Some(self.cmp(other))
+ }
+}
+
+impl PartialEq<u32> for F {
+ fn eq(&self, other: &u32) -> bool {
+ todo!();
+ }
+}
+
+impl PartialOrd<u32> for F {
+ fn partial_cmp(&self, other: &u32) -> Option<Ordering> {
+ todo!();
+ }
+}

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<Ordering> {
- todo!(); // don't run rustfix, or else this will cause it to fail to compile
+ todo!();
  }
 }
 

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<Ordering> {
@@ -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<Ordering> {
- | | _________________________________________________________-
-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<Ordering> {
+LL | | todo!();
+LL | | }
+LL | | }
+ | |_^
+ |
+help: change this to
+ |
+LL | fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
+ | ~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~
 
 error: aborting due to 2 previous errors