refactor

clippy_lints/src/lib.rs

@@ -1013,11 +1013,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
  store.register_early_pass(|| Box::new(needless_else::NeedlessElse));
  store.register_late_pass(|_| Box::new(missing_fields_in_debug::MissingFieldsInDebug));
  store.register_late_pass(|_| Box::new(endian_bytes::EndianBytes));
- store.register_late_pass(|_| {
- Box::new(manual_partial_ord_and_ord_impl::ManualPartialOrdAndOrdImpl {
- ord_def_id: std::cell::OnceCell::new(),
- })
- });
+ 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");