From c9d2961100317b6ee890fe9116ff3750e6beb6fe Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 22 Jun 2023 00:56:15 +0200 Subject: [PATCH 1/5] teach `eager_or_lazy` that arithmetic ops can panic --- clippy_utils/src/consts.rs | 132 +++++++++++++++++++----- clippy_utils/src/eager_or_lazy.rs | 40 +++++++- tests/ui/unnecessary_lazy_eval.fixed | 46 +++++++++ tests/ui/unnecessary_lazy_eval.rs | 46 +++++++++ tests/ui/unnecessary_lazy_eval.stderr | 138 ++++++++++++++++++-------- 5 files changed, 336 insertions(+), 66 deletions(-) diff --git a/clippy_utils/src/consts.rs b/clippy_utils/src/consts.rs index b7d8c0a8fd73..dc64ba3ee10d 100644 --- a/clippy_utils/src/consts.rs +++ b/clippy_utils/src/consts.rs @@ -10,7 +10,7 @@ use rustc_hir::{BinOp, BinOpKind, Block, ConstBlock, Expr, ExprKind, HirId, Item use rustc_lexer::tokenize; use rustc_lint::LateContext; use rustc_middle::mir::interpret::{alloc_range, Scalar}; -use rustc_middle::ty::{self, EarlyBinder, FloatTy, GenericArgsRef, List, ScalarInt, Ty, TyCtxt}; +use rustc_middle::ty::{self, EarlyBinder, FloatTy, GenericArgsRef, IntTy, List, ScalarInt, Ty, TyCtxt, UintTy}; use rustc_middle::{bug, mir, span_bug}; use rustc_span::symbol::{Ident, Symbol}; use rustc_span::SyntaxContext; @@ -51,6 +51,63 @@ pub enum Constant<'tcx> { Err, } +trait IntTypeBounds: Sized { + type Output: PartialOrd; + + fn min_max(self) -> Option<(Self::Output, Self::Output)>; + fn bits(self) -> Self::Output; + fn ensure_fits(self, val: Self::Output) -> Option { + let (min, max) = self.min_max()?; + (min <= val && val <= max).then_some(val) + } +} +impl IntTypeBounds for UintTy { + type Output = u128; + fn min_max(self) -> Option<(Self::Output, Self::Output)> { + Some(match self { + UintTy::U8 => (u8::MIN.into(), u8::MAX.into()), + UintTy::U16 => (u16::MIN.into(), u16::MAX.into()), + UintTy::U32 => (u32::MIN.into(), u32::MAX.into()), + UintTy::U64 => (u64::MIN.into(), u64::MAX.into()), + UintTy::U128 => (u128::MIN, u128::MAX), + UintTy::Usize => (usize::MIN.try_into().ok()?, usize::MAX.try_into().ok()?), + }) + } + fn bits(self) -> Self::Output { + match self { + UintTy::U8 => 8, + UintTy::U16 => 16, + UintTy::U32 => 32, + UintTy::U64 => 64, + UintTy::U128 => 128, + UintTy::Usize => usize::BITS.into(), + } + } +} +impl IntTypeBounds for IntTy { + type Output = i128; + fn min_max(self) -> Option<(Self::Output, Self::Output)> { + Some(match self { + IntTy::I8 => (i8::MIN.into(), i8::MAX.into()), + IntTy::I16 => (i16::MIN.into(), i16::MAX.into()), + IntTy::I32 => (i32::MIN.into(), i32::MAX.into()), + IntTy::I64 => (i64::MIN.into(), i64::MAX.into()), + IntTy::I128 => (i128::MIN, i128::MAX), + IntTy::Isize => (isize::MIN.try_into().ok()?, isize::MAX.try_into().ok()?), + }) + } + fn bits(self) -> Self::Output { + match self { + IntTy::I8 => 8, + IntTy::I16 => 16, + IntTy::I32 => 32, + IntTy::I64 => 64, + IntTy::I128 => 128, + IntTy::Isize => isize::BITS.into(), + } + } +} + impl<'tcx> PartialEq for Constant<'tcx> { fn eq(&self, other: &Self) -> bool { match (self, other) { @@ -435,8 +492,15 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> { match *o { Int(value) => { let ty::Int(ity) = *ty.kind() else { return None }; + let (min, _) = ity.min_max()?; // sign extend let value = sext(self.lcx.tcx, value, ity); + + // Applying unary - to the most negative value of any signed integer type panics. + if value == min { + return None; + } + let value = value.checked_neg()?; // clear unused bits Some(Int(unsext(self.lcx.tcx, value, ity))) @@ -572,17 +636,33 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> { match (l, r) { (Constant::Int(l), Some(Constant::Int(r))) => match *self.typeck_results.expr_ty_opt(left)?.kind() { ty::Int(ity) => { + let (ty_min_value, _) = ity.min_max()?; + let bits = ity.bits(); let l = sext(self.lcx.tcx, l, ity); let r = sext(self.lcx.tcx, r, ity); + + // Using / or %, where the left-hand argument is the smallest integer of a signed integer type and + // the right-hand argument is -1 always panics, even with overflow-checks disabled + if let BinOpKind::Div | BinOpKind::Rem = op.node + && l == ty_min_value + && r == -1 + { + return None; + } + let zext = |n: i128| Constant::Int(unsext(self.lcx.tcx, n, ity)); match op.node { - BinOpKind::Add => l.checked_add(r).map(zext), - BinOpKind::Sub => l.checked_sub(r).map(zext), - BinOpKind::Mul => l.checked_mul(r).map(zext), + // When +, * or binary - create a value greater than the maximum value, or less than + // the minimum value that can be stored, it panics. + BinOpKind::Add => l.checked_add(r).and_then(|n| ity.ensure_fits(n)).map(zext), + BinOpKind::Sub => l.checked_sub(r).and_then(|n| ity.ensure_fits(n)).map(zext), + BinOpKind::Mul => l.checked_mul(r).and_then(|n| ity.ensure_fits(n)).map(zext), BinOpKind::Div if r != 0 => l.checked_div(r).map(zext), BinOpKind::Rem if r != 0 => l.checked_rem(r).map(zext), - BinOpKind::Shr => l.checked_shr(r.try_into().ok()?).map(zext), - BinOpKind::Shl => l.checked_shl(r.try_into().ok()?).map(zext), + // Using << or >> where the right-hand argument is greater than or equal to the number of bits + // in the type of the left-hand argument, or is negative panics. + BinOpKind::Shr if r < bits && !r.is_negative() => l.checked_shr(r.try_into().ok()?).map(zext), + BinOpKind::Shl if r < bits && !r.is_negative() => l.checked_shl(r.try_into().ok()?).map(zext), BinOpKind::BitXor => Some(zext(l ^ r)), BinOpKind::BitOr => Some(zext(l | r)), BinOpKind::BitAnd => Some(zext(l & r)), @@ -595,24 +675,28 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> { _ => None, } }, - ty::Uint(_) => match op.node { - BinOpKind::Add => l.checked_add(r).map(Constant::Int), - BinOpKind::Sub => l.checked_sub(r).map(Constant::Int), - BinOpKind::Mul => l.checked_mul(r).map(Constant::Int), - BinOpKind::Div => l.checked_div(r).map(Constant::Int), - BinOpKind::Rem => l.checked_rem(r).map(Constant::Int), - BinOpKind::Shr => l.checked_shr(r.try_into().ok()?).map(Constant::Int), - BinOpKind::Shl => l.checked_shl(r.try_into().ok()?).map(Constant::Int), - BinOpKind::BitXor => Some(Constant::Int(l ^ r)), - BinOpKind::BitOr => Some(Constant::Int(l | r)), - BinOpKind::BitAnd => Some(Constant::Int(l & r)), - BinOpKind::Eq => Some(Constant::Bool(l == r)), - BinOpKind::Ne => Some(Constant::Bool(l != r)), - BinOpKind::Lt => Some(Constant::Bool(l < r)), - BinOpKind::Le => Some(Constant::Bool(l <= r)), - BinOpKind::Ge => Some(Constant::Bool(l >= r)), - BinOpKind::Gt => Some(Constant::Bool(l > r)), - _ => None, + ty::Uint(ity) => { + let bits = ity.bits(); + + match op.node { + BinOpKind::Add => l.checked_add(r).and_then(|n| ity.ensure_fits(n)).map(Constant::Int), + BinOpKind::Sub => l.checked_sub(r).and_then(|n| ity.ensure_fits(n)).map(Constant::Int), + BinOpKind::Mul => l.checked_mul(r).and_then(|n| ity.ensure_fits(n)).map(Constant::Int), + BinOpKind::Div => l.checked_div(r).map(Constant::Int), + BinOpKind::Rem => l.checked_rem(r).map(Constant::Int), + BinOpKind::Shr if r < bits => l.checked_shr(r.try_into().ok()?).map(Constant::Int), + BinOpKind::Shl if r < bits => l.checked_shl(r.try_into().ok()?).map(Constant::Int), + BinOpKind::BitXor => Some(Constant::Int(l ^ r)), + BinOpKind::BitOr => Some(Constant::Int(l | r)), + BinOpKind::BitAnd => Some(Constant::Int(l & r)), + BinOpKind::Eq => Some(Constant::Bool(l == r)), + BinOpKind::Ne => Some(Constant::Bool(l != r)), + BinOpKind::Lt => Some(Constant::Bool(l < r)), + BinOpKind::Le => Some(Constant::Bool(l <= r)), + BinOpKind::Ge => Some(Constant::Bool(l >= r)), + BinOpKind::Gt => Some(Constant::Bool(l > r)), + _ => None, + } }, _ => None, }, diff --git a/clippy_utils/src/eager_or_lazy.rs b/clippy_utils/src/eager_or_lazy.rs index 0bcefba75a7b..9a48e7727748 100644 --- a/clippy_utils/src/eager_or_lazy.rs +++ b/clippy_utils/src/eager_or_lazy.rs @@ -9,12 +9,13 @@ //! - or-fun-call //! - option-if-let-else +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}; use rustc_hir::def_id::DefId; use rustc_hir::intravisit::{walk_expr, Visitor}; -use rustc_hir::{Block, Expr, ExprKind, QPath, UnOp}; +use rustc_hir::{BinOpKind, Block, Expr, ExprKind, QPath, UnOp}; use rustc_lint::LateContext; use rustc_middle::ty; use rustc_middle::ty::adjustment::Adjust; @@ -193,6 +194,12 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS self.eagerness = Lazy; } }, + + // `-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() => { + self.eagerness |= NoChange; + }, + // Custom `Deref` impl might have side effects ExprKind::Unary(UnOp::Deref, e) if self.cx.typeck_results().expr_ty(e).builtin_deref(true).is_none() => @@ -207,6 +214,37 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS self.cx.typeck_results().expr_ty(e).kind(), ty::Bool | ty::Int(_) | ty::Uint(_), ) => {}, + + // `>>` 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) + 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), + }) => + { + 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, ..) + if matches!( + op.node, + BinOpKind::Add | BinOpKind::Mul | BinOpKind::Sub | BinOpKind::Div | BinOpKind::Rem + ) && !self.cx.typeck_results().expr_ty(e).is_floating_point() + && constant(self.cx, self.cx.typeck_results(), e).is_none() => + { + self.eagerness |= NoChange; + }, + ExprKind::Binary(_, lhs, rhs) if self.cx.typeck_results().expr_ty(lhs).is_primitive() && self.cx.typeck_results().expr_ty(rhs).is_primitive() => {}, diff --git a/tests/ui/unnecessary_lazy_eval.fixed b/tests/ui/unnecessary_lazy_eval.fixed index 4778eaefdbdb..da5da9ef85f0 100644 --- a/tests/ui/unnecessary_lazy_eval.fixed +++ b/tests/ui/unnecessary_lazy_eval.fixed @@ -6,6 +6,8 @@ #![allow(clippy::needless_borrow)] #![allow(clippy::unnecessary_literal_unwrap)] #![allow(clippy::unit_arg)] +#![allow(arithmetic_overflow)] +#![allow(unconditional_panic)] use std::ops::Deref; @@ -183,6 +185,9 @@ 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)] @@ -190,3 +195,44 @@ fn issue9485() { // should not lint, is in proc macro with_span!(span Some(42).unwrap_or_else(|| 2);); } + +fn issue9422(x: usize) -> Option { + (x >= 5).then(|| x - 5) + // (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); + 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((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(|| 255u8 >> x); + let _x = false.then(|| i32::MIN / -1); + 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(x << 2); + //~^ ERROR: unnecessary closure used with `bool::then` + + // const eval doesn't read variables, but floating point math never panics, so we can still emit a + // warning + let f1 = 1.0; + let f2 = 2.0; + let _x = false.then_some(f1 + f2); + //~^ ERROR: unnecessary closure used with `bool::then` +} diff --git a/tests/ui/unnecessary_lazy_eval.rs b/tests/ui/unnecessary_lazy_eval.rs index d4b7fd31b1b0..18b7eb4760fb 100644 --- a/tests/ui/unnecessary_lazy_eval.rs +++ b/tests/ui/unnecessary_lazy_eval.rs @@ -6,6 +6,8 @@ #![allow(clippy::needless_borrow)] #![allow(clippy::unnecessary_literal_unwrap)] #![allow(clippy::unit_arg)] +#![allow(arithmetic_overflow)] +#![allow(unconditional_panic)] use std::ops::Deref; @@ -183,6 +185,9 @@ 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)] @@ -190,3 +195,44 @@ fn issue9485() { // should not lint, is in proc macro with_span!(span Some(42).unwrap_or_else(|| 2);); } + +fn issue9422(x: usize) -> Option { + (x >= 5).then(|| x - 5) + // (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); + 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)] + 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); + let _x = false.then(|| 255u8 >> 8); + let _x = false.then(|| 255u8 >> x); + let _x = false.then(|| i32::MIN / -1); + 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); + 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(|| x << 2); + //~^ ERROR: unnecessary closure used with `bool::then` + + // const eval doesn't read variables, but floating point math never panics, so we can still emit a + // warning + let f1 = 1.0; + let f2 = 2.0; + let _x = false.then(|| f1 + f2); + //~^ ERROR: unnecessary closure used with `bool::then` +} diff --git a/tests/ui/unnecessary_lazy_eval.stderr b/tests/ui/unnecessary_lazy_eval.stderr index 1b0db4759bba..55cbed7a4150 100644 --- a/tests/ui/unnecessary_lazy_eval.stderr +++ b/tests/ui/unnecessary_lazy_eval.stderr @@ -1,5 +1,5 @@ error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:69:13 + --> $DIR/unnecessary_lazy_eval.rs:71:13 | LL | let _ = opt.unwrap_or_else(|| 2); | ^^^^-------------------- @@ -10,7 +10,7 @@ LL | let _ = opt.unwrap_or_else(|| 2); = help: to override `-D warnings` add `#[allow(clippy::unnecessary_lazy_evaluations)]` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:70:13 + --> $DIR/unnecessary_lazy_eval.rs:72:13 | LL | let _ = opt.unwrap_or_else(|| astronomers_pi); | ^^^^--------------------------------- @@ -18,7 +18,7 @@ LL | let _ = opt.unwrap_or_else(|| astronomers_pi); | help: use `unwrap_or(..)` instead: `unwrap_or(astronomers_pi)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:71:13 + --> $DIR/unnecessary_lazy_eval.rs:73:13 | LL | let _ = opt.unwrap_or_else(|| ext_str.some_field); | ^^^^------------------------------------- @@ -26,7 +26,7 @@ LL | let _ = opt.unwrap_or_else(|| ext_str.some_field); | help: use `unwrap_or(..)` instead: `unwrap_or(ext_str.some_field)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:73:13 + --> $DIR/unnecessary_lazy_eval.rs:75:13 | LL | let _ = opt.and_then(|_| ext_opt); | ^^^^--------------------- @@ -34,7 +34,7 @@ LL | let _ = opt.and_then(|_| ext_opt); | help: use `and(..)` instead: `and(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:74:13 + --> $DIR/unnecessary_lazy_eval.rs:76:13 | LL | let _ = opt.or_else(|| ext_opt); | ^^^^------------------- @@ -42,7 +42,7 @@ LL | let _ = opt.or_else(|| ext_opt); | help: use `or(..)` instead: `or(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:75:13 + --> $DIR/unnecessary_lazy_eval.rs:77:13 | LL | let _ = opt.or_else(|| None); | ^^^^---------------- @@ -50,7 +50,7 @@ LL | let _ = opt.or_else(|| None); | help: use `or(..)` instead: `or(None)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:76:13 + --> $DIR/unnecessary_lazy_eval.rs:78:13 | LL | let _ = opt.get_or_insert_with(|| 2); | ^^^^------------------------ @@ -58,7 +58,7 @@ LL | let _ = opt.get_or_insert_with(|| 2); | help: use `get_or_insert(..)` instead: `get_or_insert(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:77:13 + --> $DIR/unnecessary_lazy_eval.rs:79:13 | LL | let _ = opt.ok_or_else(|| 2); | ^^^^---------------- @@ -66,7 +66,7 @@ LL | let _ = opt.ok_or_else(|| 2); | help: use `ok_or(..)` instead: `ok_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:78:13 + --> $DIR/unnecessary_lazy_eval.rs:80:13 | LL | let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2))); | ^^^^^^^^^^^^^^^^^------------------------------- @@ -74,7 +74,7 @@ LL | let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2))); | help: use `unwrap_or(..)` instead: `unwrap_or(Some((1, 2)))` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:79:13 + --> $DIR/unnecessary_lazy_eval.rs:81:13 | LL | let _ = cond.then(|| astronomers_pi); | ^^^^^----------------------- @@ -82,7 +82,7 @@ LL | let _ = cond.then(|| astronomers_pi); | help: use `then_some(..)` instead: `then_some(astronomers_pi)` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:80:13 + --> $DIR/unnecessary_lazy_eval.rs:82:13 | LL | let _ = true.then(|| -> _ {}); | ^^^^^---------------- @@ -90,7 +90,7 @@ LL | let _ = true.then(|| -> _ {}); | help: use `then_some(..)` instead: `then_some({})` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:81:13 + --> $DIR/unnecessary_lazy_eval.rs:83:13 | LL | let _ = true.then(|| {}); | ^^^^^----------- @@ -98,7 +98,7 @@ LL | let _ = true.then(|| {}); | help: use `then_some(..)` instead: `then_some({})` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:85:13 + --> $DIR/unnecessary_lazy_eval.rs:87:13 | LL | let _ = Some(1).unwrap_or_else(|| *r); | ^^^^^^^^--------------------- @@ -106,7 +106,7 @@ LL | let _ = Some(1).unwrap_or_else(|| *r); | help: use `unwrap_or(..)` instead: `unwrap_or(*r)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:87:13 + --> $DIR/unnecessary_lazy_eval.rs:89:13 | LL | let _ = Some(1).unwrap_or_else(|| *b); | ^^^^^^^^--------------------- @@ -114,7 +114,7 @@ LL | let _ = Some(1).unwrap_or_else(|| *b); | help: use `unwrap_or(..)` instead: `unwrap_or(*b)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:89:13 + --> $DIR/unnecessary_lazy_eval.rs:91:13 | LL | let _ = Some(1).as_ref().unwrap_or_else(|| &r); | ^^^^^^^^^^^^^^^^^--------------------- @@ -122,7 +122,7 @@ LL | let _ = Some(1).as_ref().unwrap_or_else(|| &r); | help: use `unwrap_or(..)` instead: `unwrap_or(&r)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:90:13 + --> $DIR/unnecessary_lazy_eval.rs:92:13 | LL | let _ = Some(1).as_ref().unwrap_or_else(|| &b); | ^^^^^^^^^^^^^^^^^--------------------- @@ -130,7 +130,7 @@ LL | let _ = Some(1).as_ref().unwrap_or_else(|| &b); | help: use `unwrap_or(..)` instead: `unwrap_or(&b)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:93:13 + --> $DIR/unnecessary_lazy_eval.rs:95:13 | LL | let _ = Some(10).unwrap_or_else(|| 2); | ^^^^^^^^^-------------------- @@ -138,7 +138,7 @@ LL | let _ = Some(10).unwrap_or_else(|| 2); | help: use `unwrap_or(..)` instead: `unwrap_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:94:13 + --> $DIR/unnecessary_lazy_eval.rs:96:13 | LL | let _ = Some(10).and_then(|_| ext_opt); | ^^^^^^^^^--------------------- @@ -146,7 +146,7 @@ LL | let _ = Some(10).and_then(|_| ext_opt); | help: use `and(..)` instead: `and(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:95:28 + --> $DIR/unnecessary_lazy_eval.rs:97:28 | LL | let _: Option = None.or_else(|| ext_opt); | ^^^^^------------------- @@ -154,7 +154,7 @@ LL | let _: Option = None.or_else(|| ext_opt); | help: use `or(..)` instead: `or(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:96:13 + --> $DIR/unnecessary_lazy_eval.rs:98:13 | LL | let _ = None.get_or_insert_with(|| 2); | ^^^^^------------------------ @@ -162,7 +162,7 @@ LL | let _ = None.get_or_insert_with(|| 2); | help: use `get_or_insert(..)` instead: `get_or_insert(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:97:35 + --> $DIR/unnecessary_lazy_eval.rs:99:35 | LL | let _: Result = None.ok_or_else(|| 2); | ^^^^^---------------- @@ -170,7 +170,7 @@ LL | let _: Result = None.ok_or_else(|| 2); | help: use `ok_or(..)` instead: `ok_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:98:28 + --> $DIR/unnecessary_lazy_eval.rs:100:28 | LL | let _: Option = None.or_else(|| None); | ^^^^^---------------- @@ -178,7 +178,7 @@ LL | let _: Option = None.or_else(|| None); | help: use `or(..)` instead: `or(None)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:101:13 + --> $DIR/unnecessary_lazy_eval.rs:103:13 | LL | let _ = deep.0.unwrap_or_else(|| 2); | ^^^^^^^-------------------- @@ -186,7 +186,7 @@ LL | let _ = deep.0.unwrap_or_else(|| 2); | help: use `unwrap_or(..)` instead: `unwrap_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:102:13 + --> $DIR/unnecessary_lazy_eval.rs:104:13 | LL | let _ = deep.0.and_then(|_| ext_opt); | ^^^^^^^--------------------- @@ -194,7 +194,7 @@ LL | let _ = deep.0.and_then(|_| ext_opt); | help: use `and(..)` instead: `and(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:103:13 + --> $DIR/unnecessary_lazy_eval.rs:105:13 | LL | let _ = deep.0.or_else(|| None); | ^^^^^^^---------------- @@ -202,7 +202,7 @@ LL | let _ = deep.0.or_else(|| None); | help: use `or(..)` instead: `or(None)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:104:13 + --> $DIR/unnecessary_lazy_eval.rs:106:13 | LL | let _ = deep.0.get_or_insert_with(|| 2); | ^^^^^^^------------------------ @@ -210,7 +210,7 @@ LL | let _ = deep.0.get_or_insert_with(|| 2); | help: use `get_or_insert(..)` instead: `get_or_insert(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:105:13 + --> $DIR/unnecessary_lazy_eval.rs:107:13 | LL | let _ = deep.0.ok_or_else(|| 2); | ^^^^^^^---------------- @@ -218,7 +218,7 @@ LL | let _ = deep.0.ok_or_else(|| 2); | help: use `ok_or(..)` instead: `ok_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:135:28 + --> $DIR/unnecessary_lazy_eval.rs:137:28 | LL | let _: Option = None.or_else(|| Some(3)); | ^^^^^------------------- @@ -226,7 +226,7 @@ LL | let _: Option = None.or_else(|| Some(3)); | help: use `or(..)` instead: `or(Some(3))` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:136:13 + --> $DIR/unnecessary_lazy_eval.rs:138:13 | LL | let _ = deep.0.or_else(|| Some(3)); | ^^^^^^^------------------- @@ -234,7 +234,7 @@ LL | let _ = deep.0.or_else(|| Some(3)); | help: use `or(..)` instead: `or(Some(3))` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:137:13 + --> $DIR/unnecessary_lazy_eval.rs:139:13 | LL | let _ = opt.or_else(|| Some(3)); | ^^^^------------------- @@ -242,7 +242,7 @@ LL | let _ = opt.or_else(|| Some(3)); | help: use `or(..)` instead: `or(Some(3))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:143:13 + --> $DIR/unnecessary_lazy_eval.rs:145:13 | LL | let _ = res2.unwrap_or_else(|_| 2); | ^^^^^--------------------- @@ -250,7 +250,7 @@ LL | let _ = res2.unwrap_or_else(|_| 2); | help: use `unwrap_or(..)` instead: `unwrap_or(2)` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:144:13 + --> $DIR/unnecessary_lazy_eval.rs:146:13 | LL | let _ = res2.unwrap_or_else(|_| astronomers_pi); | ^^^^^---------------------------------- @@ -258,7 +258,7 @@ LL | let _ = res2.unwrap_or_else(|_| astronomers_pi); | help: use `unwrap_or(..)` instead: `unwrap_or(astronomers_pi)` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:145:13 + --> $DIR/unnecessary_lazy_eval.rs:147:13 | LL | let _ = res2.unwrap_or_else(|_| ext_str.some_field); | ^^^^^-------------------------------------- @@ -266,7 +266,7 @@ LL | let _ = res2.unwrap_or_else(|_| ext_str.some_field); | help: use `unwrap_or(..)` instead: `unwrap_or(ext_str.some_field)` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:167:35 + --> $DIR/unnecessary_lazy_eval.rs:169:35 | LL | let _: Result = res.and_then(|_| Err(2)); | ^^^^-------------------- @@ -274,7 +274,7 @@ LL | let _: Result = res.and_then(|_| Err(2)); | help: use `and(..)` instead: `and(Err(2))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:168:35 + --> $DIR/unnecessary_lazy_eval.rs:170:35 | LL | let _: Result = res.and_then(|_| Err(astronomers_pi)); | ^^^^--------------------------------- @@ -282,7 +282,7 @@ LL | let _: Result = res.and_then(|_| Err(astronomers_pi)); | help: use `and(..)` instead: `and(Err(astronomers_pi))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:169:35 + --> $DIR/unnecessary_lazy_eval.rs:171:35 | LL | let _: Result = res.and_then(|_| Err(ext_str.some_field)); | ^^^^------------------------------------- @@ -290,7 +290,7 @@ LL | let _: Result = res.and_then(|_| Err(ext_str.some_field)) | help: use `and(..)` instead: `and(Err(ext_str.some_field))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:171:35 + --> $DIR/unnecessary_lazy_eval.rs:173:35 | LL | let _: Result = res.or_else(|_| Ok(2)); | ^^^^------------------ @@ -298,7 +298,7 @@ LL | let _: Result = res.or_else(|_| Ok(2)); | help: use `or(..)` instead: `or(Ok(2))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:172:35 + --> $DIR/unnecessary_lazy_eval.rs:174:35 | LL | let _: Result = res.or_else(|_| Ok(astronomers_pi)); | ^^^^------------------------------- @@ -306,7 +306,7 @@ LL | let _: Result = res.or_else(|_| Ok(astronomers_pi)); | help: use `or(..)` instead: `or(Ok(astronomers_pi))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:173:35 + --> $DIR/unnecessary_lazy_eval.rs:175:35 | LL | let _: Result = res.or_else(|_| Ok(ext_str.some_field)); | ^^^^----------------------------------- @@ -314,7 +314,7 @@ LL | let _: Result = res.or_else(|_| Ok(ext_str.some_field)); | help: use `or(..)` instead: `or(Ok(ext_str.some_field))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:174:35 + --> $DIR/unnecessary_lazy_eval.rs:176:35 | LL | let _: Result = res. | ___________________________________^ @@ -328,5 +328,61 @@ LL | | or_else(|_| Ok(ext_str.some_field)); | | | help: use `or(..)` instead: `or(Ok(ext_str.some_field))` -error: aborting due to 40 previous errors +error: unnecessary closure used with `bool::then` + --> $DIR/unnecessary_lazy_eval.rs:208: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(|| (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 + | +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 + | +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 + | +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 + | +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 + | +LL | let _x = false.then(|| f1 + f2); + | ^^^^^^---------------- + | | + | help: use `then_some(..)` instead: `then_some(f1 + f2)` + +error: aborting due to 47 previous errors From 23ee33255c1fe039ad81bb95682e3dfc15308502 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Tue, 14 Nov 2023 21:40:38 +0100 Subject: [PATCH 2/5] lint when relevant sides of binops are constants --- clippy_utils/src/eager_or_lazy.rs | 37 +++++---- tests/ui/unnecessary_lazy_eval.fixed | 56 +++++++++----- tests/ui/unnecessary_lazy_eval.rs | 34 +++++++-- tests/ui/unnecessary_lazy_eval.stderr | 104 ++++++++++++++++++++++++-- 4 files changed, 180 insertions(+), 51 deletions(-) diff --git a/clippy_utils/src/eager_or_lazy.rs b/clippy_utils/src/eager_or_lazy.rs index 9a48e7727748..371fc029701e 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 da5da9ef85f0..07b43d3cc1d2 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 18b7eb4760fb..232a94d96382 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 55cbed7a4150..6c7edb24faec 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 From 1e0597cb68134ae221765deb3f3e9dd6d66f7dcc Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 16 Nov 2023 17:28:37 +0100 Subject: [PATCH 3/5] [`match_same_arms`]: respect allow attrs on arms --- clippy_lints/src/matches/match_same_arms.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index 6fc79faddbee..ba9ca7c5a52e 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::diagnostics::span_lint_hir_and_then; use clippy_utils::source::snippet; use clippy_utils::{is_lint_allowed, path_to_local, search_same, SpanlessEq, SpanlessHash}; use core::cmp::Ordering; @@ -106,9 +106,10 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { if !cx.tcx.features().non_exhaustive_omitted_patterns_lint || is_lint_allowed(cx, NON_EXHAUSTIVE_OMITTED_PATTERNS, arm2.hir_id) { - span_lint_and_then( + span_lint_hir_and_then( cx, MATCH_SAME_ARMS, + arm1.hir_id, arm1.span, "this match arm has an identical body to the `_` wildcard arm", |diag| { @@ -126,9 +127,10 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { (arm2, arm1) }; - span_lint_and_then( + span_lint_hir_and_then( cx, MATCH_SAME_ARMS, + keep_arm.hir_id, keep_arm.span, "this match arm has an identical body to another arm", |diag| { 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 4/5] 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 371fc029701e..f7a8bd131eb8 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 07b43d3cc1d2..66598f89208b 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 232a94d96382..5045fcd790ed 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 6c7edb24faec..466664aee6c7 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 From aca4086d7f4c889d2e55908325c782706ef4fa4f Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 16 Nov 2023 23:05:17 +0100 Subject: [PATCH 5/5] simplify matching on binop result --- clippy_utils/src/eager_or_lazy.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/clippy_utils/src/eager_or_lazy.rs b/clippy_utils/src/eager_or_lazy.rs index f7a8bd131eb8..4e71c6483e6a 100644 --- a/clippy_utils/src/eager_or_lazy.rs +++ b/clippy_utils/src/eager_or_lazy.rs @@ -229,21 +229,17 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS 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, - } => + && matches!( + (left, right), + // `1 / x`: x might be zero + (_, None) + // `x / -1`: x might be T::MIN + | (None, Some(FullInt::S(-1))) + ) => { self.eagerness |= NoChange; },