Skip to content

Commit 955a25e

Browse files
JarredAllenJarredAllen
committed
Added negative test cases and ran cargo dev fmt
1 parent 943cb94 commit 955a25e

File tree

4 files changed

+100
-45
lines changed

4 files changed

+100
-45
lines changed

‎clippy_lints/src/sort_by_key_reverse.rs

Lines changed: 83 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use crate::utils::paths;
33
use crate::utils::sugg::Sugg;
44
use if_chain::if_chain;
55
use rustc_errors::Applicability;
6-
use rustc_hir::*;
6+
use rustc_hir::{Expr, ExprKind, Mutability, Param, Pat, PatKind, Path, QPath};
77
use rustc_lint::{LateContext, LateLintPass};
88
use rustc_session::{declare_lint_pass, declare_tool_lint};
99
use rustc_span::symbol::Ident;
@@ -43,64 +43,105 @@ struct LintTrigger {
4343

4444
/// Detect if the two expressions are mirrored (identical, except one
4545
/// contains a and the other replaces it with b)
46-
fn mirrored_exprs(cx: &LateContext<'_, '_>, a_expr: &Expr<'_>, a_ident: &Ident, b_expr: &Expr<'_>, b_ident: &Ident) -> bool {
46+
fn mirrored_exprs(
47+
cx: &LateContext<'_, '_>,
48+
a_expr: &Expr<'_>,
49+
a_ident: &Ident,
50+
b_expr: &Expr<'_>,
51+
b_ident: &Ident,
52+
) -> bool {
4753
match (&a_expr.kind, &b_expr.kind) {
4854
// Two boxes with mirrored contents
49-
(ExprKind::Box(left_expr), ExprKind::Box(right_expr)) => mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
55+
(ExprKind::Box(left_expr), ExprKind::Box(right_expr)) => {
56+
mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
57+
},
5058
// Two arrays with mirrored contents
51-
(ExprKind::Array(left_exprs), ExprKind::Array(right_exprs))
52-
=> left_exprs.iter().zip(right_exprs.iter()).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
59+
(ExprKind::Array(left_exprs), ExprKind::Array(right_exprs)) => left_exprs
60+
.iter()
61+
.zip(right_exprs.iter())
62+
.all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
5363
// The two exprs are function calls.
5464
// Check to see that the function itself and its arguments are mirrored
55-
(ExprKind::Call(left_expr, left_args), ExprKind::Call(right_expr, right_args))
56-
=> {
57-
// println!("{:?}\n{:?}\n", left_expr, left_args);
58-
// println!("{:?}\n{:?}\n", right_expr, right_args);
59-
mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
60-
&& left_args.iter().zip(right_args.iter()).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
61-
},
65+
(ExprKind::Call(left_expr, left_args), ExprKind::Call(right_expr, right_args)) => {
66+
mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
67+
&& left_args
68+
.iter()
69+
.zip(right_args.iter())
70+
.all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
71+
},
6272
// The two exprs are method calls.
6373
// Check to see that the function is the same and the arguments are mirrored
6474
// This is enough because the receiver of the method is listed in the arguments
65-
(ExprKind::MethodCall(left_segment, _, left_args), ExprKind::MethodCall(right_segment, _, right_args))
66-
=> left_segment.ident == right_segment.ident
67-
&& left_args.iter().zip(right_args.iter()).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
75+
(ExprKind::MethodCall(left_segment, _, left_args), ExprKind::MethodCall(right_segment, _, right_args)) => {
76+
left_segment.ident == right_segment.ident
77+
&& left_args
78+
.iter()
79+
.zip(right_args.iter())
80+
.all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
81+
},
6882
// Two tuples with mirrored contents
69-
(ExprKind::Tup(left_exprs), ExprKind::Tup(right_exprs))
70-
=> left_exprs.iter().zip(right_exprs.iter()).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
83+
(ExprKind::Tup(left_exprs), ExprKind::Tup(right_exprs)) => left_exprs
84+
.iter()
85+
.zip(right_exprs.iter())
86+
.all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
7187
// Two binary ops, which are the same operation and which have mirrored arguments
72-
(ExprKind::Binary(left_op, left_left, left_right), ExprKind::Binary(right_op, right_left, right_right))
73-
=> left_op.node == right_op.node
88+
(ExprKind::Binary(left_op, left_left, left_right), ExprKind::Binary(right_op, right_left, right_right)) => {
89+
left_op.node == right_op.node
7490
&& mirrored_exprs(cx, left_left, a_ident, right_left, b_ident)
75-
&& mirrored_exprs(cx, left_right, a_ident, right_right, b_ident),
91+
&& mirrored_exprs(cx, left_right, a_ident, right_right, b_ident)
92+
},
7693
// Two unary ops, which are the same operation and which have the same argument
77-
(ExprKind::Unary(left_op, left_expr), ExprKind::Unary(right_op, right_expr))
78-
=> left_op == right_op && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
94+
(ExprKind::Unary(left_op, left_expr), ExprKind::Unary(right_op, right_expr)) => {
95+
left_op == right_op && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
96+
},
7997
// The two exprs are literals of some kind
8098
(ExprKind::Lit(left_lit), ExprKind::Lit(right_lit)) => left_lit.node == right_lit.node,
81-
(ExprKind::Cast(left_expr, left_ty), ExprKind::Cast(right_expr, right_ty))
82-
=> mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
83-
(ExprKind::DropTemps(left), ExprKind::DropTemps(right)) => mirrored_exprs(cx, left, a_ident, right, b_ident),
84-
(ExprKind::Field(left_expr, left_ident), ExprKind::Field(right_expr, right_ident))
85-
=> left_ident.name == right_ident.name && mirrored_exprs(cx, left_expr, a_ident, right_expr, right_ident),
99+
(ExprKind::Cast(left, _), ExprKind::Cast(right, _)) => mirrored_exprs(cx, left, a_ident, right, b_ident),
100+
(ExprKind::DropTemps(left_block), ExprKind::DropTemps(right_block)) => {
101+
mirrored_exprs(cx, left_block, a_ident, right_block, b_ident)
102+
},
103+
(ExprKind::Field(left_expr, left_ident), ExprKind::Field(right_expr, right_ident)) => {
104+
left_ident.name == right_ident.name && mirrored_exprs(cx, left_expr, a_ident, right_expr, right_ident)
105+
},
86106
// Two paths: either one is a and the other is b, or they're identical to each other
87-
(ExprKind::Path(QPath::Resolved(_, Path { segments: left_segments, .. })),
88-
ExprKind::Path(QPath::Resolved(_, Path { segments: right_segments, .. })))
89-
=> (left_segments.iter().zip(right_segments.iter()).all(|(left, right)| left.ident == right.ident)
90-
&& left_segments.iter().all(|seg| &seg.ident != a_ident && &seg.ident != b_ident))
91-
|| (left_segments.len() == 1 && &left_segments[0].ident == a_ident && right_segments.len() == 1 && &right_segments[0].ident == b_ident),
107+
(
108+
ExprKind::Path(QPath::Resolved(
109+
_,
110+
Path {
111+
segments: left_segments,
112+
..
113+
},
114+
)),
115+
ExprKind::Path(QPath::Resolved(
116+
_,
117+
Path {
118+
segments: right_segments,
119+
..
120+
},
121+
)),
122+
) => {
123+
(left_segments
124+
.iter()
125+
.zip(right_segments.iter())
126+
.all(|(left, right)| left.ident == right.ident)
127+
&& left_segments
128+
.iter()
129+
.all(|seg| &seg.ident != a_ident && &seg.ident != b_ident))
130+
|| (left_segments.len() == 1
131+
&& &left_segments[0].ident == a_ident
132+
&& right_segments.len() == 1
133+
&& &right_segments[0].ident == b_ident)
134+
},
92135
// Matching expressions, but one or both is borrowed
93-
(ExprKind::AddrOf(left_kind, Mutability::Not, left_expr), ExprKind::AddrOf(right_kind, Mutability::Not, right_expr))
94-
=> left_kind == right_kind && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
95-
(_, ExprKind::AddrOf(_, Mutability::Not, right_expr))
96-
=> mirrored_exprs(cx, a_expr, a_ident, right_expr, b_ident),
97-
(ExprKind::AddrOf(_, Mutability::Not, left_expr), _)
98-
=> mirrored_exprs(cx, left_expr, a_ident, b_expr, b_ident),
136+
(
137+
ExprKind::AddrOf(left_kind, Mutability::Not, left_expr),
138+
ExprKind::AddrOf(right_kind, Mutability::Not, right_expr),
139+
) => left_kind == right_kind && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
140+
(_, ExprKind::AddrOf(_, Mutability::Not, right_expr)) => {
141+
mirrored_exprs(cx, a_expr, a_ident, right_expr, b_ident)
142+
},
143+
(ExprKind::AddrOf(_, Mutability::Not, left_expr), _) => mirrored_exprs(cx, left_expr, a_ident, b_expr, b_ident),
99144
_ => false,
100-
// (left, right) => {
101-
// println!("{:?}\n{:?}", left, right);
102-
// false
103-
// },
104145
}
105146
}
106147

‎tests/ui/sort_by_key_reverse.fixed

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,11 @@ fn main() {
1212
vec.sort_by_key(|&b| Reverse(b));
1313
vec.sort_by_key(|&b| Reverse((b + 5).abs()));
1414
vec.sort_by_key(|&b| Reverse(id(-b)));
15+
// Negative examples (shouldn't be changed)
16+
let c = &7;
17+
vec.sort_by(|a, b| (b - a).cmp(&(a - b)));
18+
vec.sort_by(|_, b| b.cmp(&5));
19+
vec.sort_by(|_, b| b.cmp(c));
20+
vec.sort_by(|a, _| a.cmp(c));
21+
vec.sort_by(|a, b| a.cmp(b));
1522
}

‎tests/ui/sort_by_key_reverse.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,13 @@ fn id(x: isize) -> isize {
1010
fn main() {
1111
let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
1212
vec.sort_by(|a, b| b.cmp(a));
13-
vec.sort_by(|a, b| (b + 5).abs().cmp(&(a+5).abs()));
13+
vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
1414
vec.sort_by(|a, b| id(-b).cmp(&id(-a)));
15+
// Negative examples (shouldn't be changed)
16+
let c = &7;
17+
vec.sort_by(|a, b| (b - a).cmp(&(a - b)));
18+
vec.sort_by(|_, b| b.cmp(&5));
19+
vec.sort_by(|_, b| b.cmp(c));
20+
vec.sort_by(|a, _| a.cmp(c));
21+
vec.sort_by(|a, b| a.cmp(b));
1522
}

‎tests/ui/sort_by_key_reverse.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ LL | vec.sort_by(|a, b| b.cmp(a));
99
error: use Vec::sort_by_key here instead
1010
--> $DIR/sort_by_key_reverse.rs:13:5
1111
|
12-
LL | vec.sort_by(|a, b| (b + 5).abs().cmp(&(a+5).abs()));
13-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse((b + 5).abs()))`
12+
LL | vec.sort_by(|a, b| (b + 5).abs().cmp(&(a + 5).abs()));
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|&b| Reverse((b + 5).abs()))`
1414

1515
error: use Vec::sort_by_key here instead
1616
--> $DIR/sort_by_key_reverse.rs:14:5

0 commit comments

Comments
 (0)