From 3ea9a249bc7059952c9a9a5932c2bf7596e236ae Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 20 Jul 2016 13:29:01 +0200 Subject: [PATCH 1/3] get snippets inside the suggestions-closure --- clippy_lints/src/assign_ops.rs | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index f394452e6..3c4e2cef0 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -1,6 +1,6 @@ use rustc::hir; use rustc::lint::*; -use utils::{span_lint_and_then, span_lint, snippet_opt, SpanlessEq, get_trait_def_id, implements_trait}; +use utils::{span_lint_and_then, snippet_opt, SpanlessEq, get_trait_def_id, implements_trait}; use utils::{higher, sugg}; /// **What it does:** This lint checks for `+=` operations and similar. @@ -104,23 +104,18 @@ impl LateLintPass for AssignOps { BitXor: BiBitXor, Shr: BiShr, Shl: BiShl) { - if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), - snippet_opt(cx, rhs.span)) { - span_lint_and_then(cx, - ASSIGN_OP_PATTERN, - expr.span, - "manual implementation of an assign operation", - |db| { + span_lint_and_then(cx, + ASSIGN_OP_PATTERN, + expr.span, + "manual implementation of an assign operation", + |db| { + if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), + snippet_opt(cx, rhs.span)) { db.span_suggestion(expr.span, "replace it with", format!("{} {}= {}", snip_a, op.node.as_str(), snip_r)); - }); - } else { - span_lint(cx, - ASSIGN_OP_PATTERN, - expr.span, - "manual implementation of an assign operation"); - } + } + }); } }; // a = a op b From 100d381d2b4c3c595e816971790ebd39647bd849 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 20 Jul 2016 15:29:24 +0200 Subject: [PATCH 2/3] lint `a += a + b` (possible mis-refactoring of `a = a + b`) --- clippy_lints/src/assign_ops.rs | 79 ++++++++++++++++++++++++++++++- tests/compile-fail/assign_ops2.rs | 36 ++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 tests/compile-fail/assign_ops2.rs diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index 3c4e2cef0..3327482f0 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -38,12 +38,31 @@ declare_lint! { "assigning the result of an operation on a variable to that same variable" } +/// **What it does:** Check for `a op= a op b` or `a op= b op a` patterns. +/// +/// **Why is this bad?** Most likely these are bugs where one meant to write `a op= b` +/// +/// **Known problems:** Someone might actually mean `a op= a op b`, but that should rather be written as `a = (2 * a) op b` where applicable. +/// +/// **Example:** +/// +/// ```rust +/// let mut a = 5; +/// ... +/// a += a + b; +/// ``` +declare_lint! { + pub MISREFACTORED_ASSIGN_OP, + Warn, + "having a variable on both sides of an assign op" +} + #[derive(Copy, Clone, Default)] pub struct AssignOps; impl LintPass for AssignOps { fn get_lints(&self) -> LintArray { - lint_array!(ASSIGN_OPS, ASSIGN_OP_PATTERN) + lint_array!(ASSIGN_OPS, ASSIGN_OP_PATTERN, MISREFACTORED_ASSIGN_OP) } } @@ -59,6 +78,40 @@ impl LateLintPass for AssignOps { "replace it with", format!("{} = {}", lhs, sugg::make_binop(higher::binop(op.node), lhs, rhs))); }); + if let hir::ExprBinary(binop, ref l, ref r) = rhs.node { + if op.node == binop.node { + let lint = |assignee: &hir::Expr, rhs: &hir::Expr| { + let ty = cx.tcx.expr_ty(assignee); + if ty.walk_shallow().next().is_some() { + return; // implements_trait does not work with generics + } + let rty = cx.tcx.expr_ty(rhs); + if rty.walk_shallow().next().is_some() { + return; // implements_trait does not work with generics + } + span_lint_and_then(cx, + MISREFACTORED_ASSIGN_OP, + expr.span, + "variable appears on both sides of an assignment operation", + |db| { + if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), + snippet_opt(cx, rhs.span)) { + db.span_suggestion(expr.span, + "replace it with", + format!("{} {}= {}", snip_a, op.node.as_str(), snip_r)); + } + }); + }; + // lhs op= l op r + if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, l) { + lint(lhs, r); + } + // lhs op= l commutative_op r + if is_commutative(op.node) && SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, r) { + lint(lhs, l); + } + } + } } hir::ExprAssign(ref assignee, ref e) => { if let hir::ExprBinary(op, ref l, ref r) = e.node { @@ -138,3 +191,27 @@ impl LateLintPass for AssignOps { } } } + +fn is_commutative(op: hir::BinOp_) -> bool { + use rustc::hir::BinOp_::*; + match op { + BiAdd | + BiMul | + BiAnd | + BiOr | + BiBitXor | + BiBitAnd | + BiBitOr | + BiEq | + BiNe => true, + BiSub | + BiDiv | + BiRem | + BiShl | + BiShr | + BiLt | + BiLe | + BiGe | + BiGt => false, + } +} diff --git a/tests/compile-fail/assign_ops2.rs b/tests/compile-fail/assign_ops2.rs new file mode 100644 index 000000000..1537232bf --- /dev/null +++ b/tests/compile-fail/assign_ops2.rs @@ -0,0 +1,36 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[allow(unused_assignments)] +#[deny(misrefactored_assign_op)] +fn main() { + let mut a = 5; + a += a + 1; //~ ERROR variable appears on both sides of an assignment operation + //~^ HELP replace it with + //~| SUGGESTION a += 1 + a += 1 + a; //~ ERROR variable appears on both sides of an assignment operation + //~^ HELP replace it with + //~| SUGGESTION a += 1 + a -= a - 1; //~ ERROR variable appears on both sides of an assignment operation + //~^ HELP replace it with + //~| SUGGESTION a -= 1 + a *= a * 99; //~ ERROR variable appears on both sides of an assignment operation + //~^ HELP replace it with + //~| SUGGESTION a *= 99 + a *= 42 * a; //~ ERROR variable appears on both sides of an assignment operation + //~^ HELP replace it with + //~| SUGGESTION a *= 42 + a /= a / 2; //~ ERROR variable appears on both sides of an assignment operation + //~^ HELP replace it with + //~| SUGGESTION a /= 2 + a %= a % 5; //~ ERROR variable appears on both sides of an assignment operation + //~^ HELP replace it with + //~| SUGGESTION a %= 5 + a &= a & 1; //~ ERROR variable appears on both sides of an assignment operation + //~^ HELP replace it with + //~| SUGGESTION a &= 1 + a -= 1 - a; + a /= 5 / a; + a %= 42 % a; + a <<= 6 << a; +} From f7f9930b891da8cd8616bec8b3b63fb8c8406123 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 20 Jul 2016 17:34:58 +0200 Subject: [PATCH 3/3] update lints --- CHANGELOG.md | 1 + README.md | 3 ++- clippy_lints/src/lib.rs | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea228668e..d15b08016 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -207,6 +207,7 @@ All notable changes to this project will be documented in this file. [`match_same_arms`]: https://github.com/Manishearth/rust-clippy/wiki#match_same_arms [`mem_forget`]: https://github.com/Manishearth/rust-clippy/wiki#mem_forget [`min_max`]: https://github.com/Manishearth/rust-clippy/wiki#min_max +[`misrefactored_assign_op`]: https://github.com/Manishearth/rust-clippy/wiki#misrefactored_assign_op [`modulo_one`]: https://github.com/Manishearth/rust-clippy/wiki#modulo_one [`mut_mut`]: https://github.com/Manishearth/rust-clippy/wiki#mut_mut [`mutex_atomic`]: https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic diff --git a/README.md b/README.md index 121f2c24b..ed315a0ca 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 159 lints included in this crate: +There are 160 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -97,6 +97,7 @@ name [match_same_arms](https://github.com/Manishearth/rust-clippy/wiki#match_same_arms) | warn | `match` with identical arm bodies [mem_forget](https://github.com/Manishearth/rust-clippy/wiki#mem_forget) | allow | `mem::forget` usage on `Drop` types is likely to cause memory leaks [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max) | warn | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant +[misrefactored_assign_op](https://github.com/Manishearth/rust-clippy/wiki#misrefactored_assign_op) | warn | having a variable on both sides of an assign op [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one) | warn | taking a number modulo 1, which always returns 0 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut) | allow | usage of double-mut refs, e.g. `&mut &mut ...` [mutex_atomic](https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic) | warn | using a mutex where an atomic value could be used instead diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 53eb07a8d..d68a99a7b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -300,6 +300,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { approx_const::APPROX_CONSTANT, array_indexing::OUT_OF_BOUNDS_INDEXING, assign_ops::ASSIGN_OP_PATTERN, + assign_ops::MISREFACTORED_ASSIGN_OP, attrs::DEPRECATED_SEMVER, attrs::INLINE_ALWAYS, bit_mask::BAD_BIT_MASK,