Auto merge of #12031 - y21:eager_transmute_more_binops, r=llogiq

Lint nested binary operations and handle field projections in `eager_transmute`

This PR makes the lint a bit stronger. Previously it would only lint `(x < 4).then_some(transmute(x))` (that is, a single binary op in the condition). With this change, it understands:
- multiple, nested binary ops: `(x < 4 && x > 1).then_some(...)`
- local references with projections: `(x.field < 4 && x.field > 1).then_some(transmute(x.field))`

changelog: [`eager_transmute`]: lint nested binary operations and look through field/array accesses

r? llogiq (since you reviewed my initial PR #11981, I figured you have the most context here, sorry if you are too busy with other PRs, feel free to reassign to someone else then)

clippy_lints/src/transmute/eager_transmute.rs

@@ -1,6 +1,6 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::ty::is_normalizable;
-use clippy_utils::{path_to_local, path_to_local_id};
+use clippy_utils::{eq_expr_value, path_to_local};
 use rustc_abi::WrappingRange;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, Node};
@@ -25,6 +25,52 @@ fn range_fully_contained(from: WrappingRange, to: WrappingRange) -> bool {
     to.contains(from.start) && to.contains(from.end)
 }
 
+/// Checks if a given expression is a binary operation involving a local variable or is made up of
+/// other (nested) binary expressions involving the local. There must be at least one local
+/// reference that is the same as `local_expr`.
+///
+/// This is used as a heuristic to detect if a variable
+/// is checked to be within the valid range of a transmuted type.
+/// All of these would return true:
+/// * `x < 4`
+/// * `x < 4 && x > 1`
+/// * `x.field < 4 && x.field > 1` (given `x.field`)
+/// * `x.field < 4 && unrelated()`
+/// * `(1..=3).contains(&x)`
+fn binops_with_local(cx: &LateContext<'_>, local_expr: &Expr<'_>, expr: &Expr<'_>) -> bool {
+    match expr.kind {
+        ExprKind::Binary(_, lhs, rhs) => {
+            binops_with_local(cx, local_expr, lhs) || binops_with_local(cx, local_expr, rhs)
+        },
+        ExprKind::MethodCall(path, receiver, [arg], _)
+            if path.ident.name == sym!(contains)
+                // ... `contains` called on some kind of range
+                && let Some(receiver_adt) = cx.typeck_results().expr_ty(receiver).peel_refs().ty_adt_def()
+                && let lang_items = cx.tcx.lang_items()
+                && [
+                    lang_items.range_from_struct(),
+                    lang_items.range_inclusive_struct(),
+                    lang_items.range_struct(),
+                    lang_items.range_to_inclusive_struct(),
+                    lang_items.range_to_struct()
+                ].into_iter().any(|did| did == Some(receiver_adt.did())) =>
+        {
+            eq_expr_value(cx, local_expr, arg.peel_borrows())
+        },
+        _ => eq_expr_value(cx, local_expr, expr),
+    }
+}
+
+/// Checks if an expression is a path to a local variable (with optional projections), e.g.
+/// `x.field[0].field2` would return true.
+fn is_local_with_projections(expr: &Expr<'_>) -> bool {
+    match expr.kind {
+        ExprKind::Path(_) => path_to_local(expr).is_some(),
+        ExprKind::Field(expr, _) | ExprKind::Index(expr, ..) => is_local_with_projections(expr),
+        _ => false,
+    }
+}
+
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx Expr<'tcx>,
@@ -36,9 +82,8 @@ pub(super) fn check<'tcx>(
         && let ExprKind::MethodCall(path, receiver, [arg], _) = then_some_call.kind
         && cx.typeck_results().expr_ty(receiver).is_bool()
         && path.ident.name == sym!(then_some)
-        && let ExprKind::Binary(_, lhs, rhs) = receiver.kind
-        && let Some(local_id) = path_to_local(transmutable)
-        && (path_to_local_id(lhs, local_id) || path_to_local_id(rhs, local_id))
+        && is_local_with_projections(transmutable)
+        && binops_with_local(cx, transmutable, receiver)
         && is_normalizable(cx, cx.param_env, from_ty)
         && is_normalizable(cx, cx.param_env, to_ty)
         // 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

@@ -12,16 +12,49 @@ enum Opcode {
     Div = 3,
 }
 
+struct Data {
+    foo: &'static [u8],
+    bar: &'static [u8],
+}
+
 fn int_to_opcode(op: u8) -> Option<Opcode> {
     (op < 4).then(|| unsafe { std::mem::transmute(op) })
 }
 
