diff --git a/clippy_lints/src/operators/arithmetic.rs b/clippy_lints/src/operators/arithmetic.rs index 800cf249f5c..27eef92deef 100644 --- a/clippy_lints/src/operators/arithmetic.rs +++ b/clippy_lints/src/operators/arithmetic.rs @@ -5,13 +5,21 @@ use super::ARITHMETIC; use clippy_utils::{consts::constant_simple, diagnostics::span_lint}; +use rustc_ast as ast; use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; use rustc_session::impl_lint_pass; -use rustc_span::source_map::Span; +use rustc_span::source_map::{Span, Spanned}; -const HARD_CODED_ALLOWED: &[&str] = &["std::num::Saturating", "std::string::String", "std::num::Wrapping"]; +const HARD_CODED_ALLOWED: &[&str] = &[ + "f32", + "f64", + "std::num::Saturating", + "std::string::String", + "std::num::Wrapping", +]; #[derive(Debug)] pub struct Arithmetic { @@ -34,6 +42,27 @@ impl Arithmetic { } } + /// Checks assign operators (+=, -=, *=, /=) of integers in a non-constant environment that + /// won't overflow. + fn has_valid_assign_op(op: &Spanned, rhs: &hir::Expr<'_>, rhs_refs: Ty<'_>) -> bool { + if !Self::is_literal_integer(rhs, rhs_refs) { + return false; + } + if let hir::BinOpKind::Div | hir::BinOpKind::Mul = op.node + && let hir::ExprKind::Lit(ref lit) = rhs.kind + && let ast::LitKind::Int(1, _) = lit.node + { + return true; + } + false + } + + /// Checks "raw" binary operators (+, -, *, /) of integers in a non-constant environment + /// already handled by the CTFE. + fn has_valid_bin_op(lhs: &hir::Expr<'_>, lhs_refs: Ty<'_>, rhs: &hir::Expr<'_>, rhs_refs: Ty<'_>) -> bool { + Self::is_literal_integer(lhs, lhs_refs) && Self::is_literal_integer(rhs, rhs_refs) + } + /// Checks if the given `expr` has any of the inner `allowed` elements. fn is_allowed_ty(&self, cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { self.allowed.contains( @@ -46,40 +75,66 @@ impl Arithmetic { ) } + /// Explicit integers like `1` or `i32::MAX`. Does not take into consideration references. + fn is_literal_integer(expr: &hir::Expr<'_>, expr_refs: Ty<'_>) -> bool { + let is_integral = expr_refs.is_integral(); + let is_literal = matches!(expr.kind, hir::ExprKind::Lit(_)); + is_integral && is_literal + } + fn issue_lint(&mut self, cx: &LateContext<'_>, expr: &hir::Expr<'_>) { span_lint(cx, ARITHMETIC, expr.span, "arithmetic detected"); self.expr_span = Some(expr.span); } + + /// Manages when the lint should be triggered. Operations in constant environments, hard coded + /// types, custom allowed types and non-constant operations that won't overflow are ignored. + fn manage_bin_ops( + &mut self, + cx: &LateContext<'_>, + expr: &hir::Expr<'_>, + op: &Spanned, + lhs: &hir::Expr<'_>, + rhs: &hir::Expr<'_>, + ) { + if constant_simple(cx, cx.typeck_results(), expr).is_some() { + return; + } + if !matches!( + op.node, + hir::BinOpKind::Add + | hir::BinOpKind::Sub + | hir::BinOpKind::Mul + | hir::BinOpKind::Div + | hir::BinOpKind::Rem + | hir::BinOpKind::Shl + | hir::BinOpKind::Shr + ) { + return; + }; + if self.is_allowed_ty(cx, lhs) || self.is_allowed_ty(cx, rhs) { + return; + } + let lhs_refs = cx.typeck_results().expr_ty(lhs).peel_refs(); + let rhs_refs = cx.typeck_results().expr_ty(rhs).peel_refs(); + let has_valid_assign_op = Self::has_valid_assign_op(op, rhs, rhs_refs); + if has_valid_assign_op || Self::has_valid_bin_op(lhs, lhs_refs, rhs, rhs_refs) { + return; + } + self.issue_lint(cx, expr); + } } impl<'tcx> LateLintPass<'tcx> for Arithmetic { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { - if self.expr_span.is_some() { - return; - } - if let Some(span) = self.const_span && span.contains(expr.span) { + if self.expr_span.is_some() || self.const_span.map_or(false, |sp| sp.contains(expr.span)) { return; } match &expr.kind { hir::ExprKind::Binary(op, lhs, rhs) | hir::ExprKind::AssignOp(op, lhs, rhs) => { - let ( - hir::BinOpKind::Add - | hir::BinOpKind::Sub - | hir::BinOpKind::Mul - | hir::BinOpKind::Div - | hir::BinOpKind::Rem - | hir::BinOpKind::Shl - | hir::BinOpKind::Shr - ) = op.node else { - return; - }; - if self.is_allowed_ty(cx, lhs) || self.is_allowed_ty(cx, rhs) { - return; - } - self.issue_lint(cx, expr); + self.manage_bin_ops(cx, expr, op, lhs, rhs); }, hir::ExprKind::Unary(hir::UnOp::Neg, _) => { - // CTFE already takes care of things like `-1` that do not overflow. if constant_simple(cx, cx.typeck_results(), expr).is_none() { self.issue_lint(cx, expr); } @@ -89,16 +144,15 @@ impl<'tcx> LateLintPass<'tcx> for Arithmetic { } fn check_body(&mut self, cx: &LateContext<'_>, body: &hir::Body<'_>) { - let body_owner = cx.tcx.hir().body_owner_def_id(body.id()); - match cx.tcx.hir().body_owner_kind(body_owner) { - hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) => { - let body_span = cx.tcx.def_span(body_owner); - if let Some(span) = self.const_span && span.contains(body_span) { - return; - } - self.const_span = Some(body_span); - }, - hir::BodyOwnerKind::Closure | hir::BodyOwnerKind::Fn => {}, + let body_owner = cx.tcx.hir().body_owner(body.id()); + let body_owner_def_id = cx.tcx.hir().local_def_id(body_owner); + let body_owner_kind = cx.tcx.hir().body_owner_kind(body_owner_def_id); + if let hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) = body_owner_kind { + let body_span = cx.tcx.hir().span_with_body(body_owner); + if let Some(span) = self.const_span && span.contains(body_span) { + return; + } + self.const_span = Some(body_span); } } diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index bb6d99406b4..c7b0633b79c 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -61,25 +61,29 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for any kind of arithmetic operation of any type. + /// Checks any kind of arithmetic operation of any type. /// /// Operators like `+`, `-`, `*` or `<<` are usually capable of overflowing according to the [Rust /// Reference](https://doc.rust-lang.org/reference/expressions/operator-expr.html#overflow), - /// or can panic (`/`, `%`). Known safe built-in types like `Wrapping` or `Saturing` are filtered - /// away. + /// or can panic (`/`, `%`). + /// + /// Known safe built-in types like `Wrapping` or `Saturing`, floats, operations in constant + /// environments, allowed types and non-constant operations that won't overflow are ignored. /// /// ### Why is this bad? - /// Integer overflow will trigger a panic in debug builds or will wrap in - /// release mode. Division by zero will cause a panic in either mode. In some applications one - /// wants explicitly checked, wrapping or saturating arithmetic. + /// For integers, overflow will trigger a panic in debug builds or wrap the result in + /// release mode; division by zero will cause a panic in either mode. As a result, it is + /// desirable to explicitly call checked, wrapping or saturating arithmetic methods. /// /// #### Example /// ```rust - /// # let a = 0; - /// a + 1; + /// // `n` can be any number, including `i32::MAX`. + /// fn foo(n: i32) -> i32 { + /// n + 1 + /// } /// ``` /// - /// Third-party types also tend to overflow. + /// Third-party types can also overflow or present unwanted side-effects. /// /// #### Example /// ```ignore,rust diff --git a/src/docs/arithmetic.txt b/src/docs/arithmetic.txt index 0b3f07d9505..f04125060fb 100644 --- a/src/docs/arithmetic.txt +++ b/src/docs/arithmetic.txt @@ -1,22 +1,27 @@ ### What it does -Checks for any kind of arithmetic operation of any type. +Checks any kind of arithmetic operation of any type. Operators like `+`, `-`, `*` or `<<` are usually capable of overflowing according to the [Rust Reference](https://doc.rust-lang.org/reference/expressions/operator-expr.html#overflow), -or can panic (`/`, `%`). Known safe built-in types like `Wrapping` or `Saturing` are filtered -away. +or can panic (`/`, `%`). + +Known safe built-in types like `Wrapping` or `Saturing`, floats, operations in constant +environments, allowed types and non-constant operations that won't overflow are ignored. ### Why is this bad? -Integer overflow will trigger a panic in debug builds or will wrap in -release mode. Division by zero will cause a panic in either mode. In some applications one -wants explicitly checked, wrapping or saturating arithmetic. +For integers, overflow will trigger a panic in debug builds or wrap the result in +release mode; division by zero will cause a panic in either mode. As a result, it is +desirable to explicitly call checked, wrapping or saturating arithmetic methods. #### Example ``` -a + 1; +// `n` can be any number, including `i32::MAX`. +fn foo(n: i32) -> i32 { + n + 1 +} ``` -Third-party types also tend to overflow. +Third-party types can also overflow or present unwanted side-effects. #### Example ``` diff --git a/tests/ui/arithmetic.fixed b/tests/ui/arithmetic.fixed deleted file mode 100644 index a2a1c4394c2..00000000000 --- a/tests/ui/arithmetic.fixed +++ /dev/null @@ -1,27 +0,0 @@ -// run-rustfix - -#![allow(clippy::unnecessary_owned_empty_strings)] -#![feature(saturating_int_impl)] -#![warn(clippy::arithmetic)] - -use core::num::{Saturating, Wrapping}; - -pub fn hard_coded_allowed() { - let _ = Saturating(0u32) + Saturating(0u32); - let _ = String::new() + ""; - let _ = Wrapping(0u32) + Wrapping(0u32); - - let saturating: Saturating = Saturating(0u32); - let string: String = String::new(); - let wrapping: Wrapping = Wrapping(0u32); - - let inferred_saturating = saturating + saturating; - let inferred_string = string + ""; - let inferred_wrapping = wrapping + wrapping; - - let _ = inferred_saturating + inferred_saturating; - let _ = inferred_string + ""; - let _ = inferred_wrapping + inferred_wrapping; -} - -fn main() {} diff --git a/tests/ui/arithmetic.rs b/tests/ui/arithmetic.rs index a2a1c4394c2..a9ac46e9a19 100644 --- a/tests/ui/arithmetic.rs +++ b/tests/ui/arithmetic.rs @@ -1,12 +1,13 @@ -// run-rustfix - -#![allow(clippy::unnecessary_owned_empty_strings)] -#![feature(saturating_int_impl)] +#![allow(clippy::assign_op_pattern, clippy::unnecessary_owned_empty_strings)] +#![feature(inline_const, saturating_int_impl)] #![warn(clippy::arithmetic)] use core::num::{Saturating, Wrapping}; pub fn hard_coded_allowed() { + let _ = 1f32 + 1f32; + let _ = 1f64 + 1f64; + let _ = Saturating(0u32) + Saturating(0u32); let _ = String::new() + ""; let _ = Wrapping(0u32) + Wrapping(0u32); @@ -24,4 +25,33 @@ pub fn hard_coded_allowed() { let _ = inferred_wrapping + inferred_wrapping; } +#[rustfmt::skip] +pub fn non_overflowing_ops() { + const _: i32 = { let mut n = 1; n += 1; n }; + let _ = const { let mut n = 1; n += 1; n }; + + const _: i32 = { let mut n = 1; n = n + 1; n }; + let _ = const { let mut n = 1; n = n + 1; n }; + + const _: i32 = { let mut n = 1; n = 1 + n; n }; + let _ = const { let mut n = 1; n = 1 + n; n }; + + const _: i32 = 1 + 1; + let _ = 1 + 1; + let _ = const { 1 + 1 }; + + let mut _a = 1; + _a *= 1; + _a /= 1; +} + +#[rustfmt::skip] +pub fn overflowing_ops() { + let mut _a = 1; _a += 1; + + let mut _b = 1; _b = _b + 1; + + let mut _c = 1; _c = 1 + _c; +} + fn main() {} diff --git a/tests/ui/arithmetic.stderr b/tests/ui/arithmetic.stderr new file mode 100644 index 00000000000..a51cf6ba600 --- /dev/null +++ b/tests/ui/arithmetic.stderr @@ -0,0 +1,22 @@ +error: arithmetic detected + --> $DIR/arithmetic.rs:50:21 + | +LL | let mut _a = 1; _a += 1; + | ^^^^^^^ + | + = note: `-D clippy::arithmetic` implied by `-D warnings` + +error: arithmetic detected + --> $DIR/arithmetic.rs:52:26 + | +LL | let mut _b = 1; _b = _b + 1; + | ^^^^^^ + +error: arithmetic detected + --> $DIR/arithmetic.rs:54:26 + | +LL | let mut _c = 1; _c = 1 + _c; + | ^^^^^^ + +error: aborting due to 3 previous errors +