Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Commit e8e9510

Browse files
borsbors
committed
Auto merge of rust-lang#11002 - y21:issue9422, r=Jarcho
teach `eager_or_lazy` about panicky arithmetic operations Fixes rust-lang#9422 Fixes rust-lang#9814 Fixes rust-lang#11793 It's a bit sad that we have to do this because arithmetic operations seemed to me like the prime example where a closure would not be necessary, but this has "side effects" (changes behavior when going from lazy to eager) as some of these panic on overflow/underflow if compiled with `-Coverflow-checks` (which is the default in debug mode). Given the number of backlinks in the mentioned issues, this seems to be a FP that is worth fixing, probably. changelog: [`unnecessary_lazy_evaluations`]: don't lint if closure has panicky arithmetic operations
2 parents 31e38fe + aca4086 commit e8e9510

File tree

6 files changed

+545
-69
lines changed

6 files changed

+545
-69
lines changed

‎clippy_lints/src/matches/match_same_arms.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use clippy_utils::diagnostics::span_lint_and_then;
1+
use clippy_utils::diagnostics::span_lint_hir_and_then;
22
use clippy_utils::source::snippet;
33
use clippy_utils::{is_lint_allowed, path_to_local, search_same, SpanlessEq, SpanlessHash};
44
use core::cmp::Ordering;
@@ -104,9 +104,10 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
104104
if !cx.tcx.features().non_exhaustive_omitted_patterns_lint
105105
|| is_lint_allowed(cx, NON_EXHAUSTIVE_OMITTED_PATTERNS, arm2.hir_id)
106106
{
107-
span_lint_and_then(
107+
span_lint_hir_and_then(
108108
cx,
109109
MATCH_SAME_ARMS,
110+
arm1.hir_id,
110111
arm1.span,
111112
"this match arm has an identical body to the `_` wildcard arm",
112113
|diag| {
@@ -124,9 +125,10 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
124125
(arm2, arm1)
125126
};
126127

127-
span_lint_and_then(
128+
span_lint_hir_and_then(
128129
cx,
129130
MATCH_SAME_ARMS,
131+
keep_arm.hir_id,
130132
keep_arm.span,
131133
"this match arm has an identical body to another arm",
132134
|diag| {

‎clippy_utils/src/consts.rs‎

Lines changed: 108 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use rustc_hir::{BinOp, BinOpKind, Block, ConstBlock, Expr, ExprKind, HirId, Item
1010
use rustc_lexer::tokenize;
1111
use rustc_lint::LateContext;
1212
use rustc_middle::mir::interpret::{alloc_range, Scalar};
13-
use rustc_middle::ty::{self, EarlyBinder, FloatTy, GenericArgsRef, List, ScalarInt, Ty, TyCtxt};
13+
use rustc_middle::ty::{self, EarlyBinder, FloatTy, GenericArgsRef, IntTy, List, ScalarInt, Ty, TyCtxt, UintTy};
1414
use rustc_middle::{bug, mir, span_bug};
1515
use rustc_span::symbol::{Ident, Symbol};
1616
use rustc_span::SyntaxContext;
@@ -51,6 +51,63 @@ pub enum Constant<'tcx> {
5151
Err,
5252
}
5353

54+
trait IntTypeBounds: Sized {
55+
type Output: PartialOrd;
56+
57+
fn min_max(self) -> Option<(Self::Output, Self::Output)>;
58+
fn bits(self) -> Self::Output;
59+
fn ensure_fits(self, val: Self::Output) -> Option<Self::Output> {
60+
let (min, max) = self.min_max()?;
61+
(min <= val && val <= max).then_some(val)
62+
}
63+
}
64+
impl IntTypeBounds for UintTy {
65+
type Output = u128;
66+
fn min_max(self) -> Option<(Self::Output, Self::Output)> {
67+
Some(match self {
68+
UintTy::U8 => (u8::MIN.into(), u8::MAX.into()),
69+
UintTy::U16 => (u16::MIN.into(), u16::MAX.into()),
70+
UintTy::U32 => (u32::MIN.into(), u32::MAX.into()),
71+
UintTy::U64 => (u64::MIN.into(), u64::MAX.into()),
72+
UintTy::U128 => (u128::MIN, u128::MAX),
73+
UintTy::Usize => (usize::MIN.try_into().ok()?, usize::MAX.try_into().ok()?),
74+
})
75+
}
76+
fn bits(self) -> Self::Output {
77+
match self {
78+
UintTy::U8 => 8,
79+
UintTy::U16 => 16,
80+
UintTy::U32 => 32,
81+
UintTy::U64 => 64,
82+
UintTy::U128 => 128,
83+
UintTy::Usize => usize::BITS.into(),
84+
}
85+
}
86+
}
87+
impl IntTypeBounds for IntTy {
88+
type Output = i128;
89+
fn min_max(self) -> Option<(Self::Output, Self::Output)> {
90+
Some(match self {
91+
IntTy::I8 => (i8::MIN.into(), i8::MAX.into()),
92+
IntTy::I16 => (i16::MIN.into(), i16::MAX.into()),
93+
IntTy::I32 => (i32::MIN.into(), i32::MAX.into()),
94+
IntTy::I64 => (i64::MIN.into(), i64::MAX.into()),
95+
IntTy::I128 => (i128::MIN, i128::MAX),
96+
IntTy::Isize => (isize::MIN.try_into().ok()?, isize::MAX.try_into().ok()?),
97+
})
98+
}
99+
fn bits(self) -> Self::Output {
100+
match self {
101+
IntTy::I8 => 8,
102+
IntTy::I16 => 16,
103+
IntTy::I32 => 32,
104+
IntTy::I64 => 64,
105+
IntTy::I128 => 128,
106+
IntTy::Isize => isize::BITS.into(),
107+
}
108+
}
109+
}
110+
54111
impl<'tcx> PartialEq for Constant<'tcx> {
55112
fn eq(&self, other: &Self) -> bool {
56113
match (self, other) {
@@ -433,8 +490,15 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
433490
match *o {
434491
Int(value) => {
435492
let ty::Int(ity) = *ty.kind() else { return None };
493+
let (min, _) = ity.min_max()?;
436494
// sign extend
437495
let value = sext(self.lcx.tcx, value, ity);
496+
497+
// Applying unary - to the most negative value of any signed integer type panics.
498+
if value == min {
499+
return None;
500+
}
501+
438502
let value = value.checked_neg()?;
439503
// clear unused bits
440504
Some(Int(unsext(self.lcx.tcx, value, ity)))
@@ -570,17 +634,33 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
570634
match (l, r) {
571635
(Constant::Int(l), Some(Constant::Int(r))) => match *self.typeck_results.expr_ty_opt(left)?.kind() {
572636
ty::Int(ity) => {
637+
let (ty_min_value, _) = ity.min_max()?;
638+
let bits = ity.bits();
573639
let l = sext(self.lcx.tcx, l, ity);
574640
let r = sext(self.lcx.tcx, r, ity);
641+
642+
// Using / or %, where the left-hand argument is the smallest integer of a signed integer type and
643+
// the right-hand argument is -1 always panics, even with overflow-checks disabled
644+
if let BinOpKind::Div | BinOpKind::Rem = op.node
645+
&& l == ty_min_value
646+
&& r == -1
647+
{
648+
return None;
649+
}
650+
575651
let zext = |n: i128| Constant::Int(unsext(self.lcx.tcx, n, ity));
576652
match op.node {
577-
BinOpKind::Add => l.checked_add(r).map(zext),
578-
BinOpKind::Sub => l.checked_sub(r).map(zext),
579-
BinOpKind::Mul => l.checked_mul(r).map(zext),
653+
// When +, * or binary - create a value greater than the maximum value, or less than
654+
// the minimum value that can be stored, it panics.
655+
BinOpKind::Add => l.checked_add(r).and_then(|n| ity.ensure_fits(n)).map(zext),
656+
BinOpKind::Sub => l.checked_sub(r).and_then(|n| ity.ensure_fits(n)).map(zext),
657+
BinOpKind::Mul => l.checked_mul(r).and_then(|n| ity.ensure_fits(n)).map(zext),
580658
BinOpKind::Div if r != 0 => l.checked_div(r).map(zext),
581659
BinOpKind::Rem if r != 0 => l.checked_rem(r).map(zext),
582-
BinOpKind::Shr => l.checked_shr(r.try_into().ok()?).map(zext),
583-
BinOpKind::Shl => l.checked_shl(r.try_into().ok()?).map(zext),
660+
// Using << or >> where the right-hand argument is greater than or equal to the number of bits
661+
// in the type of the left-hand argument, or is negative panics.
662+
BinOpKind::Shr if r < bits && !r.is_negative() => l.checked_shr(r.try_into().ok()?).map(zext),
663+
BinOpKind::Shl if r < bits && !r.is_negative() => l.checked_shl(r.try_into().ok()?).map(zext),
584664
BinOpKind::BitXor => Some(zext(l ^ r)),
585665
BinOpKind::BitOr => Some(zext(l | r)),
586666
BinOpKind::BitAnd => Some(zext(l & r)),
@@ -593,24 +673,28 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
593673
_ => None,
594674
}
595675
},
596-
ty::Uint(_) => match op.node {
597-
BinOpKind::Add => l.checked_add(r).map(Constant::Int),
598-
BinOpKind::Sub => l.checked_sub(r).map(Constant::Int),
599-
BinOpKind::Mul => l.checked_mul(r).map(Constant::Int),
600-
BinOpKind::Div => l.checked_div(r).map(Constant::Int),
601-
BinOpKind::Rem => l.checked_rem(r).map(Constant::Int),
602-
BinOpKind::Shr => l.checked_shr(r.try_into().ok()?).map(Constant::Int),
603-
BinOpKind::Shl => l.checked_shl(r.try_into().ok()?).map(Constant::Int),
604-
BinOpKind::BitXor => Some(Constant::Int(l ^ r)),
605-
BinOpKind::BitOr => Some(Constant::Int(l | r)),
606-
BinOpKind::BitAnd => Some(Constant::Int(l & r)),
607-
BinOpKind::Eq => Some(Constant::Bool(l == r)),
608-
BinOpKind::Ne => Some(Constant::Bool(l != r)),
609-
BinOpKind::Lt => Some(Constant::Bool(l < r)),
610-
BinOpKind::Le => Some(Constant::Bool(l <= r)),
611-
BinOpKind::Ge => Some(Constant::Bool(l >= r)),
612-
BinOpKind::Gt => Some(Constant::Bool(l > r)),
613-
_ => None,
676+
ty::Uint(ity) => {
677+
let bits = ity.bits();
678+
679+
match op.node {
680+
BinOpKind::Add => l.checked_add(r).and_then(|n| ity.ensure_fits(n)).map(Constant::Int),
681+
BinOpKind::Sub => l.checked_sub(r).and_then(|n| ity.ensure_fits(n)).map(Constant::Int),
682+
BinOpKind::Mul => l.checked_mul(r).and_then(|n| ity.ensure_fits(n)).map(Constant::Int),
683+
BinOpKind::Div => l.checked_div(r).map(Constant::Int),
684+
BinOpKind::Rem => l.checked_rem(r).map(Constant::Int),
685+
BinOpKind::Shr if r < bits => l.checked_shr(r.try_into().ok()?).map(Constant::Int),
686+
BinOpKind::Shl if r < bits => l.checked_shl(r.try_into().ok()?).map(Constant::Int),
687+
BinOpKind::BitXor => Some(Constant::Int(l ^ r)),
688+
BinOpKind::BitOr => Some(Constant::Int(l | r)),
689+
BinOpKind::BitAnd => Some(Constant::Int(l & r)),
690+
BinOpKind::Eq => Some(Constant::Bool(l == r)),
691+
BinOpKind::Ne => Some(Constant::Bool(l != r)),
692+
BinOpKind::Lt => Some(Constant::Bool(l < r)),
693+
BinOpKind::Le => Some(Constant::Bool(l <= r)),
694+
BinOpKind::Ge => Some(Constant::Bool(l >= r)),
695+
BinOpKind::Gt => Some(Constant::Bool(l > r)),
696+
_ => None,
697+
}
614698
},
615699
_ => None,
616700
},

‎clippy_utils/src/eager_or_lazy.rs‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,13 @@
99
//! - or-fun-call
1010
//! - option-if-let-else
1111
12+
use crate::consts::{constant, FullInt};
1213
use crate::ty::{all_predicates_of, is_copy};
1314
use crate::visitors::is_const_evaluatable;
1415
use rustc_hir::def::{DefKind, Res};
1516
use rustc_hir::def_id::DefId;
1617
use rustc_hir::intravisit::{walk_expr, Visitor};
17-
use rustc_hir::{Block, Expr, ExprKind, QPath, UnOp};
18+
use rustc_hir::{BinOpKind, Block, Expr, ExprKind, QPath, UnOp};
1819
use rustc_lint::LateContext;
1920
use rustc_middle::ty;
2021
use rustc_middle::ty::adjustment::Adjust;
@@ -193,6 +194,12 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS
193194
self.eagerness = Lazy;
194195
}
195196
},
197+
198+
// `-i32::MIN` panics with overflow checks
199+
ExprKind::Unary(UnOp::Neg, right) if constant(self.cx, self.cx.typeck_results(), right).is_none() => {
200+
self.eagerness |= NoChange;
201+
},
202+
196203
// Custom `Deref` impl might have side effects
197204
ExprKind::Unary(UnOp::Deref, e)
198205
if self.cx.typeck_results().expr_ty(e).builtin_deref(true).is_none() =>
@@ -207,6 +214,49 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS
207214
self.cx.typeck_results().expr_ty(e).kind(),
208215
ty::Bool | ty::Int(_) | ty::Uint(_),
209216
) => {},
217+
218+
// `>>` and `<<` panic when the right-hand side is greater than or equal to the number of bits in the
219+
// type of the left-hand side, or is negative.
220+
// We intentionally only check if the right-hand isn't a constant, because even if the suggestion would
221+
// overflow with constants, the compiler emits an error for it and the programmer will have to fix it.
222+
// Thus, we would realistically only delay the lint.
223+
ExprKind::Binary(op, _, right)
224+
if matches!(op.node, BinOpKind::Shl | BinOpKind::Shr)
225+
&& constant(self.cx, self.cx.typeck_results(), right).is_none() =>
226+
{
227+
self.eagerness |= NoChange;
228+
},
229+
230+
ExprKind::Binary(op, left, right)
231+
if matches!(op.node, BinOpKind::Div | BinOpKind::Rem)
232+
&& let right_ty = self.cx.typeck_results().expr_ty(right)
233+
&& let left = constant(self.cx, self.cx.typeck_results(), left)
234+
&& let right = constant(self.cx, self.cx.typeck_results(), right)
235+
.and_then(|c| c.int_value(self.cx, right_ty))
236+
&& matches!(
237+
(left, right),
238+
// `1 / x`: x might be zero
239+
(_, None)
240+
// `x / -1`: x might be T::MIN
241+
| (None, Some(FullInt::S(-1)))
242+
) =>
243+
{
244+
self.eagerness |= NoChange;
245+
},
246+
247+
// Similar to `>>` and `<<`, we only want to avoid linting entirely if either side is unknown and the
248+
// compiler can't emit an error for an overflowing expression.
249+
// Suggesting eagerness for `true.then(|| i32::MAX + 1)` is okay because the compiler will emit an
250+
// error and it's good to have the eagerness warning up front when the user fixes the logic error.
251+
ExprKind::Binary(op, left, right)
252+
if matches!(op.node, BinOpKind::Add | BinOpKind::Sub | BinOpKind::Mul)
253+
&& !self.cx.typeck_results().expr_ty(e).is_floating_point()
254+
&& (constant(self.cx, self.cx.typeck_results(), left).is_none()
255+
|| constant(self.cx, self.cx.typeck_results(), right).is_none()) =>
256+
{
257+
self.eagerness |= NoChange;
258+
},
259+
210260
ExprKind::Binary(_, lhs, rhs)
211261
if self.cx.typeck_results().expr_ty(lhs).is_primitive()
212262
&& self.cx.typeck_results().expr_ty(rhs).is_primitive() => {},

‎tests/ui/unnecessary_lazy_eval.fixed‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
#![allow(clippy::needless_borrow)]
77
#![allow(clippy::unnecessary_literal_unwrap)]
88
#![allow(clippy::unit_arg)]
9+
#![allow(arithmetic_overflow)]
10+
#![allow(unconditional_panic)]
911

1012
use std::ops::Deref;
1113

@@ -190,3 +192,79 @@ fn issue9485() {
190192
// should not lint, is in proc macro
191193
with_span!(span Some(42).unwrap_or_else(|| 2););
192194
}
195+
196+
fn issue9422(x: usize) -> Option<usize> {
197+
(x >= 5).then(|| x - 5)
198+
// (x >= 5).then_some(x - 5) // clippy suggestion panics
199+
}
200+
201+
fn panicky_arithmetic_ops(x: usize, y: isize) {
202+
#![allow(clippy::identity_op, clippy::eq_op)]
203+
204+
// See comments in `eager_or_lazy.rs` for the rules that this is meant to follow
205+
206+
let _x = false.then_some(i32::MAX + 1);
207+
//~^ ERROR: unnecessary closure used with `bool::then`
208+
let _x = false.then_some(i32::MAX * 2);
209+
//~^ ERROR: unnecessary closure used with `bool::then`
210+
let _x = false.then_some(i32::MAX - 1);
211+
//~^ ERROR: unnecessary closure used with `bool::then`
212+
let _x = false.then_some(i32::MIN - 1);
213+
//~^ ERROR: unnecessary closure used with `bool::then`
214+
let _x = false.then_some((1 + 2 * 3 - 2 / 3 + 9) << 2);
215+
//~^ ERROR: unnecessary closure used with `bool::then`
216+
let _x = false.then_some(255u8 << 7);
217+
//~^ ERROR: unnecessary closure used with `bool::then`
218+
let _x = false.then_some(255u8 << 8);
219+
//~^ ERROR: unnecessary closure used with `bool::then`
220+
let _x = false.then_some(255u8 >> 8);
221+
//~^ ERROR: unnecessary closure used with `bool::then`
222+
let _x = false.then(|| 255u8 >> x);
223+
let _x = false.then_some(i32::MAX + -1);
224+
//~^ ERROR: unnecessary closure used with `bool::then`
225+
let _x = false.then_some(-i32::MAX);
226+
//~^ ERROR: unnecessary closure used with `bool::then`
227+
let _x = false.then_some(-i32::MIN);
228+
//~^ ERROR: unnecessary closure used with `bool::then`
229+
let _x = false.then(|| -y);
230+
let _x = false.then_some(255 >> -7);
231+
//~^ ERROR: unnecessary closure used with `bool::then`
232+
let _x = false.then_some(255 << -1);
233+
//~^ ERROR: unnecessary closure used with `bool::then`
234+
let _x = false.then_some(1 / 0);
235+
//~^ ERROR: unnecessary closure used with `bool::then`
236+
let _x = false.then_some(x << -1);
237+
//~^ ERROR: unnecessary closure used with `bool::then`
238+
let _x = false.then_some(x << 2);
239+
//~^ ERROR: unnecessary closure used with `bool::then`
240+
let _x = false.then(|| x + x);
241+
let _x = false.then(|| x * x);
242+
let _x = false.then(|| x - x);
243+
let _x = false.then(|| x / x);
244+
let _x = false.then(|| x % x);
245+
let _x = false.then(|| x + 1);
246+
let _x = false.then(|| 1 + x);
247+
248+
let _x = false.then_some(x / 0);
249+
//~^ ERROR: unnecessary closure used with `bool::then`
250+
let _x = false.then_some(x % 0);
251+
//~^ ERROR: unnecessary closure used with `bool::then`
252+
let _x = false.then(|| y / -1);
253+
let _x = false.then_some(1 / -1);
254+
//~^ ERROR: unnecessary closure used with `bool::then`
255+
let _x = false.then_some(i32::MIN / -1);
256+
//~^ ERROR: unnecessary closure used with `bool::then`
257+
let _x = false.then(|| i32::MIN / x as i32);
258+
let _x = false.then_some(i32::MIN / 0);
259+
//~^ ERROR: unnecessary closure used with `bool::then`
260+
let _x = false.then_some(4 / 2);
261+
//~^ ERROR: unnecessary closure used with `bool::then`
262+
let _x = false.then(|| 1 / x);
263+
264+
// const eval doesn't read variables, but floating point math never panics, so we can still emit a
265+
// warning
266+
let f1 = 1.0;
267+
let f2 = 2.0;
268+
let _x = false.then_some(f1 + f2);
269+
//~^ ERROR: unnecessary closure used with `bool::then`
270+
}

0 commit comments

Comments
 (0)