diff options
| -rw-r--r-- | clippy_lints/src/transmute/eager_transmute.rs | 53 | ||||
| -rw-r--r-- | tests/ui/eager_transmute.fixed | 35 | ||||
| -rw-r--r-- | tests/ui/eager_transmute.rs | 35 | ||||
| -rw-r--r-- | tests/ui/eager_transmute.stderr | 117 |
4 files changed, 225 insertions, 15 deletions
diff --git a/clippy_lints/src/transmute/eager_transmute.rs b/clippy_lints/src/transmute/eager_transmute.rs index 01a23c515f5..c44f5150dd1 100644 --- a/clippy_lints/src/transmute/eager_transmute.rs +++ b/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 diff --git a/tests/ui/eager_transmute.fixed b/tests/ui/eager_transmute.fixed index e06aa2cc9fd..bece09bba1a 100644 --- a/tests/ui/eager_transmute.fixed +++ b/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) { diff --git a/tests/ui/eager_transmute.rs b/tests/ui/eager_transmute.rs index 89ccdf583f3..a82bd578f76 100644 --- a/tests/ui/eager_transmute.rs +++ b/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) { diff --git a/tests/ui/eager_transmute.stderr b/tests/ui/eager_transmute.stderr index 5eb163c5fcb..5f42eec544f 100644 --- a/tests/ui/eager_transmute.stderr +++ b/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 |