From 7696c9d1d506933dd4da46115dd0fd713fed6df4 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 16 Nov 2023 17:30:10 +0100 Subject: [PATCH] allow more div and rem operations that can be checked --- clippy_utils/src/eager_or_lazy.rs | 31 +++++++-- tests/ui/unnecessary_lazy_eval.fixed | 24 ++++--- tests/ui/unnecessary_lazy_eval.rs | 24 ++++--- tests/ui/unnecessary_lazy_eval.stderr | 90 +++++++++++++++++++-------- 4 files changed, 124 insertions(+), 45 deletions(-) diff --git a/clippy_utils/src/eager_or_lazy.rs b/clippy_utils/src/eager_or_lazy.rs index 371fc029701..f7a8bd131eb 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; +use crate::consts::{constant, FullInt}; use crate::ty::{all_predicates_of, is_copy}; use crate::visitors::is_const_evaluatable; use rustc_hir::def::{DefKind, Res}; @@ -227,15 +227,34 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS self.eagerness |= NoChange; }, - // Similar to `>>` and `<<`, we only want to avoid linting entirely if both sides are unknown and the + ExprKind::Binary(op, left, right) + if matches!(op.node, BinOpKind::Div | BinOpKind::Rem) + && let left_ty = self.cx.typeck_results().expr_ty(left) + && let right_ty = self.cx.typeck_results().expr_ty(right) + && let left = constant(self.cx, self.cx.typeck_results(), left) + .and_then(|c| c.int_value(self.cx, left_ty)) + && let right = constant(self.cx, self.cx.typeck_results(), right) + .and_then(|c| c.int_value(self.cx, right_ty)) + && match (left, right) { + // `1 / x` -- x might be zero + (_, None) => true, + // `x / -1` -- x might be T::MIN = panic + #[expect(clippy::match_same_arms)] + (None, Some(FullInt::S(-1))) => true, + // anything else is either fine or checked by the compiler + _ => false, + } => + { + self.eagerness |= NoChange; + }, + + // Similar to `>>` and `<<`, we only want to avoid linting entirely if either side is 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::Sub | BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem - ) && !self.cx.typeck_results().expr_ty(e).is_floating_point() + if matches!(op.node, BinOpKind::Add | BinOpKind::Sub | BinOpKind::Mul) + && !self.cx.typeck_results().expr_ty(e).is_floating_point() && (constant(self.cx, self.cx.typeck_results(), left).is_none() || constant(self.cx, self.cx.typeck_results(), right).is_none()) => { diff --git a/tests/ui/unnecessary_lazy_eval.fixed b/tests/ui/unnecessary_lazy_eval.fixed index 07b43d3cc1d..66598f89208 100644 --- a/tests/ui/unnecessary_lazy_eval.fixed +++ b/tests/ui/unnecessary_lazy_eval.fixed @@ -201,11 +201,7 @@ fn issue9422(x: usize) -> Option { 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 + // See comments in `eager_or_lazy.rs` for the rules that this is meant to follow let _x = false.then_some(i32::MAX + 1); //~^ ERROR: unnecessary closure used with `bool::then` @@ -224,8 +220,6 @@ fn panicky_arithmetic_ops(x: usize, y: isize) { let _x = false.then_some(255u8 >> 8); //~^ ERROR: unnecessary closure used with `bool::then` let _x = false.then(|| 255u8 >> x); - 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); @@ -251,6 +245,22 @@ fn panicky_arithmetic_ops(x: usize, y: isize) { let _x = false.then(|| x + 1); let _x = false.then(|| 1 + x); + let _x = false.then_some(x / 0); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then_some(x % 0); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| y / -1); + let _x = false.then_some(1 / -1); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then_some(i32::MIN / -1); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| i32::MIN / x as i32); + let _x = false.then_some(i32::MIN / 0); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then_some(4 / 2); + //~^ ERROR: unnecessary closure used with `bool::then` + 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 let f1 = 1.0; diff --git a/tests/ui/unnecessary_lazy_eval.rs b/tests/ui/unnecessary_lazy_eval.rs index 232a94d9638..5045fcd790e 100644 --- a/tests/ui/unnecessary_lazy_eval.rs +++ b/tests/ui/unnecessary_lazy_eval.rs @@ -201,11 +201,7 @@ fn issue9422(x: usize) -> Option { 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 + // See comments in `eager_or_lazy.rs` for the rules that this is meant to follow let _x = false.then(|| i32::MAX + 1); //~^ ERROR: unnecessary closure used with `bool::then` @@ -224,8 +220,6 @@ fn panicky_arithmetic_ops(x: usize, y: isize) { 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); @@ -251,6 +245,22 @@ fn panicky_arithmetic_ops(x: usize, y: isize) { let _x = false.then(|| x + 1); let _x = false.then(|| 1 + x); + let _x = false.then(|| x / 0); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| x % 0); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| y / -1); + let _x = false.then(|| 1 / -1); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| i32::MIN / -1); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| i32::MIN / x as i32); + let _x = false.then(|| i32::MIN / 0); + //~^ ERROR: unnecessary closure used with `bool::then` + let _x = false.then(|| 4 / 2); + //~^ ERROR: unnecessary closure used with `bool::then` + 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 let f1 = 1.0; diff --git a/tests/ui/unnecessary_lazy_eval.stderr b/tests/ui/unnecessary_lazy_eval.stderr index 6c7edb24fae..466664aee6c 100644 --- a/tests/ui/unnecessary_lazy_eval.stderr +++ b/tests/ui/unnecessary_lazy_eval.stderr @@ -329,7 +329,7 @@ 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:210:14 + --> $DIR/unnecessary_lazy_eval.rs:206:14 | LL | let _x = false.then(|| i32::MAX + 1); | ^^^^^^--------------------- @@ -337,7 +337,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:212:14 + --> $DIR/unnecessary_lazy_eval.rs:208:14 | LL | let _x = false.then(|| i32::MAX * 2); | ^^^^^^--------------------- @@ -345,7 +345,7 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:210:14 | LL | let _x = false.then(|| i32::MAX - 1); | ^^^^^^--------------------- @@ -353,7 +353,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:216:14 + --> $DIR/unnecessary_lazy_eval.rs:212:14 | LL | let _x = false.then(|| i32::MIN - 1); | ^^^^^^--------------------- @@ -361,7 +361,7 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:214:14 | LL | let _x = false.then(|| (1 + 2 * 3 - 2 / 3 + 9) << 2); | ^^^^^^------------------------------------- @@ -369,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:220:14 + --> $DIR/unnecessary_lazy_eval.rs:216:14 | LL | let _x = false.then(|| 255u8 << 7); | ^^^^^^------------------- @@ -377,7 +377,7 @@ 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:222:14 + --> $DIR/unnecessary_lazy_eval.rs:218:14 | LL | let _x = false.then(|| 255u8 << 8); | ^^^^^^------------------- @@ -385,7 +385,7 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:220:14 | LL | let _x = false.then(|| 255u8 >> 8); | ^^^^^^------------------- @@ -393,15 +393,7 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:223:14 | LL | let _x = false.then(|| i32::MAX + -1); | ^^^^^^---------------------- @@ -409,7 +401,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:231:14 + --> $DIR/unnecessary_lazy_eval.rs:225:14 | LL | let _x = false.then(|| -i32::MAX); | ^^^^^^------------------ @@ -417,7 +409,7 @@ 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:233:14 + --> $DIR/unnecessary_lazy_eval.rs:227:14 | LL | let _x = false.then(|| -i32::MIN); | ^^^^^^------------------ @@ -425,7 +417,7 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:230:14 | LL | let _x = false.then(|| 255 >> -7); | ^^^^^^------------------ @@ -433,7 +425,7 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:232:14 | LL | let _x = false.then(|| 255 << -1); | ^^^^^^------------------ @@ -441,7 +433,7 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:234:14 | LL | let _x = false.then(|| 1 / 0); | ^^^^^^-------------- @@ -449,7 +441,7 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:236:14 | LL | let _x = false.then(|| x << -1); | ^^^^^^---------------- @@ -457,20 +449,68 @@ 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 + --> $DIR/unnecessary_lazy_eval.rs:238:14 | 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:248:14 + | +LL | let _x = false.then(|| x / 0); + | ^^^^^^-------------- + | | + | help: use `then_some(..)` instead: `then_some(x / 0)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:250:14 + | +LL | let _x = false.then(|| x % 0); + | ^^^^^^-------------- + | | + | help: use `then_some(..)` instead: `then_some(x % 0)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:253:14 + | +LL | let _x = false.then(|| 1 / -1); + | ^^^^^^--------------- + | | + | help: use `then_some(..)` instead: `then_some(1 / -1)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:255: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:258:14 | +LL | let _x = false.then(|| i32::MIN / 0); + | ^^^^^^--------------------- + | | + | help: use `then_some(..)` instead: `then_some(i32::MIN / 0)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:260:14 + | +LL | let _x = false.then(|| 4 / 2); + | ^^^^^^-------------- + | | + | help: use `then_some(..)` instead: `then_some(4 / 2)` + +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:268:14 + | LL | let _x = false.then(|| f1 + f2); | ^^^^^^---------------- | | | help: use `then_some(..)` instead: `then_some(f1 + f2)` -error: aborting due to 58 previous errors +error: aborting due to 63 previous errors