Auto merge of #11981 - y21:eager_int_transmute, r=llogiq

new lint: `eager_transmute`

A small but still hopefully useful lint that looks for patterns such as `(x < 5).then_some(transmute(x))`.
This is almost certainly wrong because it evaluates the transmute eagerly and can lead to surprises such as the check being completely removed and always evaluating to `Some` no matter what `x` is (it is UB after all when the integer is not a valid bitpattern for the transmuted-to type). [Example](https://godbolt.org/z/xoY34fPzh).
The user most likely meant to use `then` instead.

I can't remember where I saw this but this is inspired by a real bug that happened in practice.

This could probably be a correctness lint?

changelog: new lint: [`eager_int_transmute`]

Author: bors

CHANGELOG.md

@@ -5101,6 +5101,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_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#eager_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

@@ -653,6 +653,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_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/lib.rs

@@ -22,6 +22,7 @@
 // FIXME: switch to something more ergonomic here, once available.
 // (Currently there is no way to opt into sysroot crates without `extern crate`.)
 extern crate pulldown_cmark;
+extern crate rustc_abi;
 extern crate rustc_arena;
 extern crate rustc_ast;
 extern crate rustc_ast_pretty;

clippy_lints/src/transmute/eager_transmute.rs

@@ -0,0 +1,74 @@
+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 rustc_abi::WrappingRange;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, Node};
+use rustc_lint::LateContext;
+use rustc_middle::ty::Ty;
+
+use super::EAGER_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
+}
+
+fn range_fully_contained(from: WrappingRange, to: WrappingRange) -> bool {
+    to.contains(from.start) && to.contains(from.end)
+}
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    transmutable: &'tcx Expr<'tcx>,
+    from_ty: Ty<'tcx>,
+    to_ty: Ty<'tcx>,
+) -> bool {
+    if let Some(then_some_call) = peel_parent_unsafe_blocks(cx, expr)
+        && 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_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
+        // e.g. `u8` to `NonZeroU8` should lint, but `NonZeroU8` to `u8` should not
+        && let Ok(from_layout) = cx.tcx.layout_of(cx.param_env.and(from_ty))
+        && let Ok(to_layout) = cx.tcx.layout_of(cx.param_env.and(to_ty))
+        && match (from_layout.largest_niche, to_layout.largest_niche) {
+            (Some(from_niche), Some(to_niche)) => !range_fully_contained(from_niche.valid_range, to_niche.valid_range),
+            (None, Some(_)) => true,
+            (_, None) => false,
+        }
+    {
+        span_lint_and_then(
+            cx,
+            EAGER_TRANSMUTE,
+            expr.span,
+            "this transmute is always evaluated eagerly, even if the condition is false",
+            |diag| {
+                diag.multipart_suggestion(
+                    "consider using `bool::then` to only transmute if the condition holds",
+                    vec![
+                        (path.ident.span, "then".into()),
+                        (arg.span.shrink_to_lo(), "|| ".into()),
+                    ],
+                    Applicability::MaybeIncorrect,
+                );
+            },
+        );
+        true
+    } else {
+        false
+    }
+}

clippy_lints/src/transmute/mod.rs

@@ -1,4 +1,5 @@
 mod crosspointer_transmute;
