Skip to content

Commit a5dfb68

Browse files
Centri3Centri3
committed
refactor
1 parent 844afbf commit a5dfb68

File tree

5 files changed

+173
-49
lines changed

5 files changed

+173
-49
lines changed

‎clippy_lints/src/incorrect_impls.rs‎

Lines changed: 41 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,12 @@ use clippy_utils::{
44
ty::implements_trait,
55
};
66
use rustc_errors::Applicability;
7-
use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, PatKind, UnOp};
8-
use rustc_hir::{ExprKind, ImplItem, ImplItemKind, Node, UnOp};
7+
use rustc_hir::{def::Res, Expr, ExprKind, ImplItem, ImplItemKind, ItemKind, LangItem, Node, UnOp};
8+
use rustc_hir_analysis::hir_ty_to_ty;
99
use rustc_lint::{LateContext, LateLintPass};
1010
use rustc_middle::ty::EarlyBinder;
1111
use rustc_session::{declare_lint_pass, declare_tool_lint};
12-
use rustc_span::{sym, symbol};
13-
use std::borrow::Cow;
12+
use rustc_span::{sym, symbol::kw};
1413

1514
declare_clippy_lint! {
1615
/// ### What it does
@@ -61,31 +60,39 @@ declare_clippy_lint! {
6160
/// wrapping the result of `cmp` in `Some` for `partial_cmp`. Not doing this may silently
6261
/// introduce an error upon refactoring.
6362
///
63+
/// ### Limitations
64+
/// Will not lint if `Self` and `Rhs` do not have the same type.
65+
///
6466
/// ### Example
65-
/// ```rust,ignore
67+
/// ```rust
68+
/// # use std::cmp::Ordering;
6669
/// #[derive(Eq, PartialEq)]
6770
/// struct A(u32);
6871
///
6972
/// impl Ord for A {
7073
/// fn cmp(&self, other: &Self) -> Ordering {
71-
/// todo!();
74+
/// // ...
75+
/// # todo!();
7276
/// }
7377
/// }
7478
///
7579
/// impl PartialOrd for A {
7680
/// fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
77-
/// todo!();
81+
/// // ...
82+
/// # todo!();
7883
/// }
7984
/// }
8085
/// ```
8186
/// Use instead:
82-
/// ```rust,ignore
87+
/// ```rust
88+
/// # use std::cmp::Ordering;
8389
/// #[derive(Eq, PartialEq)]
8490
/// struct A(u32);
8591
///
8692
/// impl Ord for A {
8793
/// fn cmp(&self, other: &Self) -> Ordering {
88-
/// todo!();
94+
/// // ...
95+
/// # todo!();
8996
/// }
9097
/// }
9198
///
@@ -105,17 +112,18 @@ declare_lint_pass!(IncorrectImpls => [INCORRECT_CLONE_IMPL_ON_COPY_TYPE, INCORRE
105112
impl LateLintPass<'_> for IncorrectImpls {
106113
#[expect(clippy::too_many_lines)]
107114
fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
108-
let node = get_parent_node(cx.tcx, impl_item.hir_id());
109-
let Some(Node::Item(item)) = node else {
115+
let Some(Node::Item(item)) = get_parent_node(cx.tcx, impl_item.hir_id()) else {
110116
return;
111117
};
112118
let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else {
113119
return;
114120
};
115-
let trait_impl_def_id = trait_impl.def_id;
116121
if cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) {
117122
return;
118123
}
124+
let ItemKind::Impl(imp) = item.kind else {
125+
return;
126+
};
119127
let ImplItemKind::Fn(_, impl_item_id) = cx.tcx.hir().impl_item(impl_item.impl_item_id()).kind else {
120128
return;
121129
};
@@ -124,7 +132,7 @@ impl LateLintPass<'_> for IncorrectImpls {
124132
return;
125133
};
126134

127-
if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl_def_id)
135+
if cx.tcx.is_diagnostic_item(sym::Clone, trait_impl.def_id)
128136
&& let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy)
129137
&& implements_trait(
130138
cx,
@@ -136,9 +144,9 @@ impl LateLintPass<'_> for IncorrectImpls {
136144
if impl_item.ident.name == sym::clone {
137145
if block.stmts.is_empty()
138146
&& let Some(expr) = block.expr
139-
&& let ExprKind::Unary(UnOp::Deref, inner) = expr.kind
140-
&& let ExprKind::Path(qpath) = inner.kind
141-
&& last_path_segment(&qpath).ident.name == symbol::kw::SelfLower
147+
&& let ExprKind::Unary(UnOp::Deref, deref) = expr.kind
148+
&& let ExprKind::Path(qpath) = deref.kind
149+
&& last_path_segment(&qpath).ident.name == kw::SelfLower
142150
{} else {
143151
span_lint_and_sugg(
144152
cx,
@@ -160,7 +168,7 @@ impl LateLintPass<'_> for IncorrectImpls {
160168
INCORRECT_CLONE_IMPL_ON_COPY_TYPE,
161169
impl_item.span,
162170
"incorrect implementation of `clone_from` on a `Copy` type",
163-
"remove this",
171+
"remove it",
164172
String::new(),
165173
Applicability::MaybeIncorrect,
166174
);
@@ -169,7 +177,7 @@ impl LateLintPass<'_> for IncorrectImpls {
169177
}
170178
}
171179

172-
if cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl_def_id)
180+
if cx.tcx.is_diagnostic_item(sym::PartialOrd, trait_impl.def_id)
173181
&& impl_item.ident.name == sym::partial_cmp
174182
&& let Some(ord_def_id) = cx
175183
.tcx
@@ -198,12 +206,9 @@ impl LateLintPass<'_> for IncorrectImpls {
198206
&& cmp_path.ident.name == sym::cmp
199207
&& let Res::Local(..) = path_res(cx, other_expr)
200208
{} else {
201-
// If lhs and rhs are not the same type, bail. This makes creating a valid
209+
// If `Self` and `Rhs` are not the same type, bail. This makes creating a valid
202210
// suggestion tons more complex.
203-
if let Some(lhs) = trait_impl.substs.get(0)
204-
&& let Some(rhs) = trait_impl.substs.get(1)
205-
&& lhs != rhs
206-
{
211+
if let [lhs, rhs, ..] = trait_impl.substs.as_slice() && lhs != rhs {
207212
return;
208213
}
209214

@@ -213,22 +218,23 @@ impl LateLintPass<'_> for IncorrectImpls {
213218
item.span,
214219
"incorrect implementation of `partial_cmp` on an `Ord` type",
215220
|diag| {
216-
let (help, app) = if let Some(other) = body.params.get(1)
217-
&& let PatKind::Binding(_, _, other_ident, ..) = other.pat.kind
218-
{
219-
(
220-
Cow::Owned(format!("{{ Some(self.cmp({})) }}", other_ident.name)),
221-
Applicability::Unspecified,
222-
)
221+
let [_, other] = body.params else {
222+
return;
223+
};
224+
225+
let suggs = if let Some(other_ident) = other.pat.simple_ident() {
226+
vec![(block.span, format!("{{ Some(self.cmp({})) }}", other_ident.name))]
223227
} else {
224-
(Cow::Borrowed("{ Some(self.cmp(...)) }"), Applicability::HasPlaceholders)
228+
vec![
229+
(block.span, "{ Some(self.cmp(other)) }".to_owned()),
230+
(other.pat.span, "other".to_owned()),
231+
]
225232
};
226233

227-
diag.span_suggestion(
228-
block.span,
234+
diag.multipart_suggestion(
229235
"change this to",
230-
help,
231-
app,
236+
suggs,
237+
Applicability::Unspecified,
232238
);
233239
}
234240
);

‎tests/ui/incorrect_clone_impl_on_copy_type.stderr‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ LL | / fn clone_from(&mut self, source: &Self) {
1616
LL | | source.clone();
1717
LL | | *self = source.clone();
1818
LL | | }
19-
| |_____^ help: remove this
19+
| |_____^ help: remove it
2020

2121
error: incorrect implementation of `clone` on a `Copy` type
2222
--> $DIR/incorrect_clone_impl_on_copy_type.rs:81:29
@@ -34,7 +34,7 @@ LL | / fn clone_from(&mut self, source: &Self) {
3434
LL | | source.clone();
3535
LL | | *self = source.clone();
3636
LL | | }
37-
| |_____^ help: remove this
37+
| |_____^ help: remove it
3838

3939
error: aborting due to 4 previous errors
4040

‎tests/ui/incorrect_partial_ord_impl_on_ord_type.fixed‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
//@run-rustfix
2+
#![allow(unused)]
3+
#![no_main]
4+
5+
use std::cmp::Ordering;
6+
7+
// lint
8+
9+
#[derive(Eq, PartialEq)]
10+
struct A(u32);
11+
12+
impl Ord for A {
13+
fn cmp(&self, other: &Self) -> Ordering {
14+
todo!();
15+
}
16+
}
17+
18+
impl PartialOrd for A {
19+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
20+
}
21+
22+
// do not lint
23+
24+
#[derive(Eq, PartialEq)]
25+
struct B(u32);
26+
27+
impl Ord for B {
28+
fn cmp(&self, other: &Self) -> Ordering {
29+
todo!();
30+
}
31+
}
32+
33+
impl PartialOrd for B {
34+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
35+
Some(self.cmp(other))
36+
}
37+
}
38+
39+
// lint, and give `_` a name
40+
41+
#[derive(Eq, PartialEq)]
42+
struct C(u32);
43+
44+
impl Ord for C {
45+
fn cmp(&self, other: &Self) -> Ordering {
46+
todo!();
47+
}
48+
}
49+
50+
impl PartialOrd for C {
51+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
52+
}
53+
54+
// do not lint derived
55+
56+
#[derive(Eq, Ord, PartialEq, PartialOrd)]
57+
struct D(u32);
58+
59+
// do not lint if ord is not manually implemented
60+
61+
#[derive(Eq, PartialEq)]
62+
struct E(u32);
63+
64+
impl PartialOrd for E {
65+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
66+
todo!();
67+
}
68+
}
69+
70+
// do not lint since ord has more restrictive bounds
71+
72+
#[derive(Eq, PartialEq)]
73+
struct Uwu<A>(A);
74+
75+
impl<A: std::fmt::Debug + Ord + PartialOrd> Ord for Uwu<A> {
76+
fn cmp(&self, other: &Self) -> Ordering {
77+
todo!();
78+
}
79+
}
80+
81+
impl<A: Ord + PartialOrd> PartialOrd for Uwu<A> {
82+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
83+
todo!();
84+
}
85+
}
86+
87+
// do not lint since `Rhs` is not `Self`
88+
89+
#[derive(Eq, PartialEq)]
90+
struct F(u32);
91+
92+
impl Ord for F {
93+
fn cmp(&self, other: &Self) -> Ordering {
94+
todo!();
95+
}
96+
}
97+
98+
impl PartialOrd for F {
99+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
100+
Some(self.cmp(other))
101+
}
102+
}
103+
104+
impl PartialEq<u32> for F {
105+
fn eq(&self, other: &u32) -> bool {
106+
todo!();
107+
}
108+
}
109+
110+
impl PartialOrd<u32> for F {
111+
fn partial_cmp(&self, other: &u32) -> Option<Ordering> {
112+
todo!();
113+
}
114+
}

