new lint: eager_int_transmute

CHANGELOG.md

@@ -5044,6 +5044,7 @@ Released 2018-09-13
 [`duplicate_mod`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_mod
 [`duplicate_underscore_argument`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument
 [`duration_subsec`]: https://rust-lang.github.io/rust-clippy/master/index.html#duration_subsec
+[`eager_int_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#eager_int_transmute
 [`else_if_without_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#else_if_without_else
 [`empty_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_drop
 [`empty_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_enum

clippy_lints/src/declared_lints.rs

@@ -651,6 +651,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS_INFO,
     crate::trait_bounds::TYPE_REPETITION_IN_BOUNDS_INFO,
     crate::transmute::CROSSPOINTER_TRANSMUTE_INFO,
+    crate::transmute::EAGER_INT_TRANSMUTE_INFO,
     crate::transmute::TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS_INFO,
     crate::transmute::TRANSMUTE_BYTES_TO_STR_INFO,
     crate::transmute::TRANSMUTE_FLOAT_TO_INT_INFO,

clippy_lints/src/transmute/eager_int_transmute.rs

@@ -0,0 +1,39 @@
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::{path_to_local, path_to_local_id};
+use rustc_hir::{Expr, ExprKind, Node};
+use rustc_lint::LateContext;
+
+use super::EAGER_INT_TRANSMUTE;
+
+fn peel_parent_unsafe_blocks<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
+    for (_, parent) in cx.tcx.hir().parent_iter(expr.hir_id) {
+        match parent {
+            Node::Block(_) => {},
+            Node::Expr(e) if let ExprKind::Block(..) = e.kind => {},
+            Node::Expr(e) => return Some(e),
+            _ => break,
+        }
+    }
+    None
+}
+
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, transmutable: &'tcx Expr<'tcx>) -> bool {
+    if let Some(then_some_call) = peel_parent_unsafe_blocks(cx, expr)
+        && let ExprKind::MethodCall(path, receiver, ..) = then_some_call.kind
+        && cx.typeck_results().expr_ty(receiver).is_bool()
+        && path.ident.name == sym!(then_some)
+        && let ExprKind::Binary(_, lhs, _) = receiver.kind
+        && let Some(local_id) = path_to_local(transmutable)
+        && path_to_local_id(lhs, local_id)
+    {
+        span_lint_and_help(
+            cx,
+            EAGER_INT_TRANSMUTE,
+            expr.span,
+            "this transmute is evaluated eagerly, even if the condition is false",
+            Some(path.ident.span),
+            "consider using `then` instead, which evaluates it only if the condition is true",
+        );
+    }
+    false
+}

clippy_lints/src/transmute/mod.rs

@@ -1,4 +1,5 @@
 mod crosspointer_transmute;
+mod eager_int_transmute;
 mod transmute_float_to_int;
 mod transmute_int_to_bool;
 mod transmute_int_to_char;
@@ -463,6 +464,62 @@ declare_clippy_lint! {
     "transmute results in a null function pointer, which is undefined behavior"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for integer validity checks, followed by a transmute that is (incorrectly) evaluated
+    /// eagerly (e.g. using `bool::then_some`).
+    ///
+    /// ### Why is this bad?
+    /// Eager evaluation means that the `transmute` call is executed regardless of whether the condition is true or false.
+    /// This can introduce unsoundness and other subtle bugs.
+    ///
+    /// ### Example
+    /// Consider the following function which is meant to convert an unsigned integer to its enum equivalent via transmute.
+    ///
+    /// ```no_run
+    /// #[repr(u8)]
+    /// enum Opcode {
+    ///     Add = 0,
+    ///     Sub = 1,
+    ///     Mul = 2,
+    ///     Div = 3
+    /// }
+    ///
+    /// fn int_to_opcode(op: u8) -> Option<Opcode> {
+    ///     (op < 4).then_some(unsafe { std::mem::transmute(op) })
+    /// }
+    /// ```
+    /// This may appear fine at first given that it checks that the `u8` is within the validity range of the enum,
+    /// *however* the transmute is evaluated eagerly, meaning that it executes even if `op >= 4`!
+    ///
+    /// This makes the function unsound, because it is possible for the caller to cause undefined behavior
+    /// (creating an enum with an invalid bitpattern) entirely in safe code only by passing an incorrect value,
+    /// which is normally only a bug that is possible in unsafe code.
+    ///
+    /// One possible way in which this can go wrong practically is that the compiler sees it as:
+    /// ```rust,ignore (illustrative)
+    /// let temp: Foo = unsafe { std::mem::transmute(op) };
+    /// (0 < 4).then_some(temp)
+    /// ```
+    /// and optimizes away the `(0 < 4)` check based on the assumption that since a `Foo` was created from `op` with the validity range `0..3`,
+    /// it is **impossible** for this condition to be false.
+    ///
+    /// In short, it is possible for this function to be optimized in a way that makes it [never return `None`](https://godbolt.org/z/ocrcenevq),
+    /// even if passed the value `4`.
+    ///
+    /// This can be avoided by instead using lazy evaluation. For the example above, this should be written:
+    /// ```rust,ignore (illustrative)
+    /// fn int_to_opcode(op: u8) -> Option<Opcode> {
+    ///     (op < 4).then(|| unsafe { std::mem::transmute(op) })
+    ///              ^^^^ ^^ `bool::then` only executes the closure if the condition is true!
+    /// }
+    /// ```
+    #[clippy::version = "1.76.0"]
+    pub EAGER_INT_TRANSMUTE,
+    suspicious,
+    "eager evaluation of `transmute`"
+}
+
 pub struct Transmute {
     msrv: Msrv,
 }
@@ -484,6 +541,7 @@ impl_lint_pass!(Transmute => [
     TRANSMUTE_UNDEFINED_REPR,
     TRANSMUTING_NULL,
     TRANSMUTE_NULL_TO_FN,
+    EAGER_INT_TRANSMUTE,
 ]);
 impl Transmute {
     #[must_use]
@@ -530,7 +588,8 @@ impl<'tcx> LateLintPass<'tcx> for Transmute {
                 | transmute_float_to_int::check(cx, e, from_ty, to_ty, arg, const_context)
                 | transmute_num_to_bytes::check(cx, e, from_ty, to_ty, arg, const_context)
                 | (unsound_collection_transmute::check(cx, e, from_ty, to_ty)
-                    || transmute_undefined_repr::check(cx, e, from_ty, to_ty));
+                    || transmute_undefined_repr::check(cx, e, from_ty, to_ty))
+                | (eager_int_transmute::check(cx, e, arg));
 
             if !linted {
                 transmutes_expressible_as_ptr_casts::check(cx, e, from_ty, from_ty_adjusted, to_ty, arg);

tests/ui/eager_int_transmute.rs

@@ -0,0 +1,27 @@
+#![warn(clippy::eager_int_transmute)]
+
+#[repr(u8)]
+enum Opcode {
+    Add = 0,
+    Sub = 1,
+    Mul = 2,
+    Div = 3,
+}
+
+fn int_to_opcode(op: u8) -> Option<Opcode> {
+    (op < 4).then_some(unsafe { std::mem::transmute(op) })
+}
+
+fn f(op: u8, 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) });
+}
+
+unsafe fn f2(op: u8) {
+    (op < 4).then_some(std::mem::transmute::<_, Opcode>(op));
+}
+
+fn main() {}

tests/ui/eager_int_transmute.stderr

@@ -0,0 +1,64 @@
+error: this transmute is evaluated eagerly, even if the condition is false
+  --> $DIR/eager_int_transmute.rs:12:33
+   |
+LL |     (op < 4).then_some(unsafe { std::mem::transmute(op) })
+   |                                 ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `then` instead, which evaluates it only if the condition is true
+  --> $DIR/eager_int_transmute.rs:12:14
+   |
+LL |     (op < 4).then_some(unsafe { std::mem::transmute(op) })
+   |              ^^^^^^^^^
+   = note: `-D clippy::eager-int-transmute` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::eager_int_transmute)]`
+
+error: this transmute is evaluated eagerly, even if the condition is false
+  --> $DIR/eager_int_transmute.rs:18:33
+   |
+LL |     (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
+   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `then` instead, which evaluates it only if the condition is true
+  --> $DIR/eager_int_transmute.rs:18:14
+   |
+LL |     (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
+   |              ^^^^^^^^^
+
+error: this transmute is evaluated eagerly, even if the condition is false
+  --> $DIR/eager_int_transmute.rs:19:33
+   |
+LL |     (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
+   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `then` instead, which evaluates it only if the condition is true
+  --> $DIR/eager_int_transmute.rs:19:14
+   |
+LL |     (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
+   |              ^^^^^^^^^
+
+error: this transmute is evaluated eagerly, even if the condition is false
+  --> $DIR/eager_int_transmute.rs:20:34
+   |
+LL |     (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
+   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `then` instead, which evaluates it only if the condition is true
+  --> $DIR/eager_int_transmute.rs:20:15
+   |
+LL |     (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) });
+   |               ^^^^^^^^^
+
+error: this transmute is evaluated eagerly, even if the condition is false
+  --> $DIR/eager_int_transmute.rs:24:24
+   |
+LL |     (op < 4).then_some(std::mem::transmute::<_, Opcode>(op));
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using `then` instead, which evaluates it only if the condition is true
+  --> $DIR/eager_int_transmute.rs:24:14
+   |
+LL |     (op < 4).then_some(std::mem::transmute::<_, Opcode>(op));
+   |              ^^^^^^^^^
+
+error: aborting due to 5 previous errors
+