From 83de67cfec4a1e68c8b63ec8153777c51a19addc Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 1 Jun 2022 01:23:57 -0400 Subject: [PATCH] Move `IdentityOp` into `Operators` lint pass --- clippy_lints/src/lib.register_all.rs | 2 +- clippy_lints/src/lib.register_complexity.rs | 2 +- clippy_lints/src/lib.register_lints.rs | 2 +- clippy_lints/src/lib.rs | 2 - .../src/{ => operators}/identity_op.rs | 87 +++++++------------ clippy_lints/src/operators/mod.rs | 22 +++++ 6 files changed, 58 insertions(+), 59 deletions(-) rename clippy_lints/src/{ => operators}/identity_op.rs (60%) diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 708038c8e22a..fb08d7d112d2 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -83,7 +83,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(functions::RESULT_UNIT_ERR), LintId::of(functions::TOO_MANY_ARGUMENTS), LintId::of(get_first::GET_FIRST), - LintId::of(identity_op::IDENTITY_OP), LintId::of(if_let_mutex::IF_LET_MUTEX), LintId::of(indexing_slicing::OUT_OF_BOUNDS_INDEXING), LintId::of(infinite_iter::INFINITE_ITER), @@ -257,6 +256,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(operators::EQ_OP), LintId::of(operators::ERASING_OP), LintId::of(operators::FLOAT_EQUALITY_WITHOUT_ABS), + LintId::of(operators::IDENTITY_OP), LintId::of(operators::INEFFECTIVE_BIT_MASK), LintId::of(operators::MISREFACTORED_ASSIGN_OP), LintId::of(operators::OP_REF), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index 3a8a00d4ef03..ed5446f58441 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -14,7 +14,6 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(explicit_write::EXPLICIT_WRITE), LintId::of(format::USELESS_FORMAT), LintId::of(functions::TOO_MANY_ARGUMENTS), - LintId::of(identity_op::IDENTITY_OP), LintId::of(int_plus_one::INT_PLUS_ONE), LintId::of(lifetimes::EXTRA_UNUSED_LIFETIMES), LintId::of(lifetimes::NEEDLESS_LIFETIMES), @@ -71,6 +70,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(no_effect::UNNECESSARY_OPERATION), LintId::of(operators::DOUBLE_COMPARISONS), LintId::of(operators::DURATION_SUBSEC), + LintId::of(operators::IDENTITY_OP), LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), LintId::of(partialeq_ne_impl::PARTIALEQ_NE_IMPL), LintId::of(precedence::PRECEDENCE), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index c0de13d1572d..946e8c76f173 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -176,7 +176,6 @@ store.register_lints(&[ functions::TOO_MANY_LINES, future_not_send::FUTURE_NOT_SEND, get_first::GET_FIRST, - identity_op::IDENTITY_OP, if_let_mutex::IF_LET_MUTEX, if_not_else::IF_NOT_ELSE, if_then_some_else_none::IF_THEN_SOME_ELSE_NONE, @@ -432,6 +431,7 @@ store.register_lints(&[ operators::ERASING_OP, operators::FLOAT_ARITHMETIC, operators::FLOAT_EQUALITY_WITHOUT_ABS, + operators::IDENTITY_OP, operators::INEFFECTIVE_BIT_MASK, operators::INTEGER_ARITHMETIC, operators::MISREFACTORED_ASSIGN_OP, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5e2ab9ddbecd..327a1150952e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -239,7 +239,6 @@ mod from_str_radix_10; mod functions; mod future_not_send; mod get_first; -mod identity_op; mod if_let_mutex; mod if_not_else; mod if_then_some_else_none; @@ -582,7 +581,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(needless_for_each::NeedlessForEach)); store.register_late_pass(|| Box::new(misc::MiscLints)); store.register_late_pass(|| Box::new(eta_reduction::EtaReduction)); - store.register_late_pass(|| Box::new(identity_op::IdentityOp)); store.register_late_pass(|| Box::new(mut_mut::MutMut)); store.register_late_pass(|| Box::new(mut_reference::UnnecessaryMutPassed)); store.register_late_pass(|| Box::new(len_zero::LenZero)); diff --git a/clippy_lints/src/identity_op.rs b/clippy_lints/src/operators/identity_op.rs similarity index 60% rename from clippy_lints/src/identity_op.rs rename to clippy_lints/src/operators/identity_op.rs index 419ea5a6811b..b48d6c4e2e2a 100644 --- a/clippy_lints/src/identity_op.rs +++ b/clippy_lints/src/operators/identity_op.rs @@ -3,61 +3,40 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; use clippy_utils::{clip, unsext}; use rustc_errors::Applicability; -use rustc_hir::{BinOp, BinOpKind, Expr, ExprKind, Node}; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_hir::{BinOpKind, Expr, ExprKind, Node}; +use rustc_lint::LateContext; use rustc_middle::ty; -use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; -declare_clippy_lint! { - /// ### What it does - /// Checks for identity operations, e.g., `x + 0`. - /// - /// ### Why is this bad? - /// This code can be removed without changing the - /// meaning. So it just obscures what's going on. Delete it mercilessly. - /// - /// ### Example - /// ```rust - /// # let x = 1; - /// x / 1 + 0 * 1 - 0 | 0; - /// ``` - #[clippy::version = "pre 1.29.0"] - pub IDENTITY_OP, - complexity, - "using identity operations, e.g., `x + 0` or `y / 1`" -} +use super::IDENTITY_OP; -declare_lint_pass!(IdentityOp => [IDENTITY_OP]); - -impl<'tcx> LateLintPass<'tcx> for IdentityOp { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if expr.span.from_expansion() { - return; - } - if let ExprKind::Binary(cmp, left, right) = &expr.kind { - if !is_allowed(cx, *cmp, left, right) { - match cmp.node { - BinOpKind::Add | BinOpKind::BitOr | BinOpKind::BitXor => { - check(cx, left, 0, expr.span, right.span, needs_parenthesis(cx, expr, right)); - check(cx, right, 0, expr.span, left.span, Parens::Unneeded); - }, - BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => { - check(cx, right, 0, expr.span, left.span, Parens::Unneeded); - }, - BinOpKind::Mul => { - check(cx, left, 1, expr.span, right.span, needs_parenthesis(cx, expr, right)); - check(cx, right, 1, expr.span, left.span, Parens::Unneeded); - }, - BinOpKind::Div => check(cx, right, 1, expr.span, left.span, Parens::Unneeded), - BinOpKind::BitAnd => { - check(cx, left, -1, expr.span, right.span, needs_parenthesis(cx, expr, right)); - check(cx, right, -1, expr.span, left.span, Parens::Unneeded); - }, - BinOpKind::Rem => check_remainder(cx, left, right, expr.span, left.span), - _ => (), - } - } +pub(crate) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + op: BinOpKind, + left: &'tcx Expr<'_>, + right: &'tcx Expr<'_>, +) { + if !is_allowed(cx, op, left, right) { + match op { + BinOpKind::Add | BinOpKind::BitOr | BinOpKind::BitXor => { + check_op(cx, left, 0, expr.span, right.span, needs_parenthesis(cx, expr, right)); + check_op(cx, right, 0, expr.span, left.span, Parens::Unneeded); + }, + BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => { + check_op(cx, right, 0, expr.span, left.span, Parens::Unneeded); + }, + BinOpKind::Mul => { + check_op(cx, left, 1, expr.span, right.span, needs_parenthesis(cx, expr, right)); + check_op(cx, right, 1, expr.span, left.span, Parens::Unneeded); + }, + BinOpKind::Div => check_op(cx, right, 1, expr.span, left.span, Parens::Unneeded), + BinOpKind::BitAnd => { + check_op(cx, left, -1, expr.span, right.span, needs_parenthesis(cx, expr, right)); + check_op(cx, right, -1, expr.span, left.span, Parens::Unneeded); + }, + BinOpKind::Rem => check_remainder(cx, left, right, expr.span, left.span), + _ => (), } } } @@ -108,12 +87,12 @@ fn needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, right: &Expr<'_>) Parens::Needed } -fn is_allowed(cx: &LateContext<'_>, cmp: BinOp, left: &Expr<'_>, right: &Expr<'_>) -> bool { +fn is_allowed(cx: &LateContext<'_>, cmp: BinOpKind, left: &Expr<'_>, right: &Expr<'_>) -> bool { // This lint applies to integers !cx.typeck_results().expr_ty(left).peel_refs().is_integral() || !cx.typeck_results().expr_ty(right).peel_refs().is_integral() // `1 << 0` is a common pattern in bit manipulation code - || (cmp.node == BinOpKind::Shl + || (cmp == BinOpKind::Shl && constant_simple(cx, cx.typeck_results(), right) == Some(Constant::Int(0)) && constant_simple(cx, cx.typeck_results(), left) == Some(Constant::Int(1))) } @@ -130,7 +109,7 @@ fn check_remainder(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>, span } } -fn check(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span, parens: Parens) { +fn check_op(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span, parens: Parens) { if let Some(Constant::Int(v)) = constant_simple(cx, cx.typeck_results(), e).map(Constant::peel_refs) { let check = match *cx.typeck_results().expr_ty(e).peel_refs().kind() { ty::Int(ity) => unsext(cx.tcx, -1_i128, ity), diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index 0786765119b6..91550e0e678b 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -10,6 +10,7 @@ mod duration_subsec; mod eq_op; mod erasing_op; mod float_equality_without_abs; +mod identity_op; mod misrefactored_assign_op; mod numeric_arithmetic; mod op_ref; @@ -417,6 +418,25 @@ declare_clippy_lint! { "float equality check without `.abs()`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for identity operations, e.g., `x + 0`. + /// + /// ### Why is this bad? + /// This code can be removed without changing the + /// meaning. So it just obscures what's going on. Delete it mercilessly. + /// + /// ### Example + /// ```rust + /// # let x = 1; + /// x / 1 + 0 * 1 - 0 | 0; + /// ``` + #[clippy::version = "pre 1.29.0"] + pub IDENTITY_OP, + complexity, + "using identity operations, e.g., `x + 0` or `y / 1`" +} + pub struct Operators { arithmetic_context: numeric_arithmetic::Context, verbose_bit_mask_threshold: u64, @@ -436,6 +456,7 @@ impl_lint_pass!(Operators => [ OP_REF, ERASING_OP, FLOAT_EQUALITY_WITHOUT_ABS, + IDENTITY_OP, ]); impl Operators { pub fn new(verbose_bit_mask_threshold: u64) -> Self { @@ -457,6 +478,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators { op_ref::check(cx, e, op.node, lhs, rhs); } erasing_op::check(cx, e, op.node, lhs, rhs); + identity_op::check(cx, e, op.node, lhs, rhs); } self.arithmetic_context.check_binary(cx, e, op.node, lhs, rhs); bit_mask::check(cx, e, op.node, lhs, rhs);