From c6b4b19a435a825e3c28b2851966164b5ee4ca3c Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 11 May 2016 15:32:20 +0200 Subject: [PATCH 1/2] suggest `a op= b` over `a = a op b` --- CHANGELOG.md | 2 + README.md | 4 +- src/assign_ops.rs | 158 +++++++++++++++++++++++++++++++ src/lib.rs | 5 +- src/utils/paths.rs | 1 + tests/compile-fail/assign_ops.rs | 65 +++++++++++++ tests/compile-fail/strings.rs | 2 +- 7 files changed, 234 insertions(+), 3 deletions(-) create mode 100644 src/assign_ops.rs create mode 100644 tests/compile-fail/assign_ops.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fb3c686948..9591e207a88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,8 @@ All notable changes to this project will be documented in this file. [`absurd_extreme_comparisons`]: https://github.com/Manishearth/rust-clippy/wiki#absurd_extreme_comparisons [`almost_swapped`]: https://github.com/Manishearth/rust-clippy/wiki#almost_swapped [`approx_constant`]: https://github.com/Manishearth/rust-clippy/wiki#approx_constant +[`assign_op_pattern`]: https://github.com/Manishearth/rust-clippy/wiki#assign_op_pattern +[`assign_ops`]: https://github.com/Manishearth/rust-clippy/wiki#assign_ops [`bad_bit_mask`]: https://github.com/Manishearth/rust-clippy/wiki#bad_bit_mask [`blacklisted_name`]: https://github.com/Manishearth/rust-clippy/wiki#blacklisted_name [`block_in_if_condition_expr`]: https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr diff --git a/README.md b/README.md index ee234432c63..658e2eb63a5 100644 --- a/README.md +++ b/README.md @@ -17,13 +17,15 @@ Table of contents: ## Lints -There are 147 lints included in this crate: +There are 149 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ [absurd_extreme_comparisons](https://github.com/Manishearth/rust-clippy/wiki#absurd_extreme_comparisons) | warn | a comparison involving a maximum or minimum value involves a case that is always true or always false [almost_swapped](https://github.com/Manishearth/rust-clippy/wiki#almost_swapped) | warn | `foo = bar; bar = foo` sequence [approx_constant](https://github.com/Manishearth/rust-clippy/wiki#approx_constant) | warn | the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found; suggests to use the constant +[assign_op_pattern](https://github.com/Manishearth/rust-clippy/wiki#assign_op_pattern) | warn | assigning the result of an operation on a variable to that same variable +[assign_ops](https://github.com/Manishearth/rust-clippy/wiki#assign_ops) | allow | Any assignment operation [bad_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#bad_bit_mask) | warn | expressions of the form `_ & mask == select` that will only ever return `true` or `false` (because in the example `select` containing bits that `mask` doesn't have) [blacklisted_name](https://github.com/Manishearth/rust-clippy/wiki#blacklisted_name) | warn | usage of a blacklisted/placeholder name [block_in_if_condition_expr](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr) | warn | braces can be eliminated in conditions that are expressions, e.g `if { true } ...` diff --git a/src/assign_ops.rs b/src/assign_ops.rs new file mode 100644 index 00000000000..8dc072e69f0 --- /dev/null +++ b/src/assign_ops.rs @@ -0,0 +1,158 @@ +use rustc::hir; +use rustc::lint::*; +use utils::{span_lint_and_then, span_lint, snippet_opt, SpanlessEq, get_trait_def_id, implements_trait}; + +/// **What it does:** This lint checks for `+=` operations and similar +/// +/// **Why is this bad?** Projects with many developers from languages without those operations +/// may find them unreadable and not worth their weight +/// +/// **Known problems:** None +/// +/// **Example:** +/// ``` +/// a += 1; +/// ``` +declare_restriction_lint! { + pub ASSIGN_OPS, + "Any assignment operation" +} + +/// **What it does:** Check for `a = a op b` or `a = b commutative_op a` patterns +/// +/// **Why is this bad?** These can be written as the shorter `a op= b` +/// +/// **Known problems:** Hopefully none. +/// +/// **Example:** +/// +/// ``` +/// let mut a = 5; +/// ... +/// a = a + b; +/// ``` +declare_lint! { + pub ASSIGN_OP_PATTERN, + Warn, + "assigning the result of an operation on a variable to that same variable" +} + +#[derive(Copy, Clone, Default)] +pub struct AssignOps; + +impl LintPass for AssignOps { + fn get_lints(&self) -> LintArray { + lint_array!(ASSIGN_OPS, ASSIGN_OP_PATTERN) + } +} + +impl LateLintPass for AssignOps { + fn check_expr(&mut self, cx: &LateContext, expr: &hir::Expr) { + match expr.node { + hir::ExprAssignOp(op, ref lhs, ref rhs) => { + if let (Some(l), Some(r)) = (snippet_opt(cx, lhs.span), snippet_opt(cx, rhs.span)) { + span_lint_and_then(cx, + ASSIGN_OPS, + expr.span, + "assign operation detected", + |db| { + match rhs.node { + hir::ExprBinary(op2, _, _) if op2 != op => { + db.span_suggestion(expr.span, + "replace it with", + format!("{} = {} {} ({})", l, l, op.node.as_str(), r)); + }, + _ => { + db.span_suggestion(expr.span, + "replace it with", + format!("{} = {} {} {}", l, l, op.node.as_str(), r)); + } + } + }); + } else { + span_lint(cx, + ASSIGN_OPS, + expr.span, + "assign operation detected"); + } + }, + hir::ExprAssign(ref assignee, ref e) => { + if let hir::ExprBinary(op, ref l, ref r) = e.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 + } + macro_rules! ops { + ($op:expr, $cx:expr, $ty:expr, $rty:expr, $($trait_name:ident:$full_trait_name:ident),+) => { + match $op { + $(hir::$full_trait_name => { + let [krate, module] = ::utils::paths::OPS_MODULE; + let path = [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 + }; + implements_trait($cx, $ty, trait_id, vec![$rty]) + },)* + _ => false, + } + } + } + if ops!(op.node, cx, ty, rty, Add:BiAdd, + Sub:BiSub, + Mul:BiMul, + Div:BiDiv, + Rem:BiRem, + And:BiAnd, + Or:BiOr, + BitAnd:BiBitAnd, + BitOr:BiBitOr, + 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| { + 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 + if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, l) { + lint(assignee, r); + } + // a = b commutative_op a + if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, r) { + match op.node { + hir::BiAdd | hir::BiMul | + hir::BiAnd | hir::BiOr | + hir::BiBitXor | hir::BiBitAnd | hir::BiBitOr => { + lint(assignee, l); + }, + _ => {}, + } + } + } + }, + _ => {}, + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 5dd473c8d09..aac2718b1ec 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,7 +7,6 @@ #![feature(question_mark)] #![feature(stmt_expr_attributes)] #![allow(indexing_slicing, shadow_reuse, unknown_lints)] -#![allow(float_arithmetic, integer_arithmetic)] extern crate rustc_driver; extern crate getopts; @@ -192,6 +191,7 @@ macro_rules! declare_restriction_lint { pub mod approx_const; pub mod arithmetic; pub mod array_indexing; +pub mod assign_ops; pub mod attrs; pub mod bit_mask; pub mod blacklisted_name; @@ -383,10 +383,12 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box unsafe_removed_from_name::UnsafeNameRemoval); reg.register_late_lint_pass(box mem_forget::MemForget); reg.register_late_lint_pass(box arithmetic::Arithmetic::default()); + reg.register_late_lint_pass(box assign_ops::AssignOps); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, arithmetic::INTEGER_ARITHMETIC, + assign_ops::ASSIGN_OPS, ]); reg.register_lint_group("clippy_pedantic", vec![ @@ -421,6 +423,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_group("clippy", vec![ approx_const::APPROX_CONSTANT, array_indexing::OUT_OF_BOUNDS_INDEXING, + assign_ops::ASSIGN_OP_PATTERN, attrs::DEPRECATED_SEMVER, attrs::INLINE_ALWAYS, bit_mask::BAD_BIT_MASK, diff --git a/src/utils/paths.rs b/src/utils/paths.rs index c52324a7448..2e6ceb50096 100644 --- a/src/utils/paths.rs +++ b/src/utils/paths.rs @@ -29,6 +29,7 @@ pub const MEM_FORGET: [&'static str; 3] = ["core", "mem", "forget"]; pub const MUTEX: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"]; pub const OPEN_OPTIONS: [&'static str; 3] = ["std", "fs", "OpenOptions"]; +pub const OPS_MODULE: [&'static str; 2] = ["core", "ops"]; pub const OPTION: [&'static str; 3] = ["core", "option", "Option"]; pub const RANGE: [&'static str; 3] = ["core", "ops", "Range"]; pub const RANGE_FROM: [&'static str; 3] = ["core", "ops", "RangeFrom"]; diff --git a/tests/compile-fail/assign_ops.rs b/tests/compile-fail/assign_ops.rs new file mode 100644 index 00000000000..84d868ecfcc --- /dev/null +++ b/tests/compile-fail/assign_ops.rs @@ -0,0 +1,65 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[deny(assign_ops)] +#[allow(unused_assignments)] +fn main() { + let mut i = 1i32; + i += 2; //~ ERROR assign operation detected + //~^ HELP replace it with + //~| SUGGESTION i = i + 2 + i -= 6; //~ ERROR assign operation detected + //~^ HELP replace it with + //~| SUGGESTION i = i - 6 + i *= 5; //~ ERROR assign operation detected + //~^ HELP replace it with + //~| SUGGESTION i = i * 5 + i /= 32; //~ ERROR assign operation detected + //~^ HELP replace it with + //~| SUGGESTION i = i / 32 + i %= 42; //~ ERROR assign operation detected + //~^ HELP replace it with + //~| SUGGESTION i = i % 42 + i >>= i; //~ ERROR assign operation detected + //~^ HELP replace it with + //~| SUGGESTION i = i >> i + i <<= 9 + 6 - 7; //~ ERROR assign operation detected + //~^ HELP replace it with + //~| SUGGESTION i = i << (9 + 6 - 7) +} + +#[allow(dead_code, unused_assignments)] +#[deny(assign_op_pattern)] +fn bla() { + let mut a = 5; + a = a + 1; //~ ERROR manual implementation of an assign operation + //~^ HELP replace it with + //~| SUGGESTION a += 1 + a = 1 + a; //~ ERROR manual implementation of an assign operation + //~^ HELP replace it with + //~| SUGGESTION a += 1 + a = a - 1; //~ ERROR manual implementation of an assign operation + //~^ HELP replace it with + //~| SUGGESTION a -= 1 + a = a * 99; //~ ERROR manual implementation of an assign operation + //~^ HELP replace it with + //~| SUGGESTION a *= 99 + a = 42 * a; //~ ERROR manual implementation of an assign operation + //~^ HELP replace it with + //~| SUGGESTION a *= 42 + a = a / 2; //~ ERROR manual implementation of an assign operation + //~^ HELP replace it with + //~| SUGGESTION a /= 2 + a = a % 5; //~ ERROR manual implementation of an assign operation + //~^ HELP replace it with + //~| SUGGESTION a %= 5 + a = a & 1; //~ ERROR manual implementation of an assign operation + //~^ HELP replace it with + //~| SUGGESTION a &= 1 + a = 1 - a; + a = 5 / a; + a = 42 % a; + a = 6 << a; + let mut s = String::new(); + s = s + "bla"; +} diff --git a/tests/compile-fail/strings.rs b/tests/compile-fail/strings.rs index 656349ba621..542b6db4abb 100644 --- a/tests/compile-fail/strings.rs +++ b/tests/compile-fail/strings.rs @@ -65,6 +65,6 @@ fn main() { // the add is only caught for String let mut x = 1; - x = x + 1; + x = x + 1; //~ WARN assign_op_pattern assert_eq!(2, x); } From b0d008bc9d0e2efd92c29da9c40db768b864b52b Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 11 May 2016 17:04:27 +0200 Subject: [PATCH 2/2] add known problems --- src/assign_ops.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/assign_ops.rs b/src/assign_ops.rs index 8dc072e69f0..2b1aec83e4c 100644 --- a/src/assign_ops.rs +++ b/src/assign_ops.rs @@ -7,7 +7,7 @@ /// **Why is this bad?** Projects with many developers from languages without those operations /// may find them unreadable and not worth their weight /// -/// **Known problems:** None +/// **Known problems:** Types implementing `OpAssign` don't necessarily implement `Op` /// /// **Example:** /// ``` @@ -22,7 +22,7 @@ /// /// **Why is this bad?** These can be written as the shorter `a op= b` /// -/// **Known problems:** Hopefully none. +/// **Known problems:** While forbidden by the spec, `OpAssign` traits may have implementations that differ from the regular `Op` impl /// /// **Example:** ///