Skip to content

Add Arithmetic lint #9130

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.

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 1 commit into from
Jul 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3437,6 +3437,7 @@ Released 2018-09-13
[`almost_complete_letter_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_letter_range
[`almost_swapped`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped
[`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant
[`arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#arithmetic
[`as_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions
[`as_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore
[`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ store.register_lints(&[
only_used_in_recursion::ONLY_USED_IN_RECURSION,
open_options::NONSENSICAL_OPEN_OPTIONS,
operators::ABSURD_EXTREME_COMPARISONS,
operators::ARITHMETIC,
operators::ASSIGN_OP_PATTERN,
operators::BAD_BIT_MASK,
operators::CMP_NAN,
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_restriction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
LintId::of(mixed_read_write_in_expression::MIXED_READ_WRITE_IN_EXPRESSION),
LintId::of(module_style::MOD_MODULE_FILES),
LintId::of(module_style::SELF_NAMED_MODULE_FILES),
LintId::of(operators::ARITHMETIC),
LintId::of(operators::FLOAT_ARITHMETIC),
LintId::of(operators::FLOAT_CMP_CONST),
LintId::of(operators::INTEGER_ARITHMETIC),
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| Box::new(utils::internal_lints::MsrvAttrImpl));
}

let arithmetic_allowed = conf.arithmetic_allowed.clone();
store.register_late_pass(move || Box::new(operators::arithmetic::Arithmetic::new(arithmetic_allowed.clone())));
store.register_late_pass(|| Box::new(utils::dump_hir::DumpHir));
store.register_late_pass(|| Box::new(utils::author::Author));
let await_holding_invalid_types = conf.await_holding_invalid_types.clone();
Expand Down
119 changes: 119 additions & 0 deletions clippy_lints/src/operators/arithmetic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
#![allow(
// False positive
clippy::match_same_arms
)]

use super::ARITHMETIC;
use clippy_utils::{consts::constant_simple, diagnostics::span_lint};
use rustc_data_structures::fx::FxHashSet;
use rustc_hir as hir;
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::impl_lint_pass;
use rustc_span::source_map::Span;

const HARD_CODED_ALLOWED: &[&str] = &["std::num::Saturating", "std::string::String", "std::num::Wrapping"];

#[derive(Debug)]
pub struct Arithmetic {
allowed: FxHashSet<String>,
// Used to check whether expressions are constants, such as in enum discriminants and consts
const_span: Option<Span>,
expr_span: Option<Span>,
}

impl_lint_pass!(Arithmetic => [ARITHMETIC]);

impl Arithmetic {
#[must_use]
pub fn new(mut allowed: FxHashSet<String>) -> Self {
allowed.extend(HARD_CODED_ALLOWED.iter().copied().map(String::from));
Self {
allowed,
const_span: None,
expr_span: None,
}
}

/// 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(
cx.typeck_results()
.expr_ty(expr)
.to_string()
.split('<')
.next()
.unwrap_or_default(),
)
}

fn issue_lint(&mut self, cx: &LateContext<'_>, expr: &hir::Expr<'_>) {
span_lint(cx, ARITHMETIC, expr.span, "arithmetic detected");
self.expr_span = Some(expr.span);
}
}

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) {
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);
},
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);
}
},
_ => {},
}
}

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 => {},
}
}

fn check_body_post(&mut self, cx: &LateContext<'_>, body: &hir::Body<'_>) {
let body_owner = cx.tcx.hir().body_owner(body.id());
let body_span = cx.tcx.hir().span(body_owner);
if let Some(span) = self.const_span && span.contains(body_span) {
return;
}
self.const_span = None;
}

fn check_expr_post(&mut self, _: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
if Some(expr.span) == self.expr_span {
self.expr_span = None;
}
}
}
39 changes: 39 additions & 0 deletions clippy_lints/src/operators/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ use rustc_hir::{Body, Expr, ExprKind, UnOp};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_tool_lint, impl_lint_pass};

pub(crate) mod arithmetic;

mod absurd_extreme_comparisons;
mod assign_op_pattern;
mod bit_mask;
Expand Down Expand Up @@ -57,6 +59,42 @@ declare_clippy_lint! {
"a comparison with a maximum or minimum value that is always true or false"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for 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.
///
/// ### 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.
///
/// #### Example
/// ```rust
/// # let a = 0;
/// a + 1;
/// ```
///
/// Third-party types also tend to overflow.
///
/// #### Example
/// ```ignore,rust
/// use rust_decimal::Decimal;
/// let _n = Decimal::MAX + Decimal::MAX;
/// ```
///
/// ### Allowed types
/// Custom allowed types can be specified through the "arithmetic-allowed" filter.
#[clippy::version = "1.64.0"]
pub ARITHMETIC,
restriction,
"any arithmetic expression that could overflow or panic"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for integer arithmetic operations which could overflow or panic.
Expand Down Expand Up @@ -747,6 +785,7 @@ pub struct Operators {
}
impl_lint_pass!(Operators => [
ABSURD_EXTREME_COMPARISONS,
ARITHMETIC,
INTEGER_ARITHMETIC,
FLOAT_ARITHMETIC,
ASSIGN_OP_PATTERN,
Expand Down
4 changes: 4 additions & 0 deletions clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,10 @@ macro_rules! define_Conf {
}

define_Conf! {
/// Lint: Arithmetic.
///
/// Suppress checking of the passed type names.
(arithmetic_allowed: rustc_data_structures::fx::FxHashSet<String> = <_>::default()),
/// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UNUSED_SELF, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX.
///
/// Suppress lints whenever the suggested change would cause breakage for other crates.
Expand Down
24 changes: 24 additions & 0 deletions tests/ui-toml/arithmetic_allowed/arithmetic_allowed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#![warn(clippy::arithmetic)]

use core::ops::Add;

#[derive(Clone, Copy)]
struct Point {
x: i32,
y: i32,
}

impl Add for Point {
type Output = Self;

fn add(self, other: Self) -> Self {
todo!()
}
}

fn main() {
let _ = Point { x: 1, y: 0 } + Point { x: 2, y: 3 };

let point: Point = Point { x: 1, y: 0 };
let _ = point + point;
}
1 change: 1 addition & 0 deletions tests/ui-toml/arithmetic_allowed/clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
arithmetic-allowed = ["Point"]
1 change: 1 addition & 0 deletions tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown fie
allow-expect-in-tests
allow-unwrap-in-tests
allowed-scripts
arithmetic-allowed
array-size-threshold
avoid-breaking-exported-api
await-holding-invalid-types
Expand Down
27 changes: 27 additions & 0 deletions tests/ui/arithmetic.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// 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<u32> = Saturating(0u32);
let string: String = String::new();
let wrapping: Wrapping<u32> = 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() {}
27 changes: 27 additions & 0 deletions tests/ui/arithmetic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// 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<u32> = Saturating(0u32);
let string: String = String::new();
let wrapping: Wrapping<u32> = 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() {}