diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/manual_partial_ord_impl.rs | 132 | ||||
| -rw-r--r-- | tests/ui/manual_partial_ord_impl.rs | 57 | ||||
| -rw-r--r-- | tests/ui/manual_partial_ord_impl.stderr | 28 |
6 files changed, 220 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index da070adb808..840a46aa83a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4921,6 +4921,7 @@ Released 2018-09-13 [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or +[`manual_partial_ord_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_partial_ord_impl [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains [`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ca97db04079..7764dd7391f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -277,6 +277,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::manual_let_else::MANUAL_LET_ELSE_INFO, crate::manual_main_separator_str::MANUAL_MAIN_SEPARATOR_STR_INFO, crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO, + crate::manual_partial_ord_impl::MANUAL_PARTIAL_ORD_IMPL_INFO, crate::manual_range_patterns::MANUAL_RANGE_PATTERNS_INFO, crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO, crate::manual_retain::MANUAL_RETAIN_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 00d46025caa..36a0cf860d7 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -188,6 +188,7 @@ mod manual_is_ascii_check; mod manual_let_else; mod manual_main_separator_str; mod manual_non_exhaustive; +mod manual_partial_ord_impl; mod manual_range_patterns; mod manual_rem_euclid; mod manual_retain; diff --git a/clippy_lints/src/manual_partial_ord_impl.rs b/clippy_lints/src/manual_partial_ord_impl.rs new file mode 100644 index 00000000000..4c43b5e52e8 --- /dev/null +++ b/clippy_lints/src/manual_partial_ord_impl.rs @@ -0,0 +1,132 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::{ + def_path_def_ids, diagnostics::span_lint_and_sugg, get_trait_def_id, match_def_path, path_res, ty::implements_trait, +}; +use rustc_errors::Applicability; +use rustc_hir::def::Res; +use rustc_hir::*; +use rustc_hir_analysis::hir_ty_to_ty; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::def_id::DefId; +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. + /// + /// ### Example + /// ```rust + /// #[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 + /// #[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.71.0"] + pub MANUAL_PARTIAL_ORD_IMPL, + nursery, + "default lint description" +} +impl_lint_pass!(ManualPartialOrdImpl => [MANUAL_PARTIAL_ORD_IMPL]); + +#[derive(Clone)] +pub struct ManualPartialOrdImpl { + pub ord_def_id: OnceCell<DefId>, +} + +impl LateLintPass<'_> for ManualPartialOrdImpl { + fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + if_chain! { + if let ItemKind::Impl(imp) = &item.kind; + if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(item.owner_id); + if cx.tcx.is_diagnostic_item(sym!(PartialOrd), impl_trait_ref.skip_binder().def_id); + then { + lint_impl_body(self, cx, imp, item); + } + } + } +} + +fn lint_impl_body(conf: &mut ManualPartialOrdImpl, cx: &LateContext<'_>, imp: &Impl<'_>, item: &Item<'_>) { + 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); + let ord_def_id = conf.ord_def_id.get_or_init(|| get_trait_def_id(cx, &["core", "cmp", "Ord"]).unwrap()); + if let ExprKind::Block(block, ..) + = body.value.kind && implements_trait(cx, hir_ty_to_ty(cx.tcx, imp.self_ty), *ord_def_id, &[]) + { + if_chain! { + if block.stmts.is_empty(); + if let Some(expr) = block.expr; + if let ExprKind::Call(Expr { kind: ExprKind::Path(path), ..}, [cmp_expr]) = expr.kind; + if let QPath::Resolved(_, some_path) = path; + if let Some(some_seg_one) = some_path.segments.get(0); + if some_seg_one.ident.name == sym!(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); + then {} + else { + span_lint_and_then( + cx, + MANUAL_PARTIAL_ORD_IMPL, + item.span, + "manual implementation of `PartialOrd` when `Ord` is already implemented", + |diag| { + if let Some(param) = body.params.get(1) + && let PatKind::Binding(_, _, param_ident, ..) = param.pat.kind + { + diag.span_suggestion( + block.span, + "change this to", + format!("{{ Some(self.cmp({})) }}", + param_ident.name), + Applicability::MaybeIncorrect + ); + } else { + diag.help("return the value of `self.cmp` wrapped in `Some` instead"); + }; + } + ); + } + } + } + } + } + } +} diff --git a/tests/ui/manual_partial_ord_impl.rs b/tests/ui/manual_partial_ord_impl.rs new file mode 100644 index 00000000000..9e2c7e2ec51 --- /dev/null +++ b/tests/ui/manual_partial_ord_impl.rs @@ -0,0 +1,57 @@ +#![allow(unused)] +#![warn(clippy::manual_partial_ord_impl)] +#![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> { + todo!(); + } +} + +// 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, but we cannot give a suggestion since &Self is not named + +#[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, _: &Self) -> Option<Ordering> { + todo!(); + } +} + diff --git a/tests/ui/manual_partial_ord_impl.stderr b/tests/ui/manual_partial_ord_impl.stderr new file mode 100644 index 00000000000..100ddb46e82 --- /dev/null +++ b/tests/ui/manual_partial_ord_impl.stderr @@ -0,0 +1,28 @@ +error: manual implementation of `PartialOrd` when `Ord` is already implemented + --> $DIR/manual_partial_ord_impl.rs:18:1 + | +LL | / impl PartialOrd for A { +LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> { + | | _____________________________________________________________- +LL | || todo!(); +LL | || } + | ||_____- help: change this to: `{ Some(self.cmp(other)) }` +LL | | } + | |__^ + | + = note: `-D clippy::manual-partial-ord-impl` implied by `-D warnings` + +error: manual implementation of `PartialOrd` when `Ord` is already implemented + --> $DIR/manual_partial_ord_impl.rs:52:1 + | +LL | / impl PartialOrd for C { +LL | | fn partial_cmp(&self, _: &Self) -> Option<Ordering> { +LL | | todo!(); +LL | | } +LL | | } + | |_^ + | + = help: return the value of `self.cmp` wrapped in `Some` instead + +error: aborting due to 2 previous errors + |