Auto merge of #9365 - c410-f3r:arith, r=Alexendoo

[Arithmetic] Consider literals

Fixes #9307 and makes the `arithmetic` lint behave like `integer_arithmetic`.

It is worth noting that literal integers of a binary operation (`1 + 1`, `i32::MAX + 1`), **regardless if they are in a constant environment**, won't trigger the lint. Assign operations also have similar reasoning.

changelog: Consider literals in the arithmetic lint

Author: bors

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<hir::BinOpKind>, 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<hir::BinOpKind>,
+        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);
         }
     }
 

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

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
 ```

tests/ui/arithmetic.fixed

@@ -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() {}

tests/ui/arithmetic.rs

@@ -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
+