Skip to content

teach eager_or_lazy about panicky arithmetic operations #11002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 17, 2023
Merged

teach eager_or_lazy about panicky arithmetic operations #11002

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c9d2961
teach `eager_or_lazy` that arithmetic ops can panic
y21 Jun 21, 2023
23ee332
lint when relevant sides of binops are constants
y21 Nov 14, 2023
1e0597c
[`match_same_arms`]: respect allow attrs on arms
y21 Nov 16, 2023
7696c9d
allow more div and rem operations that can be checked
y21 Nov 16, 2023
aca4086
simplify matching on binop result
y21 Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 108 additions & 24 deletions clippy_utils/src/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Self::Output> {
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) {
Expand Down Expand Up @@ -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)))
Expand Down Expand Up @@ -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)),
Expand All @@ -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,
},
Expand Down
40 changes: 39 additions & 1 deletion clippy_utils/src/eager_or_lazy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() =>
Expand All @@ -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() => {},
Expand Down
46 changes: 46 additions & 0 deletions tests/ui/unnecessary_lazy_eval.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -183,10 +185,54 @@ fn main() {
// neither bind_instead_of_map nor unnecessary_lazy_eval applies here
let _: Result<usize, usize> = res.and_then(|x| Err(x));
let _: Result<usize, usize> = res.or_else(|err| Ok(err));

issue9422(3);
panicky_arithmetic_ops(3);
}

#[allow(unused)]
fn issue9485() {
// should not lint, is in proc macro
with_span!(span Some(42).unwrap_or_else(|| 2););
}

fn issue9422(x: usize) -> Option<usize> {
(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`
}
46 changes: 46 additions & 0 deletions tests/ui/unnecessary_lazy_eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -183,10 +185,54 @@ fn main() {
// neither bind_instead_of_map nor unnecessary_lazy_eval applies here
let _: Result<usize, usize> = res.and_then(|x| Err(x));
let _: Result<usize, usize> = res.or_else(|err| Ok(err));

issue9422(3);
panicky_arithmetic_ops(3);
}

#[allow(unused)]
fn issue9485() {
// should not lint, is in proc macro
with_span!(span Some(42).unwrap_or_else(|| 2););
}

fn issue9422(x: usize) -> Option<usize> {
(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`
}
Loading