‎tests/ui/incorrect_partial_ord_impl_on_ord_type.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
//@run-rustfix
12
#![allow(unused)]
23
#![no_main]
34

@@ -37,7 +38,7 @@ impl PartialOrd for B {
3738
}
3839
}
3940

40-
// lint, but we can't give a suggestion since &Self is not named
41+
// lint, and give `_` a name
4142

4243
#[derive(Eq, PartialEq)]
4344
struct C(u32);
@@ -50,7 +51,7 @@ impl Ord for C {
5051

5152
impl PartialOrd for C {
5253
fn partial_cmp(&self, _: &Self) -> Option<Ordering> {
53-
todo!(); // don't run rustfix, or else this will cause it to fail to compile
54+
todo!();
5455
}
5556
}
5657

‎tests/ui/incorrect_partial_ord_impl_on_ord_type.stderr‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: incorrect implementation of `partial_cmp` on an `Ord` type
2-
--> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:17:1
2+
--> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:18:1
33
|
44
LL | / impl PartialOrd for A {
55
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
@@ -13,16 +13,19 @@ LL | | }
1313
= note: `#[deny(clippy::incorrect_partial_ord_impl_on_ord_type)]` on by default
1414

1515
error: incorrect implementation of `partial_cmp` on an `Ord` type
16-
--> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:51:1
16+
--> $DIR/incorrect_partial_ord_impl_on_ord_type.rs:52:1
1717
|
18-
LL | / impl PartialOrd for C {
19-
LL | | fn partial_cmp(&self, _: &Self) -> Option<Ordering> {
20-
| | _________________________________________________________-
21-
LL | || todo!(); // don't run rustfix, or else this will cause it to fail to compile
22-
LL | || }
23-
| ||_____- help: change this to: `{ Some(self.cmp(...)) }`
24-
LL | | }
25-
| |__^
18+
LL | / impl PartialOrd for C {
19+
LL | | fn partial_cmp(&self, _: &Self) -> Option<Ordering> {
20+
LL | | todo!();
21+
LL | | }
22+
LL | | }
23+
| |_^
24+
|
25+
help: change this to
26+
|
27+
LL | fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
28+
| ~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~
2629

2730
error: aborting due to 2 previous errors
2831

0 commit comments

Comments
 (0)