From e4ac6ba6e51d8bea6059ce6e78a60a61a15a0984 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Mon, 5 Jun 2023 04:46:05 -0500 Subject: [PATCH] refactor --- clippy_lints/src/lib.rs | 6 +-- .../src/manual_partial_ord_and_ord_impl.rs | 46 +++++++------------ 2 files changed, 18 insertions(+), 34 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 29cde7747479..c1895c8a1dd2 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1008,11 +1008,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(default_constructed_unit_structs::DefaultConstructedUnitStructs)); 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(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` } diff --git a/clippy_lints/src/manual_partial_ord_and_ord_impl.rs b/clippy_lints/src/manual_partial_ord_and_ord_impl.rs index e9346a1bc907..d50dde1b8695 100644 --- a/clippy_lints/src/manual_partial_ord_and_ord_impl.rs +++ b/clippy_lints/src/manual_partial_ord_and_ord_impl.rs @@ -1,15 +1,13 @@ 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 @@ -17,8 +15,9 @@ declare_clippy_lint! { /// 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, -} - -// 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");