+mod eager_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_TRANSMUTE,
+    correctness,
+    "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_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_transmute::check(cx, e, arg, from_ty, to_ty));
 
             if !linted {
                 transmutes_expressible_as_ptr_casts::check(cx, e, from_ty, from_ty_adjusted, to_ty, arg);

tests/ui/eager_transmute.fixed

@@ -0,0 +1,72 @@
+#![feature(rustc_attrs)]
+#![warn(clippy::eager_transmute)]
+#![allow(clippy::transmute_int_to_non_zero)]
+
+use std::num::NonZeroU8;
+
+#[repr(u8)]
+enum Opcode {
+    Add = 0,
+    Sub = 1,
+    Mul = 2,
+    Div = 3,
+}
+
+fn int_to_opcode(op: u8) -> Option<Opcode> {
+    (op < 4).then(|| 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(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
+    (op > 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
+    (op == 0).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) });
+}
+
+unsafe fn f2(op: u8) {
+    (op < 4).then(|| std::mem::transmute::<_, Opcode>(op));
+}
+
+#[rustc_layout_scalar_valid_range_end(254)]
+struct NonMaxU8(u8);
+#[rustc_layout_scalar_valid_range_end(254)]
+#[rustc_layout_scalar_valid_range_start(1)]
+struct NonZeroNonMaxU8(u8);
+
+macro_rules! impls {
+    ($($t:ty),*) => {
+        $(
+            impl PartialEq<u8> for $t {
+                fn eq(&self, other: &u8) -> bool {
+                    self.0 == *other
+                }
+            }
+            impl PartialOrd<u8> for $t {
+                fn partial_cmp(&self, other: &u8) -> Option<std::cmp::Ordering> {
+                    self.0.partial_cmp(other)
+                }
+            }
+        )*
+    };
+}
+impls!(NonMaxU8, NonZeroNonMaxU8);
+
+fn niche_tests(v1: u8, v2: NonZeroU8, v3: NonZeroNonMaxU8) {
+    // u8 -> NonZeroU8, do lint
+    let _: Option<NonZeroU8> = (v1 > 0).then(|| unsafe { std::mem::transmute(v1) });
+
+    // NonZeroU8 -> u8, don't lint, target type has no niche and therefore a higher validity range
+    let _: Option<u8> = (v2 > NonZeroU8::new(1).unwrap()).then_some(unsafe { std::mem::transmute(v2) });
+
+    // NonZeroU8 -> NonMaxU8, do lint, different niche
+    let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| unsafe { std::mem::transmute(v2) });
+
+    // NonZeroNonMaxU8 -> NonMaxU8, don't lint, target type has more validity
+    let _: Option<NonMaxU8> = (v3 < 255).then_some(unsafe { std::mem::transmute(v2) });
+
+    // NonZeroU8 -> NonZeroNonMaxU8, do lint, target type has less validity
+    let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| unsafe { std::mem::transmute(v2) });
+}
+
+fn main() {}

tests/ui/eager_transmute.rs

@@ -0,0 +1,72 @@
+#![feature(rustc_attrs)]
+#![warn(clippy::eager_transmute)]
+#![allow(clippy::transmute_int_to_non_zero)]
+
+use std::num::NonZeroU8;
+
+#[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));
+}
+
+#[rustc_layout_scalar_valid_range_end(254)]
+struct NonMaxU8(u8);
+#[rustc_layout_scalar_valid_range_end(254)]
+#[rustc_layout_scalar_valid_range_start(1)]
+struct NonZeroNonMaxU8(u8);
+
+macro_rules! impls {
+    ($($t:ty),*) => {
+        $(
+            impl PartialEq<u8> for $t {
+                fn eq(&self, other: &u8) -> bool {
+                    self.0 == *other
+                }
+            }
+            impl PartialOrd<u8> for $t {
+                fn partial_cmp(&self, other: &u8) -> Option<std::cmp::Ordering> {
+                    self.0.partial_cmp(other)
+                }
+            }
+        )*
+    };
+}
+impls!(NonMaxU8, NonZeroNonMaxU8);
+
+fn niche_tests(v1: u8, v2: NonZeroU8, v3: NonZeroNonMaxU8) {
+    // u8 -> NonZeroU8, do lint
+    let _: Option<NonZeroU8> = (v1 > 0).then_some(unsafe { std::mem::transmute(v1) });
+
+    // NonZeroU8 -> u8, don't lint, target type has no niche and therefore a higher validity range
+    let _: Option<u8> = (v2 > NonZeroU8::new(1).unwrap()).then_some(unsafe { std::mem::transmute(v2) });
+
+    // NonZeroU8 -> NonMaxU8, do lint, different niche
+    let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) });
+
+    // NonZeroNonMaxU8 -> NonMaxU8, don't lint, target type has more validity
+    let _: Option<NonMaxU8> = (v3 < 255).then_some(unsafe { std::mem::transmute(v2) });
+
+    // NonZeroU8 -> NonZeroNonMaxU8, do lint, target type has less validity
+    let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) });
+}
+
+fn main() {}