diff options
| -rw-r--r-- | clippy_lints/src/assign_ops.rs | 91 | ||||
| -rw-r--r-- | clippy_lints/src/suspicious_trait_impl.rs | 143 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 31 | ||||
| -rw-r--r-- | tests/ui/suspicious_arithmetic_impl.stderr | 18 |
4 files changed, 95 insertions, 188 deletions
diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index 42194191837..17ce3cd809f 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -1,8 +1,8 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet_opt; use clippy_utils::ty::implements_trait; -use clippy_utils::{eq_expr_value, get_trait_def_id, trait_ref_of_method}; -use clippy_utils::{paths, sugg}; +use clippy_utils::{binop_traits, sugg}; +use clippy_utils::{eq_expr_value, trait_ref_of_method}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; @@ -85,71 +85,34 @@ impl<'tcx> LateLintPass<'tcx> for AssignOps { let lint = |assignee: &hir::Expr<'_>, rhs: &hir::Expr<'_>| { let ty = cx.typeck_results().expr_ty(assignee); let rty = cx.typeck_results().expr_ty(rhs); - macro_rules! ops { - ($op:expr, - $cx:expr, - $ty:expr, - $rty:expr, - $($trait_name:ident),+) => { - match $op { - $(hir::BinOpKind::$trait_name => { - let [krate, module] = paths::OPS_MODULE; - let path: [&str; 3] = [krate, module, concat!(stringify!($trait_name), "Assign")]; - let trait_id = if let Some(trait_id) = get_trait_def_id($cx, &path) { - trait_id - } else { - return; // useless if the trait doesn't exist - }; - // check that we are not inside an `impl AssignOp` of this exact operation - let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id); - if_chain! { - if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn); - if trait_ref.path.res.def_id() == trait_id; - then { return; } + if_chain! { + if let Some((_, lang_item)) = binop_traits(op.node); + if let Ok(trait_id) = cx.tcx.lang_items().require(lang_item); + let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id); + if trait_ref_of_method(cx, parent_fn) + .map_or(true, |t| t.path.res.def_id() != trait_id); + if implements_trait(cx, ty, trait_id, &[rty.into()]); + then { + span_lint_and_then( + cx, + ASSIGN_OP_PATTERN, + expr.span, + "manual implementation of an assign operation", + |diag| { + if let (Some(snip_a), Some(snip_r)) = + (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span)) + { + diag.span_suggestion( + expr.span, + "replace it with", + format!("{} {}= {}", snip_a, op.node.as_str(), snip_r), + Applicability::MachineApplicable, + ); } - implements_trait($cx, $ty, trait_id, &[$rty]) - },)* - _ => false, - } + }, + ); } } - if ops!( - op.node, - cx, - ty, - rty.into(), - Add, - Sub, - Mul, - Div, - Rem, - And, - Or, - BitAnd, - BitOr, - BitXor, - Shr, - Shl - ) { - span_lint_and_then( - cx, - ASSIGN_OP_PATTERN, - expr.span, - "manual implementation of an assign operation", - |diag| { - if let (Some(snip_a), Some(snip_r)) = - (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span)) - { - diag.span_suggestion( - expr.span, - "replace it with", - format!("{} {}= {}", snip_a, op.node.as_str(), snip_r), - Applicability::MachineApplicable, - ); - } - }, - ); - } }; let mut visitor = ExprVisitor { diff --git a/clippy_lints/src/suspicious_trait_impl.rs b/clippy_lints/src/suspicious_trait_impl.rs index 2203ab57b10..f2bffd55321 100644 --- a/clippy_lints/src/suspicious_trait_impl.rs +++ b/clippy_lints/src/suspicious_trait_impl.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint; -use clippy_utils::{get_trait_def_id, paths, trait_ref_of_method}; +use clippy_utils::{binop_traits, trait_ref_of_method, BINOP_TRAITS, OP_ASSIGN_TRAITS}; use if_chain::if_chain; use rustc_hir as hir; use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; @@ -55,135 +55,48 @@ declare_lint_pass!(SuspiciousImpl => [SUSPICIOUS_ARITHMETIC_IMPL, SUSPICIOUS_OP_ impl<'tcx> LateLintPass<'tcx> for SuspiciousImpl { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { - if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind { - match binop.node { - hir::BinOpKind::Eq - | hir::BinOpKind::Lt - | hir::BinOpKind::Le - | hir::BinOpKind::Ne - | hir::BinOpKind::Ge - | hir::BinOpKind::Gt => return, - _ => {}, - } + if_chain! { + if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind; + if let Some((binop_trait_lang, op_assign_trait_lang)) = binop_traits(binop.node); + if let Ok(binop_trait_id) = cx.tcx.lang_items().require(binop_trait_lang); + if let Ok(op_assign_trait_id) = cx.tcx.lang_items().require(op_assign_trait_lang); // Check for more than one binary operation in the implemented function // Linting when multiple operations are involved can result in false positives let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id); - if_chain! { - if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn); - if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind; - then { - let body = cx.tcx.hir().body(body_id); - let mut visitor = BinaryExprVisitor { nb_binops: 0 }; - walk_expr(&mut visitor, &body.value); - if visitor.nb_binops > 1 { - return; - } - } - } - - if let Some(impl_trait) = check_binop( - cx, - expr, - binop.node, - &[ - "Add", "Sub", "Mul", "Div", "Rem", "BitAnd", "BitOr", "BitXor", "Shl", "Shr", - ], - &[ - hir::BinOpKind::Add, - hir::BinOpKind::Sub, - hir::BinOpKind::Mul, - hir::BinOpKind::Div, - hir::BinOpKind::Rem, - hir::BinOpKind::BitAnd, - hir::BinOpKind::BitOr, - hir::BinOpKind::BitXor, - hir::BinOpKind::Shl, - hir::BinOpKind::Shr, - ], - ) { - span_lint( - cx, - SUSPICIOUS_ARITHMETIC_IMPL, - binop.span, - &format!("suspicious use of binary operator in `{}` impl", impl_trait), - ); - } - - if let Some(impl_trait) = check_binop( - cx, - expr, - binop.node, - &[ - "AddAssign", - "SubAssign", - "MulAssign", - "DivAssign", - "BitAndAssign", - "BitOrAssign", - "BitXorAssign", - "RemAssign", - "ShlAssign", - "ShrAssign", - ], - &[ - hir::BinOpKind::Add, - hir::BinOpKind::Sub, - hir::BinOpKind::Mul, - hir::BinOpKind::Div, - hir::BinOpKind::BitAnd, - hir::BinOpKind::BitOr, - hir::BinOpKind::BitXor, - hir::BinOpKind::Rem, - hir::BinOpKind::Shl, - hir::BinOpKind::Shr, - ], - ) { + if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn); + if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind; + let body = cx.tcx.hir().body(body_id); + let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id); + if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn); + let trait_id = trait_ref.path.res.def_id(); + if ![binop_trait_id, op_assign_trait_id].contains(&trait_id); + if let Some(&(_, lint)) = [ + (&BINOP_TRAITS, SUSPICIOUS_ARITHMETIC_IMPL), + (&OP_ASSIGN_TRAITS, SUSPICIOUS_OP_ASSIGN_IMPL), + ] + .iter() + .find(|&(ts, _)| ts.iter().any(|&t| Ok(trait_id) == cx.tcx.lang_items().require(t))); + if count_binops(&body.value) == 1; + then { span_lint( cx, - SUSPICIOUS_OP_ASSIGN_IMPL, + lint, binop.span, - &format!("suspicious use of binary operator in `{}` impl", impl_trait), + &format!("suspicious use of `{}` in `{}` impl", binop.node.as_str(), cx.tcx.item_name(trait_id)), ); } } } } -fn check_binop( - cx: &LateContext<'_>, - expr: &hir::Expr<'_>, - binop: hir::BinOpKind, - traits: &[&'static str], - expected_ops: &[hir::BinOpKind], -) -> Option<&'static str> { - let mut trait_ids = vec![]; - let [krate, module] = paths::OPS_MODULE; - - for &t in traits { - let path = [krate, module, t]; - if let Some(trait_id) = get_trait_def_id(cx, &path) { - trait_ids.push(trait_id); - } else { - return None; - } - } - - // Get the actually implemented trait - let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id); - - if_chain! { - if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn); - if let Some(idx) = trait_ids.iter().position(|&tid| tid == trait_ref.path.res.def_id()); - if binop != expected_ops[idx]; - then{ - return Some(traits[idx]) - } - } - - None +fn count_binops(expr: &hir::Expr<'_>) -> u32 { + let mut visitor = BinaryExprVisitor::default(); + visitor.visit_expr(expr); + visitor.nb_binops } +#[derive(Default)] struct BinaryExprVisitor { nb_binops: u32, } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 6db221ab0fd..1fbbf4ac81f 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1710,3 +1710,34 @@ pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool { matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().contains("test") } + +macro_rules! op_utils { + ($($name:ident $assign:ident)*) => { + /// Binary operation traits like `LangItem::Add` + pub static BINOP_TRAITS: &[LangItem] = &[$(LangItem::$name,)*]; + + /// Operator-Assign traits like `LangItem::AddAssign` + pub static OP_ASSIGN_TRAITS: &[LangItem] = &[$(LangItem::$assign,)*]; + + /// Converts `BinOpKind::Add` to `(LangItem::Add, LangItem::AddAssign)`, for example + pub fn binop_traits(kind: hir::BinOpKind) -> Option<(LangItem, LangItem)> { + match kind { + $(hir::BinOpKind::$name => Some((LangItem::$name, LangItem::$assign)),)* + _ => None, + } + } + }; +} + +op_utils! { + Add AddAssign + Sub SubAssign + Mul MulAssign + Div DivAssign + Rem RemAssign + BitXor BitXorAssign + BitAnd BitAndAssign + BitOr BitOrAssign + Shl ShlAssign + Shr ShrAssign +} diff --git a/tests/ui/suspicious_arithmetic_impl.stderr b/tests/ui/suspicious_arithmetic_impl.stderr index 63fc9ecb79a..ced1305874e 100644 --- a/tests/ui/suspicious_arithmetic_impl.stderr +++ b/tests/ui/suspicious_arithmetic_impl.stderr @@ -1,4 +1,4 @@ -error: suspicious use of binary operator in `Add` impl +error: suspicious use of `-` in `Add` impl --> $DIR/suspicious_arithmetic_impl.rs:13:20 | LL | Foo(self.0 - other.0) @@ -6,7 +6,7 @@ LL | Foo(self.0 - other.0) | = note: `-D clippy::suspicious-arithmetic-impl` implied by `-D warnings` -error: suspicious use of binary operator in `AddAssign` impl +error: suspicious use of `-` in `AddAssign` impl --> $DIR/suspicious_arithmetic_impl.rs:19:23 | LL | *self = *self - other; @@ -14,43 +14,43 @@ LL | *self = *self - other; | = note: `-D clippy::suspicious-op-assign-impl` implied by `-D warnings` -error: suspicious use of binary operator in `MulAssign` impl +error: suspicious use of `/` in `MulAssign` impl --> $DIR/suspicious_arithmetic_impl.rs:32:16 | LL | self.0 /= other.0; | ^^ -error: suspicious use of binary operator in `Rem` impl +error: suspicious use of `/` in `Rem` impl --> $DIR/suspicious_arithmetic_impl.rs:70:20 | LL | Foo(self.0 / other.0) | ^ -error: suspicious use of binary operator in `BitAnd` impl +error: suspicious use of `|` in `BitAnd` impl --> $DIR/suspicious_arithmetic_impl.rs:78:20 | LL | Foo(self.0 | other.0) | ^ -error: suspicious use of binary operator in `BitOr` impl +error: suspicious use of `^` in `BitOr` impl --> $DIR/suspicious_arithmetic_impl.rs:86:20 | LL | Foo(self.0 ^ other.0) | ^ -error: suspicious use of binary operator in `BitXor` impl +error: suspicious use of `&` in `BitXor` impl --> $DIR/suspicious_arithmetic_impl.rs:94:20 | LL | Foo(self.0 & other.0) | ^ -error: suspicious use of binary operator in `Shl` impl +error: suspicious use of `>>` in `Shl` impl --> $DIR/suspicious_arithmetic_impl.rs:102:20 | LL | Foo(self.0 >> other.0) | ^^ -error: suspicious use of binary operator in `Shr` impl +error: suspicious use of `<<` in `Shr` impl --> $DIR/suspicious_arithmetic_impl.rs:110:20 | LL | Foo(self.0 << other.0) |