diff --git a/clippy_utils/src/eager_or_lazy.rs b/clippy_utils/src/eager_or_lazy.rs index 9a48e772774..371fc029701 100644 --- a/clippy_utils/src/eager_or_lazy.rs +++ b/clippy_utils/src/eager_or_lazy.rs @@ -9,7 +9,7 @@ //! - or-fun-call //! - option-if-let-else -use crate::consts::{constant, FullInt}; +use crate::consts::constant; use crate::ty::{all_predicates_of, is_copy}; use crate::visitors::is_const_evaluatable; use rustc_hir::def::{DefKind, Res}; @@ -195,8 +195,8 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS } }, - // `-i32::MIN` panics with overflow checks, but `constant` lets us rule out some simple cases - ExprKind::Unary(UnOp::Neg, _) if constant(self.cx, self.cx.typeck_results(), e).is_none() => { + // `-i32::MIN` panics with overflow checks + ExprKind::Unary(UnOp::Neg, right) if constant(self.cx, self.cx.typeck_results(), right).is_none() => { self.eagerness |= NoChange; }, @@ -216,31 +216,28 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS ) => {}, // `>>` and `<<` panic when the right-hand side is greater than or equal to the number of bits in the - // type of the left-hand side, or is negative. Doesn't need `constant` on the whole expression - // because only the right side matters. - ExprKind::Binary(op, left, right) + // type of the left-hand side, or is negative. + // We intentionally only check if the right-hand isn't a constant, because even if the suggestion would + // overflow with constants, the compiler emits an error for it and the programmer will have to fix it. + // Thus, we would realistically only delay the lint. + ExprKind::Binary(op, _, right) if matches!(op.node, BinOpKind::Shl | BinOpKind::Shr) - && let left_ty = self.cx.typeck_results().expr_ty(left) - && let left_bits = left_ty.int_size_and_signed(self.cx.tcx).0.bits() - && constant(self.cx, self.cx.typeck_results(), right) - .and_then(|c| c.int_value(self.cx, left_ty)) - .map_or(true, |c| match c { - FullInt::S(i) => i >= i128::from(left_bits) || i.is_negative(), - FullInt::U(i) => i >= u128::from(left_bits), - }) => + && constant(self.cx, self.cx.typeck_results(), right).is_none() => { self.eagerness |= NoChange; }, - // Arithmetic operations panic on under-/overflow with overflow checks, so don't suggest changing it: - // https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#overflow - // Using `constant` lets us allow some simple cases where we know for sure it can't overflow - ExprKind::Binary(op, ..) + // Similar to `>>` and `<<`, we only want to avoid linting entirely if both sides are unknown and the + // compiler can't emit an error for an overflowing expression. + // Suggesting eagerness for `true.then(|| i32::MAX + 1)` is okay because the compiler will emit an + // error and it's good to have the eagerness warning up front when the user fixes the logic error. + ExprKind::Binary(op, left, right) if matches!( op.node, - BinOpKind::Add | BinOpKind::Mul | BinOpKind::Sub | BinOpKind::Div | BinOpKind::Rem + BinOpKind::Add | BinOpKind::Sub | BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem ) && !self.cx.typeck_results().expr_ty(e).is_floating_point() - && constant(self.cx, self.cx.typeck_results(), e).is_none() => + && (constant(self.cx, self.cx.typeck_results(), left).is_none() + || constant(self.cx, self.cx.typeck_results(), right).is_none()) => { self.eagerness |= NoChange; }, diff --git a/tests/ui/unnecessary_lazy_eval.fixed b/tests/ui/unnecessary_lazy_eval.fixed index da5da9ef85f..07b43d3cc1d 100644 --- a/tests/ui/unnecessary_lazy_eval.fixed +++ b/tests/ui/unnecessary_lazy_eval.fixed @@ -185,9 +185,6 @@ fn main() { // neither bind_instead_of_map nor unnecessary_lazy_eval applies here let _: Result = res.and_then(|x| Err(x)); let _: Result = res.or_else(|err| Ok(err)); - - issue9422(3); - panicky_arithmetic_ops(3); } #[allow(unused)] @@ -201,33 +198,58 @@ fn issue9422(x: usize) -> Option { // (x >= 5).then_some(x - 5) // clippy suggestion panics } -// https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#overflow -fn panicky_arithmetic_ops(x: usize) { - let _x = false.then(|| i32::MAX + 1); - let _x = false.then(|| i32::MAX * 2); +fn panicky_arithmetic_ops(x: usize, y: isize) { + #![allow(clippy::identity_op, clippy::eq_op)] + + // Even though some of these expressions overflow, they're entirely dependent on constants. + // So, the compiler already emits a warning about overflowing expressions. + // It's a logic error and we want both warnings up front. + // ONLY when a binop side that "matters" for overflow (for `>>`, that is always the right side and + // never the left side) has a non-constant value, avoid linting + + let _x = false.then_some(i32::MAX + 1); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then_some(i32::MAX * 2); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then_some(i32::MAX - 1); //~^ ERROR: unnecessary closure used with `bool::then` - let _x = false.then(|| i32::MIN - 1); - #[allow(clippy::identity_op)] + let _x = false.then_some(i32::MIN - 1); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then_some((1 + 2 * 3 - 2 / 3 + 9) << 2); //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then_some(255u8 << 7); //~^ ERROR: unnecessary closure used with `bool::then` - let _x = false.then(|| 255u8 << 8); - let _x = false.then(|| 255u8 >> 8); + let _x = false.then_some(255u8 << 8); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then_some(255u8 >> 8); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| 255u8 >> x); - let _x = false.then(|| i32::MIN / -1); + let _x = false.then_some(i32::MIN / -1); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then_some(i32::MAX + -1); //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then_some(-i32::MAX); //~^ ERROR: unnecessary closure used with `bool::then` - let _x = false.then(|| -i32::MIN); - let _x = false.then(|| 255 >> -7); - let _x = false.then(|| 255 << -1); - let _x = false.then(|| 1 / 0); - let _x = false.then(|| x << -1); + let _x = false.then_some(-i32::MIN); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| -y); + let _x = false.then_some(255 >> -7); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then_some(255 << -1); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then_some(1 / 0); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then_some(x << -1); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then_some(x << 2); //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| x + x); + let _x = false.then(|| x * x); + let _x = false.then(|| x - x); + let _x = false.then(|| x / x); + let _x = false.then(|| x % x); + let _x = false.then(|| x + 1); + let _x = false.then(|| 1 + x); // const eval doesn't read variables, but floating point math never panics, so we can still emit a // warning diff --git a/tests/ui/unnecessary_lazy_eval.rs b/tests/ui/unnecessary_lazy_eval.rs index 18b7eb4760f..232a94d9638 100644 --- a/tests/ui/unnecessary_lazy_eval.rs +++ b/tests/ui/unnecessary_lazy_eval.rs @@ -185,9 +185,6 @@ fn main() { // neither bind_instead_of_map nor unnecessary_lazy_eval applies here let _: Result = res.and_then(|x| Err(x)); let _: Result = res.or_else(|err| Ok(err)); - - issue9422(3); - panicky_arithmetic_ops(3); } #[allow(unused)] @@ -201,33 +198,58 @@ fn issue9422(x: usize) -> Option { // (x >= 5).then_some(x - 5) // clippy suggestion panics } -// https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#overflow -fn panicky_arithmetic_ops(x: usize) { +fn panicky_arithmetic_ops(x: usize, y: isize) { + #![allow(clippy::identity_op, clippy::eq_op)] + + // Even though some of these expressions overflow, they're entirely dependent on constants. + // So, the compiler already emits a warning about overflowing expressions. + // It's a logic error and we want both warnings up front. + // ONLY when a binop side that "matters" for overflow (for `>>`, that is always the right side and + // never the left side) has a non-constant value, avoid linting + let _x = false.then(|| i32::MAX + 1); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| i32::MAX * 2); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| i32::MAX - 1); //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| i32::MIN - 1); - #[allow(clippy::identity_op)] + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| (1 + 2 * 3 - 2 / 3 + 9) << 2); //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| 255u8 << 7); //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| 255u8 << 8); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| 255u8 >> 8); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| 255u8 >> x); let _x = false.then(|| i32::MIN / -1); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| i32::MAX + -1); //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| -i32::MAX); //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| -i32::MIN); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| -y); let _x = false.then(|| 255 >> -7); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| 255 << -1); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| 1 / 0); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| x << -1); + //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| x << 2); //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| x + x); + let _x = false.then(|| x * x); + let _x = false.then(|| x - x); + let _x = false.then(|| x / x); + let _x = false.then(|| x % x); + let _x = false.then(|| x + 1); + let _x = false.then(|| 1 + x); // const eval doesn't read variables, but floating point math never panics, so we can still emit a // warning diff --git a/tests/ui/unnecessary_lazy_eval.stderr b/tests/ui/unnecessary_lazy_eval.stderr index 55cbed7a415..6c7edb24fae 100644 --- a/tests/ui/unnecessary_lazy_eval.stderr +++ b/tests/ui/unnecessary_lazy_eval.stderr @@ -329,7 +329,23 @@ LL | | or_else(|_| Ok(ext_str.some_field)); | help: use `or(..)` instead: `or(Ok(ext_str.some_field))` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:208:14 + --> $DIR/unnecessary_lazy_eval.rs:210:14 + | +LL | let _x = false.then(|| i32::MAX + 1); + | ^^^^^^--------------------- + | | + | help: use `then_some(..)` instead: `then_some(i32::MAX + 1)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:212:14 + | +LL | let _x = false.then(|| i32::MAX * 2); + | ^^^^^^--------------------- + | | + | help: use `then_some(..)` instead: `then_some(i32::MAX * 2)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:214:14 | LL | let _x = false.then(|| i32::MAX - 1); | ^^^^^^--------------------- @@ -337,7 +353,15 @@ LL | let _x = false.then(|| i32::MAX - 1); | help: use `then_some(..)` instead: `then_some(i32::MAX - 1)` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:212:14 + --> $DIR/unnecessary_lazy_eval.rs:216:14 + | +LL | let _x = false.then(|| i32::MIN - 1); + | ^^^^^^--------------------- + | | + | help: use `then_some(..)` instead: `then_some(i32::MIN - 1)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:218:14 | LL | let _x = false.then(|| (1 + 2 * 3 - 2 / 3 + 9) << 2); | ^^^^^^------------------------------------- @@ -345,7 +369,7 @@ LL | let _x = false.then(|| (1 + 2 * 3 - 2 / 3 + 9) << 2); | help: use `then_some(..)` instead: `then_some((1 + 2 * 3 - 2 / 3 + 9) << 2)` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:214:14 + --> $DIR/unnecessary_lazy_eval.rs:220:14 | LL | let _x = false.then(|| 255u8 << 7); | ^^^^^^------------------- @@ -353,7 +377,31 @@ LL | let _x = false.then(|| 255u8 << 7); | help: use `then_some(..)` instead: `then_some(255u8 << 7)` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:220:14 + --> $DIR/unnecessary_lazy_eval.rs:222:14 + | +LL | let _x = false.then(|| 255u8 << 8); + | ^^^^^^------------------- + | | + | help: use `then_some(..)` instead: `then_some(255u8 << 8)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:224:14 + | +LL | let _x = false.then(|| 255u8 >> 8); + | ^^^^^^------------------- + | | + | help: use `then_some(..)` instead: `then_some(255u8 >> 8)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:227:14 + | +LL | let _x = false.then(|| i32::MIN / -1); + | ^^^^^^---------------------- + | | + | help: use `then_some(..)` instead: `then_some(i32::MIN / -1)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:229:14 | LL | let _x = false.then(|| i32::MAX + -1); | ^^^^^^---------------------- @@ -361,7 +409,7 @@ LL | let _x = false.then(|| i32::MAX + -1); | help: use `then_some(..)` instead: `then_some(i32::MAX + -1)` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:222:14 + --> $DIR/unnecessary_lazy_eval.rs:231:14 | LL | let _x = false.then(|| -i32::MAX); | ^^^^^^------------------ @@ -369,7 +417,47 @@ LL | let _x = false.then(|| -i32::MAX); | help: use `then_some(..)` instead: `then_some(-i32::MAX)` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:229:14 + --> $DIR/unnecessary_lazy_eval.rs:233:14 + | +LL | let _x = false.then(|| -i32::MIN); + | ^^^^^^------------------ + | | + | help: use `then_some(..)` instead: `then_some(-i32::MIN)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:236:14 + | +LL | let _x = false.then(|| 255 >> -7); + | ^^^^^^------------------ + | | + | help: use `then_some(..)` instead: `then_some(255 >> -7)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:238:14 + | +LL | let _x = false.then(|| 255 << -1); + | ^^^^^^------------------ + | | + | help: use `then_some(..)` instead: `then_some(255 << -1)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:240:14 + | +LL | let _x = false.then(|| 1 / 0); + | ^^^^^^-------------- + | | + | help: use `then_some(..)` instead: `then_some(1 / 0)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:242:14 + | +LL | let _x = false.then(|| x << -1); + | ^^^^^^---------------- + | | + | help: use `then_some(..)` instead: `then_some(x << -1)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:244:14 | LL | let _x = false.then(|| x << 2); | ^^^^^^--------------- @@ -377,12 +465,12 @@ LL | let _x = false.then(|| x << 2); | help: use `then_some(..)` instead: `then_some(x << 2)` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:236:14 + --> $DIR/unnecessary_lazy_eval.rs:258:14 | LL | let _x = false.then(|| f1 + f2); | ^^^^^^---------------- | | | help: use `then_some(..)` instead: `then_some(f1 + f2)` -error: aborting due to 47 previous errors +error: aborting due to 58 previous errors