Auto merge of #10788 - Centri3:duplicate_manual_partial_ord_impl, r=x…

…Frednet,blyxyas New lint [`manual_partial_ord_and_ord_impl`] Lints when both `PartialOrd` and `Ord` are manually implemented yet the body of `PartialOrd::partial_cmp` isn't `Some(self.cmp(<other>))`. This is in `correctness` currently but I'm ok with elsewhere. Closes #10744 --- changelog: new lint [`needless_partial_ord_impl`] [#10788](#10788)
rust-lang · Jul 8, 2023 · e5db198 · e5db198
2 parents 3a1fc26 + a5dfb68
commit e5db198
Show file tree

Hide file tree

Showing 14 changed files with 455 additions and 59 deletions.
diff --git 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

diff --git 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,

diff --git a/clippy_lints/src/incorrect_impls.rs b/clippy_lints/src/incorrect_impls.rs
@@ -1,10 +1,15 @@
-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::{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 rustc_span::{sym, symbol::kw};
 
 declare_clippy_lint! {
  /// ### What it does
@@ -45,34 +50,89 @@ 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.
+ ///
+ /// ### Limitations
+ /// Will not lint if `Self` and `Rhs` do not have the same type.
+ ///
+ /// ### Example
+ /// ```rust
+ /// # use std::cmp::Ordering;
+ /// #[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> {
+ /// // ...
+ /// # todo!();
+ /// }
+ /// }
+ /// ```
+ /// Use instead:
+ /// ```rust
+ /// # use std::cmp::Ordering;
+ /// #[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))
+ /// }
+ /// }
+ /// ```
+ #[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 {
+ 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;
  };
  let body = cx.tcx.hir().body(impl_item_id);
  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)
+ 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,
@@ -84,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,
@@ -108,13 +168,77 @@ 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,
  );
 
  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 `Self` and `Rhs` are not the same type, bail. This makes creating a valid
+ // suggestion tons more complex.
+ if let [lhs, rhs, ..] = trait_impl.substs.as_slice() && 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 [_, 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 {
+ vec![
+ (block.span, "{ Some(self.cmp(other)) }".to_owned()),
+ (other.pat.span, "other".to_owned()),
+ ]
+ };
+
+ diag.multipart_suggestion(
+ "change this to",
+ suggs,
+ Applicability::Unspecified,
+ );
+ }
+ );
+ }
+ }
  }
 }
diff --git 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::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
@@ -2,6 +2,7 @@
 
 #![allow(clippy::needless_if)]
 #![warn(clippy::bool_comparison)]
+#![allow(clippy::incorrect_partial_ord_impl_on_ord_type)]
 
 fn main() {
  let x = true;

diff --git a/tests/ui/bool_comparison.stderr b/tests/ui/bool_comparison.stderr
@@ -1,133 +1,133 @@
 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`
  |
  = 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)`