Skip to content

Commit affde93

Browse files
y21y21
committed
lint on nested binary operations involving local and handle projections
1 parent 7343db9 commit affde93

File tree

4 files changed

+117
-15
lines changed

4 files changed

+117
-15
lines changed

‎clippy_lints/src/transmute/eager_transmute.rs

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::ty::is_normalizable;
3-
use clippy_utils::{path_to_local, path_to_local_id};
3+
use clippy_utils::{eq_expr_value, path_to_local};
44
use rustc_abi::WrappingRange;
55
use rustc_errors::Applicability;
66
use rustc_hir::{Expr, ExprKind, Node};
@@ -25,6 +25,36 @@ fn range_fully_contained(from: WrappingRange, to: WrappingRange) -> bool {
2525
to.contains(from.start) && to.contains(from.end)
2626
}
2727

28+
/// Checks if a given expression is a binary operation involving a local variable or is made up of
29+
/// other (nested) binary expressions involving the local. There must be at least one local
30+
/// reference that is the same as `local_expr`.
31+
///
32+
/// This is used as a heuristic to detect if a variable
33+
/// is checked to be within the valid range of a transmuted type.
34+
/// All of these would return true:
35+
/// * `x < 4`
36+
/// * `x < 4 && x > 1`
37+
/// * `x.field < 4 && x.field > 1` (given `x.field`)
38+
/// * `x.field < 4 && unrelated()`
39+
fn binops_with_local(cx: &LateContext<'_>, local_expr: &Expr<'_>, expr: &Expr<'_>) -> bool {
40+
match expr.kind {
41+
ExprKind::Binary(_, lhs, rhs) => {
42+
binops_with_local(cx, local_expr, lhs) || binops_with_local(cx, local_expr, rhs)
43+
},
44+
_ => eq_expr_value(cx, local_expr, expr),
45+
}
46+
}
47+
48+
/// Checks if an expression is a path to a local variable (with optional projections), e.g.
49+
/// `x.field[0].field2` would return true.
50+
fn is_local_with_projections(expr: &Expr<'_>) -> bool {
51+
match expr.kind {
52+
ExprKind::Path(_) => path_to_local(expr).is_some(),
53+
ExprKind::Field(expr, _) | ExprKind::Index(expr, ..) => is_local_with_projections(expr),
54+
_ => false,
55+
}
56+
}
57+
2858
pub(super) fn check<'tcx>(
2959
cx: &LateContext<'tcx>,
3060
expr: &'tcx Expr<'tcx>,
@@ -36,9 +66,8 @@ pub(super) fn check<'tcx>(
3666
&& let ExprKind::MethodCall(path, receiver, [arg], _) = then_some_call.kind
3767
&& cx.typeck_results().expr_ty(receiver).is_bool()
3868
&& path.ident.name == sym!(then_some)
39-
&& let ExprKind::Binary(_, lhs, rhs) = receiver.kind
40-
&& let Some(local_id) = path_to_local(transmutable)
41-
&& (path_to_local_id(lhs, local_id) || path_to_local_id(rhs, local_id))
69+
&& is_local_with_projections(transmutable)
70+
&& binops_with_local(cx, transmutable, receiver)
4271
&& is_normalizable(cx, cx.param_env, from_ty)
4372
&& is_normalizable(cx, cx.param_env, to_ty)
4473
// we only want to lint if the target type has a niche that is larger than the one of the source type

‎tests/ui/eager_transmute.fixed

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,36 @@ enum Opcode {
1212
Div = 3,
1313
}
1414

15+
struct Data {
16+
foo: &'static [u8],
17+
bar: &'static [u8],
18+
}
19+
1520
fn int_to_opcode(op: u8) -> Option<Opcode> {
1621
(op < 4).then(|| unsafe { std::mem::transmute(op) })
1722
}
1823