-fn f(op: u8, unrelated: u8) {
+fn f(op: u8, op2: Data, unrelated: u8) {
     true.then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
     (unrelated < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
     (op < 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
     (op > 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
     (op == 0).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
+
+    let _: Option<Opcode> = (op > 0 && op < 10).then(|| unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (op > 0 && op < 10 && unrelated == 0).then(|| unsafe { std::mem::transmute(op) });
+
+    // lint even when the transmutable goes through field/array accesses
+    let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then(|| unsafe { std::mem::transmute(op2.foo[0]) });
+
+    // don't lint: wrong index used in the transmute
+    let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then_some(unsafe { std::mem::transmute(op2.foo[1]) });
+
+    // don't lint: no check for the transmutable in the condition
+    let _: Option<Opcode> = (op2.foo[0] > 0 && op2.bar[1] < 10).then_some(unsafe { std::mem::transmute(op2.bar[0]) });
+
+    // don't lint: wrong variable
+    let _: Option<Opcode> = (op2.foo[0] > 0 && op2.bar[1] < 10).then_some(unsafe { std::mem::transmute(op) });
+
+    // range contains checks
+    let _: Option<Opcode> = (1..=3).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = ((1..=3).contains(&op) || op == 4).then(|| unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (1..3).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (1..).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (..3).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (..=3).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+
+    // unrelated binding in contains
+    let _: Option<Opcode> = (1..=3)
+        .contains(&unrelated)
+        .then_some(unsafe { std::mem::transmute(op) });
 }
 
 unsafe fn f2(op: u8) {

tests/ui/eager_transmute.rs

@@ -12,16 +12,49 @@ enum Opcode {
     Div = 3,
 }
 
+struct Data {
+    foo: &'static [u8],
+    bar: &'static [u8],
+}
+
 fn int_to_opcode(op: u8) -> Option<Opcode> {
     (op < 4).then_some(unsafe { std::mem::transmute(op) })
 }
 
-fn f(op: u8, unrelated: u8) {
+fn f(op: u8, op2: Data, unrelated: u8) {
     true.then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
     (unrelated < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
     (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
     (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
     (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
+
+    let _: Option<Opcode> = (op > 0 && op < 10).then_some(unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (op > 0 && op < 10 && unrelated == 0).then_some(unsafe { std::mem::transmute(op) });
+
+    // lint even when the transmutable goes through field/array accesses
+    let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then_some(unsafe { std::mem::transmute(op2.foo[0]) });
+
+    // don't lint: wrong index used in the transmute
+    let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then_some(unsafe { std::mem::transmute(op2.foo[1]) });
+
+    // don't lint: no check for the transmutable in the condition
+    let _: Option<Opcode> = (op2.foo[0] > 0 && op2.bar[1] < 10).then_some(unsafe { std::mem::transmute(op2.bar[0]) });
+
+    // don't lint: wrong variable
+    let _: Option<Opcode> = (op2.foo[0] > 0 && op2.bar[1] < 10).then_some(unsafe { std::mem::transmute(op) });
+
+    // range contains checks
+    let _: Option<Opcode> = (1..=3).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = ((1..=3).contains(&op) || op == 4).then_some(unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (1..3).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (1..).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (..3).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+    let _: Option<Opcode> = (..=3).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+
+    // unrelated binding in contains
+    let _: Option<Opcode> = (1..=3)
+        .contains(&unrelated)
+        .then_some(unsafe { std::mem::transmute(op) });
 }
 
 unsafe fn f2(op: u8) {

tests/ui/eager_transmute.stderr

@@ -1,5 +1,5 @@
 error: this transmute is always evaluated eagerly, even if the condition is false
-  --> $DIR/eager_transmute.rs:16:33
+  --> $DIR/eager_transmute.rs:21:33
    |
 LL |     (op < 4).then_some(unsafe { std::mem::transmute(op) })
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^
@@ -12,7 +12,7 @@ LL |     (op < 4).then(|| unsafe { std::mem::transmute(op) })
    |              ~~~~ ++
 
 error: this transmute is always evaluated eagerly, even if the condition is false
-  --> $DIR/eager_transmute.rs:22:33
+  --> $DIR/eager_transmute.rs:27:33
    |
 LL |     (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -23,7 +23,7 @@ LL |     (op < 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
    |              ~~~~ ++
 
 error: this transmute is always evaluated eagerly, even if the condition is false
-  --> $DIR/eager_transmute.rs:23:33
+  --> $DIR/eager_transmute.rs:28:33
    |
 LL |     (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -34,7 +34,7 @@ LL |     (op > 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
    |              ~~~~ ++
 
 error: this transmute is always evaluated eagerly, even if the condition is false
-  --> $DIR/eager_transmute.rs:24:34
+  --> $DIR/eager_transmute.rs:29:34
    |
 LL |     (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -45,7 +45,106 @@ LL |     (op == 0).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
    |               ~~~~ ++
 
 error: this transmute is always evaluated eagerly, even if the condition is false
-  --> $DIR/eager_transmute.rs:28:24
+  --> $DIR/eager_transmute.rs:31:68
+   |
+LL |     let _: Option<Opcode> = (op > 0 && op < 10).then_some(unsafe { std::mem::transmute(op) });
+   |                                                                    ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = (op > 0 && op < 10).then(|| unsafe { std::mem::transmute(op) });
+   |                                                 ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:32:86
+   |
+LL |     let _: Option<Opcode> = (op > 0 && op < 10 && unrelated == 0).then_some(unsafe { std::mem::transmute(op) });
+   |                                                                                      ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = (op > 0 && op < 10 && unrelated == 0).then(|| unsafe { std::mem::transmute(op) });
+   |                                                                   ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:35:84
+   |
+LL |     let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then_some(unsafe { std::mem::transmute(op2.foo[0]) });
+   |                                                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = (op2.foo[0] > 0 && op2.foo[0] < 10).then(|| unsafe { std::mem::transmute(op2.foo[0]) });
+   |                                                                 ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:47:70
+   |
+LL |     let _: Option<Opcode> = (1..=3).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+   |                                                                      ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = (1..=3).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+   |                                                   ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:48:83
+   |
+LL |     let _: Option<Opcode> = ((1..=3).contains(&op) || op == 4).then_some(unsafe { std::mem::transmute(op) });
+   |                                                                                   ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = ((1..=3).contains(&op) || op == 4).then(|| unsafe { std::mem::transmute(op) });
+   |                                                                ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:49:69
+   |
+LL |     let _: Option<Opcode> = (1..3).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+   |                                                                     ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = (1..3).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+   |                                                  ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:50:68
+   |
+LL |     let _: Option<Opcode> = (1..).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+   |                                                                    ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = (1..).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+   |                                                 ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:51:68
+   |
+LL |     let _: Option<Opcode> = (..3).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+   |                                                                    ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = (..3).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+   |                                                 ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:52:69
+   |
+LL |     let _: Option<Opcode> = (..=3).contains(&op).then_some(unsafe { std::mem::transmute(op) });
+   |                                                                     ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `bool::then` to only transmute if the condition holds
+   |
+LL |     let _: Option<Opcode> = (..=3).contains(&op).then(|| unsafe { std::mem::transmute(op) });
+   |                                                  ~~~~ ++
+
+error: this transmute is always evaluated eagerly, even if the condition is false
+  --> $DIR/eager_transmute.rs:61:24
    |
 LL |     (op < 4).then_some(std::mem::transmute::<_, Opcode>(op));
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -56,7 +155,7 @@ LL |     (op < 4).then(|| std::mem::transmute::<_, Opcode>(op));
    |              ~~~~ ++
 
 error: this transmute is always evaluated eagerly, even if the condition is false
-  --> $DIR/eager_transmute.rs:57:60
+  --> $DIR/eager_transmute.rs:90:60
    |
 LL |     let _: Option<NonZeroU8> = (v1 > 0).then_some(unsafe { std::mem::transmute(v1) });
    |                                                            ^^^^^^^^^^^^^^^^^^^^^^^
@@ -67,7 +166,7 @@ LL |     let _: Option<NonZeroU8> = (v1 > 0).then(|| unsafe { std::mem::transmut
    |                                         ~~~~ ++
 
 error: this transmute is always evaluated eagerly, even if the condition is false
-  --> $DIR/eager_transmute.rs:63:86
+  --> $DIR/eager_transmute.rs:96:86
    |
 LL |     let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) });
    |                                                                                      ^^^^^^^^^^^^^^^^^^^^^^^
@@ -78,7 +177,7 @@ LL |     let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| u
    |                                                                   ~~~~ ++
 
 error: this transmute is always evaluated eagerly, even if the condition is false
-  --> $DIR/eager_transmute.rs:69:93
+  --> $DIR/eager_transmute.rs:102:93
    |
 LL |     let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) });
    |                                                                                             ^^^^^^^^^^^^^^^^^^^^^^^
@@ -88,5 +187,5 @@ help: consider using `bool::then` to only transmute if the condition holds
 LL |     let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| unsafe { std::mem::transmute(v2) });
    |                                                                          ~~~~ ++
 
-error: aborting due to 8 previous errors
+error: aborting due to 17 previous errors