diff options
Diffstat (limited to 'src/tools/clippy/clippy_lints/src/operators')
7 files changed, 195 insertions, 137 deletions
diff --git a/src/tools/clippy/clippy_lints/src/operators/arithmetic.rs b/src/tools/clippy/clippy_lints/src/operators/arithmetic.rs deleted file mode 100644 index 800cf249f..000000000 --- a/src/tools/clippy/clippy_lints/src/operators/arithmetic.rs +++ /dev/null @@ -1,119 +0,0 @@ -#![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; - } - } -} diff --git a/src/tools/clippy/clippy_lints/src/operators/arithmetic_side_effects.rs b/src/tools/clippy/clippy_lints/src/operators/arithmetic_side_effects.rs new file mode 100644 index 000000000..83b69fbb3 --- /dev/null +++ b/src/tools/clippy/clippy_lints/src/operators/arithmetic_side_effects.rs @@ -0,0 +1,173 @@ +#![allow( + // False positive + clippy::match_same_arms +)] + +use super::ARITHMETIC_SIDE_EFFECTS; +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, Spanned}; + +const HARD_CODED_ALLOWED: &[&str] = &[ + "f32", + "f64", + "std::num::Saturating", + "std::string::String", + "std::num::Wrapping", +]; + +#[derive(Debug)] +pub struct ArithmeticSideEffects { + 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!(ArithmeticSideEffects => [ARITHMETIC_SIDE_EFFECTS]); + +impl ArithmeticSideEffects { + #[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 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( + cx.typeck_results() + .expr_ty(expr) + .to_string() + .split('<') + .next() + .unwrap_or_default(), + ) + } + + /// 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_SIDE_EFFECTS, 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 ArithmeticSideEffects { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { + 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) => { + self.manage_bin_ops(cx, expr, op, lhs, rhs); + }, + hir::ExprKind::Unary(hir::UnOp::Neg, _) => { + 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(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); + } + } + + 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; + } + } +} diff --git a/src/tools/clippy/clippy_lints/src/operators/cmp_owned.rs b/src/tools/clippy/clippy_lints/src/operators/cmp_owned.rs index e1f9b5906..638a514ff 100644 --- a/src/tools/clippy/clippy_lints/src/operators/cmp_owned.rs +++ b/src/tools/clippy/clippy_lints/src/operators/cmp_owned.rs @@ -38,7 +38,7 @@ fn symmetric_partial_eq<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, other: Ty<'t fn check_op(cx: &LateContext<'_>, expr: &Expr<'_>, other: &Expr<'_>, left: bool) { let typeck = cx.typeck_results(); let (arg, arg_span) = match expr.kind { - ExprKind::MethodCall(.., [arg], _) + ExprKind::MethodCall(_, arg, [], _) if typeck .type_dependent_def_id(expr.hir_id) .and_then(|id| cx.tcx.trait_of_item(id)) diff --git a/src/tools/clippy/clippy_lints/src/operators/duration_subsec.rs b/src/tools/clippy/clippy_lints/src/operators/duration_subsec.rs index 0d067d1e1..827a2b267 100644 --- a/src/tools/clippy/clippy_lints/src/operators/duration_subsec.rs +++ b/src/tools/clippy/clippy_lints/src/operators/duration_subsec.rs @@ -17,7 +17,7 @@ pub(crate) fn check<'tcx>( right: &'tcx Expr<'_>, ) { if op == BinOpKind::Div - && let ExprKind::MethodCall(method_path, [self_arg], _) = left.kind + && let ExprKind::MethodCall(method_path, self_arg, [], _) = left.kind && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_arg).peel_refs(), sym::Duration) && let Some((Constant::Int(divisor), _)) = constant(cx, cx.typeck_results(), right) { diff --git a/src/tools/clippy/clippy_lints/src/operators/float_cmp.rs b/src/tools/clippy/clippy_lints/src/operators/float_cmp.rs index 0ef793443..97ddcdb24 100644 --- a/src/tools/clippy/clippy_lints/src/operators/float_cmp.rs +++ b/src/tools/clippy/clippy_lints/src/operators/float_cmp.rs @@ -113,7 +113,7 @@ fn is_signum(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { } if_chain! { - if let ExprKind::MethodCall(method_name, [ref self_arg, ..], _) = expr.kind; + if let ExprKind::MethodCall(method_name, self_arg, ..) = expr.kind; if sym!(signum) == method_name.ident.name; // Check that the receiver of the signum() is a float (expressions[0] is the receiver of // the method call) diff --git a/src/tools/clippy/clippy_lints/src/operators/mod.rs b/src/tools/clippy/clippy_lints/src/operators/mod.rs index bb6d99406..c32b4df4f 100644 --- a/src/tools/clippy/clippy_lints/src/operators/mod.rs +++ b/src/tools/clippy/clippy_lints/src/operators/mod.rs @@ -21,7 +21,7 @@ mod ptr_eq; mod self_assignment; mod verbose_bit_mask; -pub(crate) mod arithmetic; +pub(crate) mod arithmetic_side_effects; use rustc_hir::{Body, Expr, ExprKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; @@ -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 @@ -88,11 +92,11 @@ declare_clippy_lint! { /// ``` /// /// ### Allowed types - /// Custom allowed types can be specified through the "arithmetic-allowed" filter. + /// Custom allowed types can be specified through the "arithmetic-side-effects-allowed" filter. #[clippy::version = "1.64.0"] - pub ARITHMETIC, + pub ARITHMETIC_SIDE_EFFECTS, restriction, - "any arithmetic expression that could overflow or panic" + "any arithmetic expression that can cause side effects like overflows or panics" } declare_clippy_lint! { @@ -785,7 +789,7 @@ pub struct Operators { } impl_lint_pass!(Operators => [ ABSURD_EXTREME_COMPARISONS, - ARITHMETIC, + ARITHMETIC_SIDE_EFFECTS, INTEGER_ARITHMETIC, FLOAT_ARITHMETIC, ASSIGN_OP_PATTERN, diff --git a/src/tools/clippy/clippy_lints/src/operators/op_ref.rs b/src/tools/clippy/clippy_lints/src/operators/op_ref.rs index 1805672e3..1085e6089 100644 --- a/src/tools/clippy/clippy_lints/src/operators/op_ref.rs +++ b/src/tools/clippy/clippy_lints/src/operators/op_ref.rs @@ -185,7 +185,7 @@ fn in_impl<'tcx>( if let ItemKind::Impl(item) = &item.kind; if let Some(of_trait) = &item.of_trait; if let Some(seg) = of_trait.path.segments.last(); - if let Some(Res::Def(_, trait_id)) = seg.res; + if let Res::Def(_, trait_id) = seg.res; if trait_id == bin_op; if let Some(generic_args) = seg.args; if let Some(GenericArg::Type(other_ty)) = generic_args.args.last(); |