[Arithmetic] Consider literals

Author: c410-f3r

clippy_lints/src/lib.rs

@@ -3,6 +3,7 @@
 #![feature(box_patterns)]
 #![feature(control_flow_enum)]
 #![feature(drain_filter)]
+#![feature(if_let_guard)]
 #![feature(iter_intersperse)]
 #![feature(let_else)]
 #![feature(lint_reasons)]

clippy_lints/src/operators/arithmetic.rs

@@ -5,11 +5,13 @@
 
 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"];
 
@@ -34,6 +36,29 @@ 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 {
+            match rhs.kind {
+                hir::ExprKind::Lit(ref lit) if 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 +71,65 @@ 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;
+        }
+        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;
+        }
+        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 +139,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);
         }
     }
 

tests/ui/arithmetic.fixed

@@ -1,7 +1,5 @@
-// 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};
@@ -24,4 +22,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:47:21
+   |
+LL |     let mut _a = 1; _a += 1;
+   |                     ^^^^^^^
+   |
+   = note: `-D clippy::arithmetic` implied by `-D warnings`
+
+error: arithmetic detected
+  --> $DIR/arithmetic.rs:49:26
+   |
+LL |     let mut _b = 1; _b = _b + 1;
+   |                          ^^^^^^
+
+error: arithmetic detected
+  --> $DIR/arithmetic.rs:51:26
+   |
+LL |     let mut _c = 1; _c = 1 + _c;
+   |                          ^^^^^^
+
+error: aborting due to 3 previous errors
+