19-
fn f(op: u8, unrelated: u8) {
24+
fn f(op: u8, op2: Data, unrelated: u8) {
2025
true.then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
2126
(unrelated < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
2227
(op < 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
2328
(op > 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
2429
(op == 0).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
30+
31+
let _: Option<Opcode> = (op > 0 && op < 10).then(|| unsafe { std::mem::transmute(op) });
32+
let _: Option<Opcode> = (op > 0 && op < 10 && unrelated == 0).then(|| unsafe { std::mem::transmute(op) });
33+
34+
// lint even when the transmutable goes through field/array accesses
35+
let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then(|| unsafe { std::mem::transmute(op2.foo[0]) });
36+
37+
// don't lint: wrong index used in the transmute
38+
let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then_some(unsafe { std::mem::transmute(op2.foo[1]) });
39+
40+
// don't lint: no check for the transmutable in the condition
41+
let _: Option<Opcode> = (op2.foo[0] > 0 && op2.bar[1] < 10).then_some(unsafe { std::mem::transmute(op2.bar[0]) });
42+
43+
// don't lint: wrong variable
44+
let _: Option<Opcode> = (op2.foo[0] > 0 && op2.bar[1] < 10).then_some(unsafe { std::mem::transmute(op) });
2545
}
2646

2747
unsafe fn f2(op: u8) {

‎tests/ui/eager_transmute.rs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,36 @@ enum Opcode {
1212
Div = 3,
1313
}
1414

15+
struct Data {
16+
foo: &'static [u8],
17+
bar: &'static [u8],
18+
}
19+
1520
fn int_to_opcode(op: u8) -> Option<Opcode> {
1621
(op < 4).then_some(unsafe { std::mem::transmute(op) })
1722
}
1823

19-
fn f(op: u8, unrelated: u8) {
24+
fn f(op: u8, op2: Data, unrelated: u8) {
2025
true.then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
2126
(unrelated < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
2227
(op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
2328
(op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
2429
(op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
30+
31+
let _: Option<Opcode> = (op > 0 && op < 10).then_some(unsafe { std::mem::transmute(op) });
32+
let _: Option<Opcode> = (op > 0 && op < 10 && unrelated == 0).then_some(unsafe { std::mem::transmute(op) });
33+
34+
// lint even when the transmutable goes through field/array accesses
35+
let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then_some(unsafe { std::mem::transmute(op2.foo[0]) });
36+
37+
// don't lint: wrong index used in the transmute
38+
let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then_some(unsafe { std::mem::transmute(op2.foo[1]) });
39+
40+
// don't lint: no check for the transmutable in the condition
41+
let _: Option<Opcode> = (op2.foo[0] > 0 && op2.bar[1] < 10).then_some(unsafe { std::mem::transmute(op2.bar[0]) });
42+
43+
// don't lint: wrong variable
44+
let _: Option<Opcode> = (op2.foo[0] > 0 && op2.bar[1] < 10).then_some(unsafe { std::mem::transmute(op) });
2545
}
2646

2747
unsafe fn f2(op: u8) {

‎tests/ui/eager_transmute.stderr

Lines changed: 42 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: this transmute is always evaluated eagerly, even if the condition is false
2-
--> $DIR/eager_transmute.rs:16:33
2+
--> $DIR/eager_transmute.rs:21:33
33
|
44
LL | (op < 4).then_some(unsafe { std::mem::transmute(op) })
55
| ^^^^^^^^^^^^^^^^^^^^^^^
@@ -12,7 +12,7 @@ LL | (op < 4).then(|| unsafe { std::mem::transmute(op) })
1212
| ~~~~ ++
1313

1414
error: this transmute is always evaluated eagerly, even if the condition is false
15-
--> $DIR/eager_transmute.rs:22:33
15+
--> $DIR/eager_transmute.rs:27:33
1616
|
1717
LL | (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
1818
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -23,7 +23,7 @@ LL | (op < 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
2323
| ~~~~ ++
2424

2525
error: this transmute is always evaluated eagerly, even if the condition is false
26-
--> $DIR/eager_transmute.rs:23:33
26+
--> $DIR/eager_transmute.rs:28:33
2727
|
2828
LL | (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
2929
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -34,7 +34,7 @@ LL | (op > 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
3434
| ~~~~ ++
3535

3636
error: this transmute is always evaluated eagerly, even if the condition is false
37-
--> $DIR/eager_transmute.rs:24:34
37+
--> $DIR/eager_transmute.rs:29:34
3838
|
3939
LL | (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
4040
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -45,7 +45,40 @@ LL | (op == 0).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
4545
| ~~~~ ++
4646

4747
error: this transmute is always evaluated eagerly, even if the condition is false
48-
--> $DIR/eager_transmute.rs:28:24
48+
--> $DIR/eager_transmute.rs:31:68
49+
|
50+
LL | let _: Option<Opcode> = (op > 0 && op < 10).then_some(unsafe { std::mem::transmute(op) });
51+
| ^^^^^^^^^^^^^^^^^^^^^^^
52+
|
53+
help: consider using `bool::then` to only transmute if the condition holds
54+
|
55+
LL | let _: Option<Opcode> = (op > 0 && op < 10).then(|| unsafe { std::mem::transmute(op) });
56+
| ~~~~ ++
57+
58+
error: this transmute is always evaluated eagerly, even if the condition is false
59+
--> $DIR/eager_transmute.rs:32:86
60+
|
61+
LL | let _: Option<Opcode> = (op > 0 && op < 10 && unrelated == 0).then_some(unsafe { std::mem::transmute(op) });
62+
| ^^^^^^^^^^^^^^^^^^^^^^^
63+
|
64+
help: consider using `bool::then` to only transmute if the condition holds
65+
|
66+
LL | let _: Option<Opcode> = (op > 0 && op < 10 && unrelated == 0).then(|| unsafe { std::mem::transmute(op) });
67+
| ~~~~ ++
68+
69+
error: this transmute is always evaluated eagerly, even if the condition is false
70+
--> $DIR/eager_transmute.rs:35:84
71+
|
72+
LL | let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then_some(unsafe { std::mem::transmute(op2.foo[0]) });
73+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
74+
|
75+
help: consider using `bool::then` to only transmute if the condition holds
76+
|
77+
LL | let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then(|| unsafe { std::mem::transmute(op2.foo[0]) });
78+
| ~~~~ ++
79+
80+
error: this transmute is always evaluated eagerly, even if the condition is false
81+
--> $DIR/eager_transmute.rs:48:24
4982
|
5083
LL | (op < 4).then_some(std::mem::transmute::<_, Opcode>(op));
5184
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -56,7 +89,7 @@ LL | (op < 4).then(|| std::mem::transmute::<_, Opcode>(op));
5689
| ~~~~ ++
5790

5891
error: this transmute is always evaluated eagerly, even if the condition is false
59-
--> $DIR/eager_transmute.rs:57:60
92+
--> $DIR/eager_transmute.rs:77:60
6093
|
6194
LL | let _: Option<NonZeroU8> = (v1 > 0).then_some(unsafe { std::mem::transmute(v1) });
6295
| ^^^^^^^^^^^^^^^^^^^^^^^
@@ -67,7 +100,7 @@ LL | let _: Option<NonZeroU8> = (v1 > 0).then(|| unsafe { std::mem::transmut
67100
| ~~~~ ++
68101

69102
error: this transmute is always evaluated eagerly, even if the condition is false
70-
--> $DIR/eager_transmute.rs:63:86
103+
--> $DIR/eager_transmute.rs:83:86
71104
|
72105
LL | let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) });
73106
| ^^^^^^^^^^^^^^^^^^^^^^^
@@ -78,7 +111,7 @@ LL | let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| u
78111
| ~~~~ ++
79112

80113
error: this transmute is always evaluated eagerly, even if the condition is false
81-
--> $DIR/eager_transmute.rs:69:93
114+
--> $DIR/eager_transmute.rs:89:93
82115
|
83116
LL | let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) });
84117
| ^^^^^^^^^^^^^^^^^^^^^^^
@@ -88,5 +121,5 @@ help: consider using `bool::then` to only transmute if the condition holds
88121
LL | let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| unsafe { std::mem::transmute(v2) });
89122
| ~~~~ ++
90123

91-
error: aborting due to 8 previous errors
124+
error: aborting due to 11 previous errors
92125

0 commit comments

Comments
 (0)