Skip to content

Commit 8590ab4

Browse files
JarredAllenJarredAllen
committed
More progress towards sort_by_key_reverse lint
1 parent 24847ea commit 8590ab4

File tree

5 files changed

+149
-19
lines changed

5 files changed

+149
-19
lines changed

‎clippy_lints/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -998,6 +998,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
998998
store.register_late_pass(|| box ptr_offset_with_cast::PtrOffsetWithCast);
999999
store.register_late_pass(|| box redundant_clone::RedundantClone);
10001000
store.register_late_pass(|| box slow_vector_initialization::SlowVectorInit);
1001+
store.register_late_pass(|| box sort_by_key_reverse::SortByKeyReverse);
10011002
store.register_late_pass(|| box types::RefToMut);
10021003
store.register_late_pass(|| box assertions_on_constants::AssertionsOnConstants);
10031004
store.register_late_pass(|| box missing_const_for_fn::MissingConstForFn);

‎clippy_lints/src/sort_by_key_reverse.rs

Lines changed: 113 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
1-
use crate::utils::{match_type, span_lint_and_sugg};
1+
use crate::utils;
22
use crate::utils::paths;
33
use crate::utils::sugg::Sugg;
44
use if_chain::if_chain;
55
use rustc_errors::Applicability;
6-
use rustc_lint::{LateLintPass, LateContext};
7-
use rustc_session::{declare_lint_pass, declare_tool_lint};
86
use rustc_hir::*;
7+
use rustc_lint::{LateContext, LateLintPass};
8+
use rustc_session::{declare_lint_pass, declare_tool_lint};
9+
use rustc_span::symbol::Ident;
910

