Skip to content

Commit

Permalink
teach eager_or_lazy that arithmetic ops can panic
Browse files Browse the repository at this point in the history
  • Loading branch information
@y21
y21 committed Nov 2, 2023
1 parent 902c79c commit c9d2961
Show file tree
Hide file tree
Showing 5 changed files with 336 additions and 66 deletions.
132 changes: 108 additions & 24 deletions clippy_utils/src/consts.rs
View file
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
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
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
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`
}

0 comments on commit c9d2961

Please sign in to comment.