mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-10 23:24:24 +00:00
Allow Self::cmp(self, other)
as a correct impl
This commit is contained in:
parent
d4a6634d37
commit
a4c367d0e9
7 changed files with 211 additions and 14 deletions
|
@ -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,
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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"];
|
||||
|
|
|
@ -1,5 +1,4 @@
|
|||
//@run-rustfix
|
||||
#![allow(unused)]
|
||||
#![no_main]
|
||||
|
||||
use std::cmp::Ordering;
|
||||
|
@ -112,3 +111,35 @@ impl PartialOrd<u32> 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<Ordering> {
|
||||
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<Ordering> {
|
||||
Some(Ord::cmp(self, other))
|
||||
}
|
||||
}
|
||||
|
|
|
@ -1,5 +1,4 @@
|
|||
//@run-rustfix
|
||||
#![allow(unused)]
|
||||
#![no_main]
|
||||
|
||||
use std::cmp::Ordering;
|
||||
|
@ -116,3 +115,35 @@ impl PartialOrd<u32> 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<Ordering> {
|
||||
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<Ordering> {
|
||||
Some(Ord::cmp(self, other))
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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<Ordering> {
|
||||
|
@ -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<Ordering> {
|
||||
|
|
|
@ -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<Ordering> {
|
||||
// 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<Ordering> {
|
||||
// This calls `B.cmp`, not `Ord::cmp`!
|
||||
Some(self.cmp(other))
|
||||
}
|
||||
}
|
|
@ -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<Ordering> {
|
||||
| | _____________________________________________________________-
|
||||
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<Ordering> {
|
||||
| | _____________________________________________________________-
|
||||
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
|
||||
|
Loading…
Reference in a new issue