1011
declare_clippy_lint! {
1112
/// **What it does:**
@@ -40,18 +41,122 @@ struct LintTrigger {
4041
unstable: bool,
4142
}
4243

44+
/// Detect if the two expressions are mirrored (identical, except one
45+
/// 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 {
47+
match (&a_expr.kind, &b_expr.kind) {
48+
// 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),
50+
// 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)),
53+
// The two exprs are function calls.
54+
// 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+
=> mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
57+
&& left_args.iter().zip(right_args.iter()).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
58+
// The two exprs are method calls.
59+
// Check to see that the function is the same and the arguments are mirrored
60+
// This is enough because the receiver of the method is listed in the arguments
61+
(ExprKind::MethodCall(left_segment, _, left_args), ExprKind::MethodCall(right_segment, _, right_args))
62+
=> left_segment.ident == right_segment.ident
63+
&& left_args.iter().zip(right_args.iter()).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
64+
// Two tuples with mirrored contents
65+
(ExprKind::Tup(left_exprs), ExprKind::Tup(right_exprs))
66+
=> left_exprs.iter().zip(right_exprs.iter()).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)),
67+
// Two binary ops, which are the same operation and which have mirrored arguments
68+
(ExprKind::Binary(left_op, left_left, left_right), ExprKind::Binary(right_op, right_left, right_right))
69+
=> left_op.node == right_op.node
70+
&& mirrored_exprs(cx, left_left, a_ident, right_left, b_ident)
71+
&& mirrored_exprs(cx, left_right, a_ident, right_right, b_ident),
72+
// Two unary ops, which are the same operation and which have the same argument
73+
(ExprKind::Unary(left_op, left_expr), ExprKind::Unary(right_op, right_expr))
74+
=> left_op == right_op && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
75+
// The two exprs are literals of some kind
76+
(ExprKind::Lit(left_lit), ExprKind::Lit(right_lit)) => left_lit.node == right_lit.node,
77+
(ExprKind::Cast(left_expr, _), ExprKind::Cast(right_expr, _))
78+
=> mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
79+
(ExprKind::DropTemps(left), ExprKind::DropTemps(right)) => mirrored_exprs(cx, left, a_ident, right, b_ident),
80+
(ExprKind::Block(left, _), ExprKind::Block(right, _)) => mirrored_blocks(cx, left, a_ident, right, b_ident),
81+
(ExprKind::Field(left_expr, left_ident), ExprKind::Field(right_expr, right_ident))
82+
=> left_ident.name == right_ident.name && mirrored_exprs(cx, left_expr, a_ident, right_expr, right_ident),
83+
// The two exprs are `a` and `b`, directly
84+
(ExprKind::Path(QPath::Resolved(_, Path { segments: &[PathSegment { ident: left_ident, .. }], .. },)),
85+
ExprKind::Path(QPath::Resolved(_, Path { segments: &[PathSegment { ident: right_ident, .. }], .. },)),
86+
) => &left_ident == a_ident && &right_ident == b_ident,
87+
// The two exprs are Paths to the same name (which is neither a nor b)
88+
(ExprKind::Path(QPath::Resolved(_, Path { segments: left_segments, .. })),
89+
ExprKind::Path(QPath::Resolved(_, Path { segments: right_segments, .. })))
90+
=> left_segments.iter().zip(right_segments.iter()).all(|(left, right)| left.ident == right.ident)
91+
&& left_segments.iter().all(|seg| &seg.ident != a_ident && &seg.ident != b_ident),
92+
// 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),
99+
// _ => false,
100+
(left, right) => {
101+
println!("{:?}\n{:?}", left, right);
102+
false
103+
},
104+
}
105+
}
106+
107+
/// Detect if the two blocks are mirrored (identical, except one
108+
/// contains a and the other replaces it with b)
109+
fn mirrored_blocks(cx: &LateContext<'_, '_>, a_block: &Block<'_>, a_ident: &Ident, b_block: &Block<'_>, b_ident: &Ident) -> bool {
110+
match (a_block, b_block) {
111+
(Block { stmts: left_stmts, expr: left_expr, .. },
112+
Block { stmts: right_stmts, expr: right_expr, .. })
113+
=> left_stmts.iter().zip(right_stmts.iter()).all(|(left, right)| match (&left.kind, &right.kind) {
114+
(StmtKind::Expr(left_expr), StmtKind::Expr(right_expr)) => mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
115+
(StmtKind::Semi(left_expr), StmtKind::Semi(right_expr)) => mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
116+
(StmtKind::Item(left_item), StmtKind::Item(right_item)) => left_item.id == right_item.id,
117+
(StmtKind::Local(left), StmtKind::Local(right)) => mirrored_locals(cx, left, a_ident, right, b_ident),
118+
_ => false,
119+
}) && match (left_expr, right_expr) {
120+
(None, None) => true,
121+
(Some(left), Some(right)) => mirrored_exprs(cx, left, a_ident, right, b_ident),
122+
_ => false,
123+
},
124+
}
125+
}
126+
127+
/// Check that the two "Local"s (let statements) are equal
128+
fn mirrored_locals(cx: &LateContext<'_, '_>, a_local: &Local<'_>, a_ident: &Ident, b_local: &Local<'_>, b_ident: &Ident) -> bool {
129+
match (a_local, b_local) {
130+
(Local { pat: left_pat, init: left_expr, .. }, Local { pat: right_pat, init: right_expr, .. })
131+
=> match (left_expr, right_expr) {
132+
(None, None) => true,
133+
(Some(left), Some(right)) => mirrored_exprs(cx, left, a_ident, right, b_ident),
134+
_ => false,
135+
},
136+
}
137+
}
138+
43139
fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<LintTrigger> {
44140
if_chain! {
45141
if let ExprKind::MethodCall(name_ident, _, args) = &expr.kind;
46142
if let name = name_ident.ident.name.to_ident_string();
47143
if name == "sort_by" || name == "sort_unstable_by";
48-
if let [vec, Expr { kind: ExprKind::Closure(_, closure_decl, closure_body_id, _, _), .. }] = args;
49-
if closure_decl.inputs.len() == 2;
50-
if match_type(cx, &cx.tables.expr_ty(vec), &paths::VEC);
144+
if let [vec, Expr { kind: ExprKind::Closure(_, _, closure_body_id, _, _), .. }] = args;
145+
if utils::match_type(cx, &cx.tables.expr_ty(vec), &paths::VEC);
146+
if let closure_body = cx.tcx.hir().body(*closure_body_id);
147+
if let &[
148+
Param { pat: Pat { kind: PatKind::Binding(_, _, a_ident, _), .. }, ..},
149+
Param { pat: Pat { kind: PatKind::Binding(_, _, b_ident, _), .. }, .. }
150+
] = &closure_body.params;
151+
if let ExprKind::MethodCall(method_path, _, [ref b_expr, ref a_expr]) = &closure_body.value.kind;
152+
if method_path.ident.name.to_ident_string() == "cmp";
153+
if mirrored_exprs(&cx, &a_expr, &a_ident, &b_expr, &b_ident);
51154
then {
52155
let vec_name = Sugg::hir(cx, &args[0], "..").to_string();
53156
let unstable = name == "sort_unstable_by";
54-
Some(LintTrigger { vec_name, unstable, closure_arg: "e".to_string(), closure_reverse_body: "e".to_string() })
157+
let closure_arg = a_ident.name.to_ident_string();
158+
let closure_reverse_body = Sugg::hir(cx, &a_expr, "..").to_string();
159+
Some(LintTrigger { vec_name, unstable, closure_arg, closure_reverse_body })
55160
} else {
56161
None
57162
}
@@ -60,18 +165,8 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<LintTrigger>
60165

61166
impl LateLintPass<'_, '_> for SortByKeyReverse {
62167
fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
63-
println!("{:?}", expr);
64-
span_lint_and_sugg(
65-
cx,
66-
SORT_BY_KEY_REVERSE,
67-
expr.span,
68-
"use Vec::sort_by_key here instead",
69-
"try",
70-
String::from("being a better person"),
71-
Applicability::MachineApplicable,
72-
);
73168
if let Some(trigger) = detect_lint(cx, expr) {
74-
span_lint_and_sugg(
169+
utils::span_lint_and_sugg(
75170
cx,
76171
SORT_BY_KEY_REVERSE,
77172
expr.span,

‎tests/ui/sort_by_key_reverse.fixed

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// run-rustfix
2+
#![warn(clippy::sort_by_key_reverse)]
3+
4+
fn main() {
5+
let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
6+
vec.sort_by_key(|a| Reverse(a));
7+
vec.sort_by_key(|a| Reverse(&(a+5).abs()));
8+
vec.sort_by_key(|a| Reverse(&-a));
9+
}

‎tests/ui/sort_by_key_reverse.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1+
// run-rustfix
12
#![warn(clippy::sort_by_key_reverse)]
23

34
fn main() {
4-
let mut vec = vec![3, 6, 1, 2, 5];
5+
let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
56
vec.sort_by(|a, b| b.cmp(a));
7+
vec.sort_by(|a, b| (b + 5).abs().cmp(&(a+5).abs()));
8+
vec.sort_by(|a, b| (-b).cmp(&-a));
69
}

‎tests/ui/sort_by_key_reverse.stderr

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
error: use Vec::sort_by_key here instead
2+
--> $DIR/sort_by_key_reverse.rs:6:5
3+
|
4+
LL | vec.sort_by(|a, b| b.cmp(a));
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|a| Reverse(a))`
6+
|
7+
= note: `-D clippy::sort-by-key-reverse` implied by `-D warnings`
8+
9+
error: use Vec::sort_by_key here instead
10+
--> $DIR/sort_by_key_reverse.rs:7:5
11+
|
12+
LL | vec.sort_by(|a, b| (b + 5).abs().cmp(&(a+5).abs()));
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|a| Reverse(&(a+5).abs()))`
14+
15+
error: use Vec::sort_by_key here instead
16+
--> $DIR/sort_by_key_reverse.rs:8:5
17+
|
18+
LL | vec.sort_by(|a, b| (-b).cmp(&-a));
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `vec.sort_by_key(|a| Reverse(&-a))`
20+
21+
error: aborting due to 3 previous errors
22+

0 commit comments

Comments
 (0)