refactor

Author: Centri3

clippy_lints/src/lib.rs

@@ -1022,6 +1022,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(endian_bytes::EndianBytes));
     store.register_late_pass(|_| Box::new(redundant_type_annotations::RedundantTypeAnnotations));
     store.register_late_pass(|_| Box::new(arc_with_non_send_sync::ArcWithNonSendSync));
+    store.register_late_pass(|_| Box::new(manual_partial_ord_and_ord_impl::ManualPartialOrdAndOrdImpl));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/manual_partial_ord_and_ord_impl.rs

@@ -1,24 +1,23 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::{get_trait_def_id, match_qpath, path_res, ty::implements_trait};
+use clippy_utils::{match_qpath, path_res, ty::implements_trait};
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
 use rustc_hir::{Expr, ExprKind, Impl, ImplItemKind, Item, ItemKind, PatKind};
 use rustc_hir_analysis::hir_ty_to_ty;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::{EarlyBinder, TraitRef};
-use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::def_id::DefId;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::sym;
-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 upon refactoring.
+    /// 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
@@ -59,15 +58,7 @@ declare_clippy_lint! {
     correctness,
     "manual implementation of `PartialOrd` when `Ord` is already implemented"
 }
-impl_lint_pass!(ManualPartialOrdAndOrdImpl => [MANUAL_PARTIAL_ORD_AND_ORD_IMPL]);
-
-#[derive(Clone)]
-pub struct ManualPartialOrdAndOrdImpl {
-    pub ord_def_id: OnceCell<DefId>,
-}
-
-// TODO: The number of if_chain! calls makes this is a bit hard to follow, can that be reduced?
-// or more generally, can this be done cleaner?
+declare_lint_pass!(ManualPartialOrdAndOrdImpl => [MANUAL_PARTIAL_ORD_AND_ORD_IMPL]);
 
 impl LateLintPass<'_> for ManualPartialOrdAndOrdImpl {
     fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
@@ -78,28 +69,25 @@ impl LateLintPass<'_> for ManualPartialOrdAndOrdImpl {
             if cx.tcx.is_diagnostic_item(sym::PartialOrd, partial_ord_def_id);
             if !cx.tcx.is_automatically_derived(item.owner_id.to_def_id());
             then {
-                lint_impl_body(self, cx, imp, item, impl_trait_ref);
+                lint_impl_body(cx, imp, item, impl_trait_ref);
             }
         }
     }
 }
 
-#[allow(clippy::unnecessary_def_path)] // ???
-fn lint_impl_body<'tcx>(
-    conf: &mut ManualPartialOrdAndOrdImpl,
-    cx: &LateContext<'tcx>,
-    imp: &Impl<'_>,
-    item: &Item<'_>,
-    impl_trait_ref: TraitRef<'tcx>,
-) {
+fn lint_impl_body<'tcx>(cx: &LateContext<'tcx>, imp: &Impl<'_>, item: &Item<'_>, impl_trait_ref: TraitRef<'tcx>) {
     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);
-                // TODO: This can't be changed without causing compilation to fail
-                let ord_def_id = conf.ord_def_id.get_or_init(|| get_trait_def_id(cx, &["core", "cmp", "Ord"]).unwrap());
+                let ord_def_id = cx
+                    .tcx
+                    .diagnostic_items(impl_trait_ref.def_id.krate)
+                    .name_to_id
+                    .get(&sym::Ord)
+                    .unwrap();
                 if let ExprKind::Block(block, ..) = body.value.kind && implements_trait(
                         cx,
                         hir_ty_to_ty(cx.tcx, imp.self_ty),
@@ -111,11 +99,12 @@ fn lint_impl_body<'tcx>(
                         if block.stmts.is_empty();
                         if let Some(expr) = block.expr;
                         if let ExprKind::Call(Expr { kind: ExprKind::Path(some_path), ..}, [cmp_expr]) = expr.kind;
+                        // TODO: We can likely use the `option_some_variant` lang item instead, but this may be tricky
+                        // due to the fact that `expr` is the constructor, not `option_some_variant` itself.
                         if match_qpath(some_path, &["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);
-                        // TODO: Self explanatory
                         then {}
                         else {
                             span_lint_and_then(
@@ -134,8 +123,7 @@ fn lint_impl_body<'tcx>(
                                             block.span,
                                             "change this to",
                                             format!("{{ Some(self.cmp({})) }}", param_ident.name),
-                                            // TODO: This is always correct afaik.
-                                            Applicability::MaybeIncorrect
+                                            Applicability::Unspecified,
                                         );
                                     } else {
                                         diag.help("return the value of `self.cmp` wrapped in `Some` instead");