From aa20277a171c108b69dc7241e40bac41c5e1c84b Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 13 Feb 2018 15:40:17 +0100 Subject: [PATCH] Lint for suspicious implementations of arithmetic std::ops traits --- clippy_lints/src/lib.rs | 4 + clippy_lints/src/suspicious_trait_impl.rs | 192 +++++++++++++++++++++ tests/ui/suspicious_arithmetic_impl.rs | 52 ++++++ tests/ui/suspicious_arithmetic_impl.stderr | 18 ++ 4 files changed, 266 insertions(+) create mode 100644 clippy_lints/src/suspicious_trait_impl.rs create mode 100644 tests/ui/suspicious_arithmetic_impl.rs create mode 100644 tests/ui/suspicious_arithmetic_impl.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bec46ba180c..cda5306dd2a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -158,6 +158,7 @@ pub mod returns; pub mod serde_api; pub mod shadow; pub mod strings; +pub mod suspicious_trait_impl; pub mod swap; pub mod temporary_assignment; pub mod transmute; @@ -373,6 +374,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box types::UnitArg); reg.register_late_lint_pass(box double_comparison::DoubleComparisonPass); reg.register_late_lint_pass(box question_mark::QuestionMarkPass); + reg.register_late_lint_pass(box suspicious_trait_impl::SuspiciousImpl); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -599,6 +601,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { returns::NEEDLESS_RETURN, serde_api::SERDE_API_MISUSE, strings::STRING_LIT_AS_BYTES, + suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL, + suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL, swap::ALMOST_SWAPPED, swap::MANUAL_SWAP, temporary_assignment::TEMPORARY_ASSIGNMENT, diff --git a/clippy_lints/src/suspicious_trait_impl.rs b/clippy_lints/src/suspicious_trait_impl.rs new file mode 100644 index 00000000000..682ab4d15ea --- /dev/null +++ b/clippy_lints/src/suspicious_trait_impl.rs @@ -0,0 +1,192 @@ +use rustc::lint::*; +use rustc::hir; +use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use syntax::ast; +use utils::{get_trait_def_id, span_lint}; + +/// **What it does:** Lints for suspicious operations in impls of arithmetic operators, e.g. +/// subtracting elements in an Add impl. +/// +/// **Why this is bad?** This is probably a typo or copy-and-paste error and not intended. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// impl Add for Foo { +/// type Output = Foo; +/// +/// fn add(self, other: Foo) -> Foo { +/// Foo(self.0 - other.0) +/// } +/// } +/// ``` +declare_lint! { + pub SUSPICIOUS_ARITHMETIC_IMPL, + Warn, + "suspicious use of operators in impl of arithmetic trait" +} + +/// **What it does:** Lints for suspicious operations in impls of OpAssign, e.g. +/// subtracting elements in an AddAssign impl. +/// +/// **Why this is bad?** This is probably a typo or copy-and-paste error and not intended. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// impl AddAssign for Foo { +/// fn add_assign(&mut self, other: Foo) { +/// *self = *self - other; +/// } +/// } +/// ``` +declare_lint! { + pub SUSPICIOUS_OP_ASSIGN_IMPL, + Warn, + "suspicious use of operators in impl of OpAssign trait" +} + +#[derive(Copy, Clone)] +pub struct SuspiciousImpl; + +impl LintPass for SuspiciousImpl { + fn get_lints(&self) -> LintArray { + lint_array![SUSPICIOUS_ARITHMETIC_IMPL, SUSPICIOUS_OP_ASSIGN_IMPL] + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for SuspiciousImpl { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) { + use rustc::hir::BinOp_::*; + if let hir::ExprBinary(binop, _, _) = expr.node { + // Check if the binary expression is part of another binary expression + // as a child node + let mut parent_expr = cx.tcx.hir.get_parent_node(expr.id); + while parent_expr != ast::CRATE_NODE_ID { + if_chain! { + if let hir::map::Node::NodeExpr(e) = cx.tcx.hir.get(parent_expr); + if let hir::ExprBinary(_, _, _) = e.node; + then { + return + } + } + + parent_expr = cx.tcx.hir.get_parent_node(parent_expr); + } + // as a parent node + let mut visitor = BinaryExprVisitor { + in_binary_expr: false, + }; + walk_expr(&mut visitor, expr); + + if visitor.in_binary_expr { + return; + } + + if let Some(impl_trait) = check_binop( + cx, + expr, + &binop.node, + &["Add", "Sub", "Mul", "Div"], + &[BiAdd, BiSub, BiMul, BiDiv], + ) { + span_lint( + cx, + SUSPICIOUS_ARITHMETIC_IMPL, + binop.span, + &format!( + r#"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", + ], + &[ + BiAdd, BiSub, BiMul, BiDiv, BiBitAnd, BiBitOr, BiBitXor, BiRem, BiShl, BiShr + ], + ) { + span_lint( + cx, + SUSPICIOUS_OP_ASSIGN_IMPL, + binop.span, + &format!( + r#"Suspicious use of binary operator in `{}` impl"#, + impl_trait + ), + ); + } + } + } +} + +fn check_binop<'a>( + cx: &LateContext, + expr: &hir::Expr, + binop: &hir::BinOp_, + traits: &[&'a str], + expected_ops: &[hir::BinOp_], +) -> Option<&'a str> { + let mut trait_ids = vec![]; + let [krate, module] = ::utils::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(expr.id); + let parent_impl = cx.tcx.hir.get_parent(parent_fn); + + if_chain! { + if parent_impl != ast::CRATE_NODE_ID; + if let hir::map::Node::NodeItem(item) = cx.tcx.hir.get(parent_impl); + if let hir::Item_::ItemImpl(_, _, _, _, Some(ref trait_ref), _, _) = item.node; + if let Some(idx) = trait_ids.iter().position(|&tid| tid == trait_ref.path.def.def_id()); + if *binop != expected_ops[idx]; + then{ + return Some(traits[idx]) + } + } + + None +} + +struct BinaryExprVisitor { + in_binary_expr: bool, +} + +impl<'a, 'tcx: 'a> Visitor<'tcx> for BinaryExprVisitor { + fn visit_expr(&mut self, expr: &'tcx hir::Expr) { + if let hir::ExprBinary(_, _, _) = expr.node { + self.in_binary_expr = true; + } + + walk_expr(self, expr); + } + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +} diff --git a/tests/ui/suspicious_arithmetic_impl.rs b/tests/ui/suspicious_arithmetic_impl.rs new file mode 100644 index 00000000000..097627e1d7c --- /dev/null +++ b/tests/ui/suspicious_arithmetic_impl.rs @@ -0,0 +1,52 @@ + + + +#![warn(suspicious_arithmetic_impl)] +use std::ops::{Add, AddAssign, Mul, Sub, Div}; + +#[derive(Copy, Clone)] +struct Foo(u32); + +impl Add for Foo { + type Output = Foo; + + fn add(self, other: Self) -> Self { + Foo(self.0 - other.0) + } +} + +impl AddAssign for Foo { + fn add_assign(&mut self, other: Foo) { + *self = *self - other; + } +} + +impl Mul for Foo { + type Output = Foo; + + fn mul(self, other: Foo) -> Foo { + Foo(self.0 * other.0 % 42) // OK: BiRem part of BiExpr as parent node + } +} + +impl Sub for Foo { + type Output = Foo; + + fn sub(self, other: Self) -> Self { + Foo(self.0 * other.0 - 42) // OK: BiMul part of BiExpr as child node + } +} + +impl Div for Foo { + type Output = Foo; + + fn div(self, other: Self) -> Self { + Foo(do_nothing(self.0 + other.0) / 42) // OK: BiAdd part of BiExpr as child node + } +} + +fn main() {} + +fn do_nothing(x: u32) -> u32 { + x +} diff --git a/tests/ui/suspicious_arithmetic_impl.stderr b/tests/ui/suspicious_arithmetic_impl.stderr new file mode 100644 index 00000000000..9d5086e5497 --- /dev/null +++ b/tests/ui/suspicious_arithmetic_impl.stderr @@ -0,0 +1,18 @@ +error: Suspicious use of binary operator in `Add` impl + --> $DIR/suspicious_arithmetic_impl.rs:14:20 + | +14 | Foo(self.0 - other.0) + | ^ + | + = note: `-D suspicious-arithmetic-impl` implied by `-D warnings` + +error: Suspicious use of binary operator in `AddAssign` impl + --> $DIR/suspicious_arithmetic_impl.rs:20:23 + | +20 | *self = *self - other; + | ^ + | + = note: `-D suspicious-op-assign-impl` implied by `-D warnings` + +error: aborting due to 2 previous errors +