diff --git a/clippy_lints/src/incorrect_impls.rs b/clippy_lints/src/incorrect_impls.rs index 166908ef4e4..e8a2042e92a 100644 --- a/clippy_lints/src/incorrect_impls.rs +++ b/clippy_lints/src/incorrect_impls.rs @@ -1,8 +1,9 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::paths::ORD_CMP; use clippy_utils::ty::implements_trait; -use clippy_utils::{get_parent_node, is_res_lang_ctor, last_path_segment, path_res}; +use clippy_utils::{get_parent_node, is_res_lang_ctor, last_path_segment, match_def_path, path_res, std_or_core}; use rustc_errors::Applicability; -use rustc_hir::def::Res; +use rustc_hir::def_id::LocalDefId; use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, UnOp}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; @@ -60,6 +61,10 @@ declare_clippy_lint! { /// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently /// introduce an error upon refactoring. /// + /// ### Known issues + /// Code that calls the `.into()` method instead will be flagged as incorrect, despite `.into()` + /// wrapping it in `Some`. + /// /// ### Limitations /// Will not lint if `Self` and `Rhs` do not have the same type. /// @@ -191,6 +196,11 @@ impl LateLintPass<'_> for IncorrectImpls { trait_impl.substs, ) { + // If the `cmp` call likely needs to be fully qualified in the suggestion + // (like `std::cmp::Ord::cmp`). It's unfortunate we must put this here but we can't + // access `cmp_expr` in the suggestion without major changes, as we lint in `else`. + let mut needs_fully_qualified = false; + if block.stmts.is_empty() && let Some(expr) = block.expr && let ExprKind::Call( @@ -202,9 +212,8 @@ impl LateLintPass<'_> for IncorrectImpls { [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) + // Fix #11178, allow `Self::cmp(self, ..)` too + && self_cmp_call(cx, cmp_expr, impl_item.owner_id.def_id, &mut needs_fully_qualified) {} else { // If `Self` and `Rhs` are not the same type, bail. This makes creating a valid // suggestion tons more complex. @@ -221,14 +230,29 @@ impl LateLintPass<'_> for IncorrectImpls { let [_, other] = body.params else { return; }; + let Some(std_or_core) = std_or_core(cx) 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![ + let suggs = match (other.pat.simple_ident(), needs_fully_qualified) { + (Some(other_ident), true) => vec![( + block.span, + format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, {})) }}", other_ident.name), + )], + (Some(other_ident), false) => { + vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))] + }, + (None, true) => vec![ + ( + block.span, + format!("{{ Some({std_or_core}::cmp::Ord::cmp(self, other)) }}"), + ), + (other.pat.span, "other".to_owned()), + ], + (None, false) => vec![ (block.span, "{ Some(self.cmp(other)) }".to_owned()), (other.pat.span, "other".to_owned()), - ] + ], }; diag.multipart_suggestion( @@ -242,3 +266,31 @@ impl LateLintPass<'_> for IncorrectImpls { } } } + +/// Returns whether this is any of `self.cmp(..)`, `Self::cmp(self, ..)` or `Ord::cmp(self, ..)`. +fn self_cmp_call<'tcx>( + cx: &LateContext<'tcx>, + cmp_expr: &'tcx Expr<'tcx>, + def_id: LocalDefId, + needs_fully_qualified: &mut bool, +) -> bool { + match cmp_expr.kind { + ExprKind::Call(path, [_self, _other]) => path_res(cx, path) + .opt_def_id() + .is_some_and(|def_id| match_def_path(cx, def_id, &ORD_CMP)), + ExprKind::MethodCall(_, _, [_other], ..) => { + // We can set this to true here no matter what as if it's a `MethodCall` and goes to the + // `else` branch, it must be a method named `cmp` that isn't `Ord::cmp` + *needs_fully_qualified = true; + + // It's a bit annoying but `typeck_results` only gives us the CURRENT body, which we + // have none, not of any `LocalDefId` we want, so we must call the query itself to avoid + // an immediate ICE + cx.tcx + .typeck(def_id) + .type_dependent_def_id(cmp_expr.hir_id) + .is_some_and(|def_id| match_def_path(cx, def_id, &ORD_CMP)) + }, + _ => false, + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index f3677e6f614..8fc13f23ef9 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -161,3 +161,4 @@ pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"]; pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"]; pub const FORMATTER: [&str; 3] = ["core", "fmt", "Formatter"]; pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"]; +pub const ORD_CMP: [&str; 4] = ["core", "cmp", "Ord", "cmp"]; diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed b/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed index dd4fdd98822..2f51bf27480 100644 --- a/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed @@ -1,5 +1,4 @@ //@run-rustfix -#![allow(unused)] #![no_main] use std::cmp::Ordering; @@ -112,3 +111,35 @@ impl PartialOrd for F { todo!(); } } + +// #11178, do not lint + +#[derive(Eq, PartialEq)] +struct G(u32); + +impl Ord for G { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for G { + fn partial_cmp(&self, other: &Self) -> Option { + Some(Self::cmp(self, other)) + } +} + +#[derive(Eq, PartialEq)] +struct H(u32); + +impl Ord for H { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for H { + fn partial_cmp(&self, other: &Self) -> Option { + Some(Ord::cmp(self, other)) + } +} diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs b/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs index 522e82299c0..47127bdaec2 100644 --- a/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.rs @@ -1,5 +1,4 @@ //@run-rustfix -#![allow(unused)] #![no_main] use std::cmp::Ordering; @@ -116,3 +115,35 @@ impl PartialOrd for F { todo!(); } } + +// #11178, do not lint + +#[derive(Eq, PartialEq)] +struct G(u32); + +impl Ord for G { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for G { + fn partial_cmp(&self, other: &Self) -> Option { + Some(Self::cmp(self, other)) + } +} + +#[derive(Eq, PartialEq)] +struct H(u32); + +impl Ord for H { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for H { + fn partial_cmp(&self, other: &Self) -> Option { + Some(Ord::cmp(self, other)) + } +} diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr b/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr index 0e477798c40..66048fc9000 100644 --- a/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr @@ -1,5 +1,5 @@ error: incorrect implementation of `partial_cmp` on an `Ord` type - --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:18:1 + --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:17:1 | LL | / impl PartialOrd for A { LL | | fn partial_cmp(&self, other: &Self) -> Option { @@ -13,7 +13,7 @@ LL | | } = note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default error: incorrect implementation of `partial_cmp` on an `Ord` type - --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:52:1 + --> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:51:1 | LL | / impl PartialOrd for C { LL | | fn partial_cmp(&self, _: &Self) -> Option { diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs b/tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs new file mode 100644 index 00000000000..3a3b84f93c4 --- /dev/null +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs @@ -0,0 +1,51 @@ +// This test's filename is... a bit verbose. But it ensures we suggest the correct code when `Ord` +// is not in scope. +#![no_main] +#![no_implicit_prelude] + +extern crate std; + +use std::cmp::{self, Eq, Ordering, PartialEq, PartialOrd}; +use std::option::Option::{self, Some}; +use std::todo; + +// lint + +#[derive(Eq, PartialEq)] +struct A(u32); + +impl cmp::Ord for A { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for A { + fn partial_cmp(&self, other: &Self) -> Option { + // NOTE: This suggestion is wrong, as `Ord` is not in scope. But this should be fine as it isn't + // automatically applied + todo!(); + } +} + +#[derive(Eq, PartialEq)] +struct B(u32); + +impl B { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl cmp::Ord for B { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for B { + fn partial_cmp(&self, other: &Self) -> Option { + // This calls `B.cmp`, not `Ord::cmp`! + Some(self.cmp(other)) + } +} diff --git a/tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.stderr b/tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.stderr new file mode 100644 index 00000000000..f4374c28128 --- /dev/null +++ b/tests/ui/incorrect_partial_ord_impl_on_ord_type_fully_qual.stderr @@ -0,0 +1,31 @@ +error: incorrect implementation of `partial_cmp` on an `Ord` type + --> $DIR/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs:23:1 + | +LL | / impl PartialOrd for A { +LL | | fn partial_cmp(&self, other: &Self) -> Option { + | | _____________________________________________________________- +LL | || // NOTE: This suggestion is wrong, as `Ord` is not in scope. But this should be fine as it isn't +LL | || // automatically applied +LL | || todo!(); +LL | || } + | ||_____- help: change this to: `{ Some(self.cmp(other)) }` +LL | | } + | |__^ + | + = note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default + +error: incorrect implementation of `partial_cmp` on an `Ord` type + --> $DIR/incorrect_partial_ord_impl_on_ord_type_fully_qual.rs:46:1 + | +LL | / impl PartialOrd for B { +LL | | fn partial_cmp(&self, other: &Self) -> Option { + | | _____________________________________________________________- +LL | || // This calls `B.cmp`, not `Ord::cmp`! +LL | || Some(self.cmp(other)) +LL | || } + | ||_____- help: change this to: `{ Some(std::cmp::Ord::cmp(self, other)) }` +LL | | } + | |__^ + +error: aborting due to 2 previous errors +