Skip to content

Commit ab9eb10

Browse files
llogiqllogiq
authored
feat: manual_rotate also recognize non-consts (rust-lang#15402)
changelog: [`manual_rotate`]: also recognize non-consts fixes rust-lang#13028 r? @llogiq
2 parents 8116b23 + b249f13 commit ab9eb10

File tree

4 files changed

+129
-47
lines changed

4 files changed

+129
-47
lines changed

‎clippy_lints/src/manual_rotate.rs‎

Lines changed: 51 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -56,20 +56,14 @@ impl Display for ShiftDirection {
5656
}
5757
}
5858

59-
fn parse_shift<'tcx>(
60-
cx: &LateContext<'tcx>,
61-
expr: &'tcx Expr<'tcx>,
62-
) -> Option<(ShiftDirection, u128, &'tcx Expr<'tcx>)> {
59+
fn parse_shift<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<(ShiftDirection, &'tcx Expr<'tcx>, &'tcx Expr<'tcx>)> {
6360
if let ExprKind::Binary(op, l, r) = expr.kind {
6461
let dir = match op.node {
6562
BinOpKind::Shl => ShiftDirection::Left,
6663
BinOpKind::Shr => ShiftDirection::Right,
6764
_ => return None,
6865
};
69-
let const_expr = ConstEvalCtxt::new(cx).eval_local(r, expr.span.ctxt())?;
70-
if let Constant::Int(shift) = const_expr {
71-
return Some((dir, shift, l));
72-
}
66+
return Some((dir, l, r));
7367
}
7468
None
7569
}
@@ -78,40 +72,62 @@ impl LateLintPass<'_> for ManualRotate {
7872
fn check_expr<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
7973
if let ExprKind::Binary(op, l, r) = expr.kind
8074
&& let BinOpKind::Add | BinOpKind::BitOr = op.node
81-
&& let Some((l_shift_dir, l_amount, l_expr)) = parse_shift(cx, l)
82-
&& let Some((r_shift_dir, r_amount, r_expr)) = parse_shift(cx, r)
83-
{
84-
if l_shift_dir == r_shift_dir {
85-
return;
86-
}
87-
if !clippy_utils::eq_expr_value(cx, l_expr, r_expr) {
88-
return;
89-
}
90-
let Some(bit_width) = (match cx.typeck_results().expr_ty(expr).kind() {
75+
&& let Some((l_shift_dir, l_expr, l_amount)) = parse_shift(l)
76+
&& let Some((r_shift_dir, r_expr, r_amount)) = parse_shift(r)
77+
&& l_shift_dir != r_shift_dir
78+
&& clippy_utils::eq_expr_value(cx, l_expr, r_expr)
79+
&& let Some(bit_width) = match cx.typeck_results().expr_ty(expr).kind() {
9180
ty::Int(itype) => itype.bit_width(),
9281
ty::Uint(itype) => itype.bit_width(),
9382
_ => return,
94-
}) else {
95-
return;
96-
};
97-
if l_amount + r_amount == u128::from(bit_width) {
98-
let (shift_function, amount) = if l_amount < r_amount {
83+
}
84+
{
85+
let const_eval = ConstEvalCtxt::new(cx);
86+
87+
let ctxt = expr.span.ctxt();
88+
let (shift_function, amount) = if let Some(Constant::Int(l_amount_val)) =
89+
const_eval.eval_local(l_amount, ctxt)
90+
&& let Some(Constant::Int(r_amount_val)) = const_eval.eval_local(r_amount, ctxt)
91+
&& l_amount_val + r_amount_val == u128::from(bit_width)
92+
{
93+
if l_amount_val < r_amount_val {
9994
(l_shift_dir, l_amount)
10095
} else {
10196
(r_shift_dir, r_amount)
97+
}
98+
} else {
99+
let (amount1, binop, minuend, amount2, shift_direction) = match (l_amount.kind, r_amount.kind) {
100+
(_, ExprKind::Binary(binop, minuend, other)) => (l_amount, binop, minuend, other, l_shift_dir),
101+
(ExprKind::Binary(binop, minuend, other), _) => (r_amount, binop, minuend, other, r_shift_dir),
102+
_ => return,
102103
};
103-
let mut applicability = Applicability::MachineApplicable;
104-
let expr_sugg = sugg::Sugg::hir_with_applicability(cx, l_expr, "_", &mut applicability).maybe_paren();
105-
span_lint_and_sugg(
106-
cx,
107-
MANUAL_ROTATE,
108-
expr.span,
109-
"there is no need to manually implement bit rotation",
110-
"this expression can be rewritten as",
111-
format!("{expr_sugg}.{shift_function}({amount})"),
112-
Applicability::MachineApplicable,
113-
);
114-
}
104+
105+
if let Some(Constant::Int(minuend)) = const_eval.eval_local(minuend, ctxt)
106+
&& clippy_utils::eq_expr_value(cx, amount1, amount2)
107+
// (x << s) | (x >> bit_width - s)
108+
&& ((binop.node == BinOpKind::Sub && u128::from(bit_width) == minuend)
109+
// (x << s) | (x >> (bit_width - 1) ^ s)
110+
|| (binop.node == BinOpKind::BitXor && u128::from(bit_width).checked_sub(minuend) == Some(1)))
111+
{
112+
// NOTE: we take these from the side that _doesn't_ have the binop, since it's probably simpler
113+
(shift_direction, amount1)
114+
} else {
115+
return;
116+
}
117+
};
118+
119+
let mut applicability = Applicability::MachineApplicable;
120+
let expr_sugg = sugg::Sugg::hir_with_applicability(cx, l_expr, "_", &mut applicability).maybe_paren();
121+
let amount = sugg::Sugg::hir_with_applicability(cx, amount, "_", &mut applicability);
122+
span_lint_and_sugg(
123+
cx,
124+
MANUAL_ROTATE,
125+
expr.span,
126+
"there is no need to manually implement bit rotation",
127+
"this expression can be rewritten as",
128+
format!("{expr_sugg}.{shift_function}({amount})"),
129+
Applicability::MachineApplicable,
130+
);
115131
}
116132
}
117133
}

‎tests/ui/manual_rotate.fixed‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ fn main() {
44
let (x_u8, x_u16, x_u32, x_u64) = (1u8, 1u16, 1u32, 1u64);
55
let (x_i8, x_i16, x_i32, x_i64) = (1i8, 1i16, 1i32, 1i64);
66
let a_u32 = 1u32;
7+
const N: u32 = 5;
78
// True positives
89
let y_u8 = x_u8.rotate_right(3);
910
//~^ manual_rotate
@@ -29,6 +30,9 @@ fn main() {
2930
//~^ manual_rotate
3031
let y_u64_as = (x_u32 as u64).rotate_right(8);
3132
//~^ manual_rotate
33+
// shift by a const
34+
let _ = x_i64.rotate_right(N);
35+
//~^ manual_rotate
3236

3337
// False positives - can't be replaced with a rotation
3438
let y_u8_false = (x_u8 >> 6) | (x_u8 << 3);
@@ -40,3 +44,20 @@ fn main() {
4044
let mut l = vec![12_u8, 34];
4145
let y = (l.pop().unwrap() << 3) + (l.pop().unwrap() >> 5);
4246
}
47+
48+
fn issue13028() {
49+
let s = 5;
50+
let u = 5;
51+
let x: u32 = 123456;
52+
53+
let _ = x.rotate_left(s);
54+
//~^ manual_rotate
55+
let _ = x.rotate_left(s);
56+
//~^ manual_rotate
57+
// still works with consts
58+
let _ = x.rotate_right(9);
59+
//~^ manual_rotate
60+
61+
// don't lint, because `s` and `u` are different variables, albeit with the same value
62+
let _ = (x << s) | (x >> (32 - u));
63+
}

‎tests/ui/manual_rotate.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ fn main() {
44
let (x_u8, x_u16, x_u32, x_u64) = (1u8, 1u16, 1u32, 1u64);
55
let (x_i8, x_i16, x_i32, x_i64) = (1i8, 1i16, 1i32, 1i64);
66
let a_u32 = 1u32;
7+
const N: u32 = 5;
78
// True positives
89
let y_u8 = (x_u8 >> 3) | (x_u8 << 5);
910
//~^ manual_rotate
@@ -29,6 +30,9 @@ fn main() {
2930
//~^ manual_rotate
3031
let y_u64_as = (x_u32 as u64 >> 8) | ((x_u32 as u64) << 56);
3132
//~^ manual_rotate
33+
// shift by a const
34+
let _ = (x_i64 >> N) | (x_i64 << (64 - N));
35+
//~^ manual_rotate
3236

3337
// False positives - can't be replaced with a rotation
3438
let y_u8_false = (x_u8 >> 6) | (x_u8 << 3);
@@ -40,3 +44,20 @@ fn main() {
4044
let mut l = vec![12_u8, 34];
4145
let y = (l.pop().unwrap() << 3) + (l.pop().unwrap() >> 5);
4246
}
47+
48+
fn issue13028() {
49+
let s = 5;
50+
let u = 5;
51+
let x: u32 = 123456;
52+
53+
let _ = (x << s) | (x >> (32 - s));
54+
//~^ manual_rotate
55+
let _ = (x << s) | (x >> (31 ^ s));
56+
//~^ manual_rotate
57+
// still works with consts
58+
let _ = (x >> 9) | (x << (32 - 9));
59+
//~^ manual_rotate
60+
61+
// don't lint, because `s` and `u` are different variables, albeit with the same value
62+
let _ = (x << s) | (x >> (32 - u));
63+
}

‎tests/ui/manual_rotate.stderr‎

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: there is no need to manually implement bit rotation
2-
--> tests/ui/manual_rotate.rs:8:16
2+
--> tests/ui/manual_rotate.rs:9:16
33
|
44
LL | let y_u8 = (x_u8 >> 3) | (x_u8 << 5);
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u8.rotate_right(3)`
@@ -8,64 +8,88 @@ LL | let y_u8 = (x_u8 >> 3) | (x_u8 << 5);
88
= help: to override `-D warnings` add `#[allow(clippy::manual_rotate)]`
99

1010
error: there is no need to manually implement bit rotation
11-
--> tests/ui/manual_rotate.rs:10:17
11+
--> tests/ui/manual_rotate.rs:11:17
1212
|
1313
LL | let y_u16 = (x_u16 >> 7) | (x_u16 << 9);
1414
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u16.rotate_right(7)`
1515

1616
error: there is no need to manually implement bit rotation
17-
--> tests/ui/manual_rotate.rs:12:17
17+
--> tests/ui/manual_rotate.rs:13:17
1818
|
1919
LL | let y_u32 = (x_u32 >> 8) | (x_u32 << 24);
2020
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u32.rotate_right(8)`
2121

2222
error: there is no need to manually implement bit rotation
23-
--> tests/ui/manual_rotate.rs:14:17
23+
--> tests/ui/manual_rotate.rs:15:17
2424
|
2525
LL | let y_u64 = (x_u64 >> 9) | (x_u64 << 55);
2626
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u64.rotate_right(9)`
2727

2828
error: there is no need to manually implement bit rotation
29-
--> tests/ui/manual_rotate.rs:16:16
29+
--> tests/ui/manual_rotate.rs:17:16
3030
|
3131
LL | let y_i8 = (x_i8 >> 3) | (x_i8 << 5);
3232
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i8.rotate_right(3)`
3333

3434
error: there is no need to manually implement bit rotation
35-
--> tests/ui/manual_rotate.rs:18:17
35+
--> tests/ui/manual_rotate.rs:19:17
3636
|
3737
LL | let y_i16 = (x_i16 >> 7) | (x_i16 << 9);
3838
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i16.rotate_right(7)`
3939

4040
error: there is no need to manually implement bit rotation
41-
--> tests/ui/manual_rotate.rs:20:17
41+
--> tests/ui/manual_rotate.rs:21:17
4242
|
4343
LL | let y_i32 = (x_i32 >> 8) | (x_i32 << 24);
4444
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i32.rotate_right(8)`
4545

4646
error: there is no need to manually implement bit rotation
47-
--> tests/ui/manual_rotate.rs:22:17
47+
--> tests/ui/manual_rotate.rs:23:17
4848
|
4949
LL | let y_i64 = (x_i64 >> 9) | (x_i64 << 55);
5050
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i64.rotate_right(9)`
5151

5252
error: there is no need to manually implement bit rotation
53-
--> tests/ui/manual_rotate.rs:25:22
53+
--> tests/ui/manual_rotate.rs:26:22
5454
|
5555
LL | let y_u32_plus = (x_u32 >> 8) + (x_u32 << 24);
5656
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u32.rotate_right(8)`
5757

5858
error: there is no need to manually implement bit rotation
59-
--> tests/ui/manual_rotate.rs:28:25
59+
--> tests/ui/manual_rotate.rs:29:25
6060
|
6161
LL | let y_u32_complex = ((x_u32 | 3256) >> 8) | ((x_u32 | 3256) << 24);
6262
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `(x_u32 | 3256).rotate_right(8)`
6363

6464
error: there is no need to manually implement bit rotation
65-
--> tests/ui/manual_rotate.rs:30:20
65+
--> tests/ui/manual_rotate.rs:31:20
6666
|
6767
LL | let y_u64_as = (x_u32 as u64 >> 8) | ((x_u32 as u64) << 56);
6868
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `(x_u32 as u64).rotate_right(8)`
6969

70-
error: aborting due to 11 previous errors
70+
error: there is no need to manually implement bit rotation
71+
--> tests/ui/manual_rotate.rs:34:13
72+
|
73+
LL | let _ = (x_i64 >> N) | (x_i64 << (64 - N));
74+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i64.rotate_right(N)`
75+
76+
error: there is no need to manually implement bit rotation
77+
--> tests/ui/manual_rotate.rs:53:13
78+
|
79+
LL | let _ = (x << s) | (x >> (32 - s));
80+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x.rotate_left(s)`
81+
82+
error: there is no need to manually implement bit rotation
83+
--> tests/ui/manual_rotate.rs:55:13
84+
|
85+
LL | let _ = (x << s) | (x >> (31 ^ s));
86+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x.rotate_left(s)`
87+
88+
error: there is no need to manually implement bit rotation
89+
--> tests/ui/manual_rotate.rs:58:13
90+
|
91+
LL | let _ = (x >> 9) | (x << (32 - 9));
92+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x.rotate_right(9)`
93+
94+
error: aborting due to 15 previous errors
7195

0 commit comments

Comments
 (0)