Merge pull request #928 from oli-obk/unnecessary_operation

add a companion lint to `no_effect` with suggestions for partially (in-)effective statements

Author: mcarton

CHANGELOG.md

@@ -215,6 +215,7 @@ All notable changes to this project will be documented in this file.
 [`unicode_not_nfc`]: https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc
 [`unit_cmp`]: https://github.com/Manishearth/rust-clippy/wiki#unit_cmp
 [`unnecessary_mut_passed`]: https://github.com/Manishearth/rust-clippy/wiki#unnecessary_mut_passed
+[`unnecessary_operation`]: https://github.com/Manishearth/rust-clippy/wiki#unnecessary_operation
 [`unneeded_field_pattern`]: https://github.com/Manishearth/rust-clippy/wiki#unneeded_field_pattern
 [`unsafe_removed_from_name`]: https://github.com/Manishearth/rust-clippy/wiki#unsafe_removed_from_name
 [`unstable_as_mut_slice`]: https://github.com/Manishearth/rust-clippy/wiki#unstable_as_mut_slice

README.md

@@ -17,7 +17,7 @@ Table of contents:
 
 ## Lints
 
-There are 149 lints included in this crate:
+There are 150 lints included in this crate:
 
 name                                                                                                                 | default | meaning
 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -154,6 +154,7 @@ name
 [unicode_not_nfc](https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc)                                   | allow   | using a unicode literal not in NFC normal form (see [unicode tr15](http://www.unicode.org/reports/tr15/) for further information)
 [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp)                                                 | warn    | comparing unit values (which is always `true` or `false`, respectively)
 [unnecessary_mut_passed](https://github.com/Manishearth/rust-clippy/wiki#unnecessary_mut_passed)                     | warn    | an argument is passed as a mutable reference although the function/method only demands an immutable reference
+[unnecessary_operation](https://github.com/Manishearth/rust-clippy/wiki#unnecessary_operation)                       | warn    | outer expressions with no effect
 [unneeded_field_pattern](https://github.com/Manishearth/rust-clippy/wiki#unneeded_field_pattern)                     | warn    | Struct fields are bound to a wildcard instead of using `..`
 [unsafe_removed_from_name](https://github.com/Manishearth/rust-clippy/wiki#unsafe_removed_from_name)                 | warn    | unsafe removed from name
 [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect)                                     | warn    | `collect()`ing an iterator without using the result; this is usually better written as a for loop

src/lib.rs

@@ -519,6 +519,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         neg_multiply::NEG_MULTIPLY,
         new_without_default::NEW_WITHOUT_DEFAULT,
         no_effect::NO_EFFECT,
+        no_effect::UNNECESSARY_OPERATION,
         non_expressive_names::MANY_SINGLE_CHAR_NAMES,
         open_options::NONSENSICAL_OPEN_OPTIONS,
         overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,

src/no_effect.rs

@@ -1,7 +1,8 @@
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
-use rustc::hir::def::Def;
+use rustc::hir::def::{Def, PathResolution};
 use rustc::hir::{Expr, Expr_, Stmt, StmtSemi};
-use utils::{in_macro, span_lint};
+use utils::{in_macro, span_lint, snippet_opt, span_lint_and_then};
+use std::ops::Deref;
 
 /// **What it does:** This lint checks for statements which have no effect.
 ///
@@ -16,6 +17,19 @@ declare_lint! {
     "statements with no effect"
 }
 
+/// **What it does:** This lint checks for expression statements that can be reduced to a sub-expression
+///
+/// **Why is this bad?** Expressions by themselves often have no side-effects. Having such expressions reduces redability.
+///
+/// **Known problems:** None.
+///
+/// **Example:** `compute_array()[0];`
+declare_lint! {
+    pub UNNECESSARY_OPERATION,
+    Warn,
+    "outer expressions with no effect"
+}
+
 fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool {
     if in_macro(cx, expr.span) {
         return false;
@@ -68,7 +82,7 @@ pub struct NoEffectPass;
 
 impl LintPass for NoEffectPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(NO_EFFECT)
+        lint_array!(NO_EFFECT, UNNECESSARY_OPERATION)
     }
 }
 
@@ -77,7 +91,65 @@ impl LateLintPass for NoEffectPass {
         if let StmtSemi(ref expr, _) = stmt.node {
             if has_no_effect(cx, expr) {
                 span_lint(cx, NO_EFFECT, stmt.span, "statement with no effect");
+            } else if let Some(reduced) = reduce_expression(cx, expr) {
+                let mut snippet = String::new();
+                for e in reduced {
+                    if in_macro(cx, e.span) {
+                        return;
+                    }
+                    if let Some(snip) = snippet_opt(cx, e.span) {
+                        snippet.push_str(&snip);
+                        snippet.push(';');
+                    } else {
+                        return;
+                    }
+                }
+                span_lint_and_then(cx, UNNECESSARY_OPERATION, stmt.span, "statement can be reduced", |db| {
+                    db.span_suggestion(stmt.span, "replace it with", snippet);
+                });
+            }
+        }
+    }
+}
+
+
+fn reduce_expression<'a>(cx: &LateContext, expr: &'a Expr) -> Option<Vec<&'a Expr>> {
+    if in_macro(cx, expr.span) {
+        return None;
+    }
+    match expr.node {
+        Expr_::ExprIndex(ref a, ref b) |
+        Expr_::ExprBinary(_, ref a, ref b) => Some(vec![&**a, &**b]),
+        Expr_::ExprVec(ref v) |
+        Expr_::ExprTup(ref v) => Some(v.iter().map(Deref::deref).collect()),
+        Expr_::ExprRepeat(ref inner, _) |
+        Expr_::ExprCast(ref inner, _) |
+        Expr_::ExprType(ref inner, _) |
+        Expr_::ExprUnary(_, ref inner) |
+        Expr_::ExprField(ref inner, _) |
+        Expr_::ExprTupField(ref inner, _) |
+        Expr_::ExprAddrOf(_, ref inner) |
+        Expr_::ExprBox(ref inner) => reduce_expression(cx, inner).or_else(|| Some(vec![inner])),
+        Expr_::ExprStruct(_, ref fields, ref base) => Some(fields.iter().map(|f| &f.expr).chain(base).map(Deref::deref).collect()),
+        Expr_::ExprCall(ref callee, ref args) => {
+            match cx.tcx.def_map.borrow().get(&callee.id).map(PathResolution::full_def) {
+                Some(Def::Struct(..)) |
+                Some(Def::Variant(..)) => Some(args.iter().map(Deref::deref).collect()),
+                _ => None,
+            }
+        }
+        Expr_::ExprBlock(ref block) => {
+            if block.stmts.is_empty() {
+                block.expr.as_ref().and_then(|e| if e.span == expr.span {
+                    // in case of compiler-inserted signaling blocks
+                    reduce_expression(cx, e)
+                } else {
+                    Some(vec![e])
+                })
+            } else {
+                None
             }
         }
+        _ => None,
     }
 }

tests/compile-fail/absurd-extreme-comparisons.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(absurd_extreme_comparisons)]
-#![allow(unused, eq_op, no_effect)]
+#![allow(unused, eq_op, no_effect, unnecessary_operation)]
 fn main() {
     const Z: u32 = 0;
 

tests/compile-fail/arithmetic.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(integer_arithmetic, float_arithmetic)]
-#![allow(unused, shadow_reuse, shadow_unrelated, no_effect)]
+#![allow(unused, shadow_reuse, shadow_unrelated, no_effect, unnecessary_operation)]
 fn main() {
     let i = 1i32;
     1 + i; //~ERROR integer arithmetic detected
@@ -11,17 +11,17 @@ fn main() {
     i / 2; // no error, this is part of the expression in the preceding line
     i - 2 + 2 - i; //~ERROR integer arithmetic detected
     -i; //~ERROR integer arithmetic detected
-    
+
     i & 1; // no wrapping
-    i | 1; 
+    i | 1;
     i ^ 1;
     i >> 1;
     i << 1;
-    
+
     let f = 1.0f32;
-    
+
     f * 2.0; //~ERROR floating-point arithmetic detected
-    
+
     1.0 + f; //~ERROR floating-point arithmetic detected
     f * 2.0; //~ERROR floating-point arithmetic detected
     f / 2.0; //~ERROR floating-point arithmetic detected

tests/compile-fail/array_indexing.rs

@@ -3,7 +3,7 @@
 
 #![deny(indexing_slicing)]
 #![deny(out_of_bounds_indexing)]
-#![allow(no_effect)]
+#![allow(no_effect, unnecessary_operation)]
 
 fn main() {
     let x = [1,2,3,4];

tests/compile-fail/bit_masks.rs

@@ -5,7 +5,7 @@ const THREE_BITS : i64 = 7;
 const EVEN_MORE_REDIRECTION : i64 = THREE_BITS;
 
 #[deny(bad_bit_mask)]
-#[allow(ineffective_bit_mask, identity_op, no_effect)]
+#[allow(ineffective_bit_mask, identity_op, no_effect, unnecessary_operation)]
 fn main() {
     let x = 5;
 
@@ -45,7 +45,7 @@ fn main() {
 }
 
 #[deny(ineffective_bit_mask)]
-#[allow(bad_bit_mask, no_effect)]
+#[allow(bad_bit_mask, no_effect, unnecessary_operation)]
 fn ineffective() {
     let x = 5;
 

tests/compile-fail/cast.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #[deny(cast_precision_loss, cast_possible_truncation, cast_sign_loss, cast_possible_wrap)]
-#[allow(no_effect)]
+#[allow(no_effect, unnecessary_operation)]
 fn main() {
     // Test cast_precision_loss
     1i32 as f32; //~ERROR casting i32 to f32 causes a loss of precision (i32 is 32 bits wide, but f32's mantissa is only 23 bits wide)

tests/compile-fail/cmp_nan.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #[deny(cmp_nan)]
-#[allow(float_cmp, no_effect)]
+#[allow(float_cmp, no_effect, unnecessary_operation)]
 fn main() {
     let x = 5f32;
     x == std::f32::NAN; //~ERROR doomed comparison with NAN

tests/compile-fail/cmp_owned.rs

@@ -2,6 +2,7 @@
 #![plugin(clippy)]
 
 #[deny(cmp_owned)]
+#[allow(unnecessary_operation)]
 fn main() {
     fn with_to_string(x : &str) {
         x != "foo".to_string();

tests/compile-fail/copies.rs

@@ -1,7 +1,7 @@
 #![feature(plugin, inclusive_range_syntax)]
 #![plugin(clippy)]
 
-#![allow(dead_code, no_effect)]
+#![allow(dead_code, no_effect, unnecessary_operation)]
 #![allow(let_and_return)]
 #![allow(needless_return)]
 #![allow(unused_variables)]

tests/compile-fail/eq_op.rs

@@ -3,7 +3,7 @@
 
 #[deny(eq_op)]
 #[allow(identity_op)]
-#[allow(no_effect, unused_variables)]
+#[allow(no_effect, unused_variables, unnecessary_operation)]
 #[deny(nonminimal_bool)]
 fn main() {
     // simple values and comparisons

tests/compile-fail/float_cmp.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(float_cmp)]
-#![allow(unused, no_effect)]
+#![allow(unused, no_effect, unnecessary_operation)]
 
 use std::ops::Add;
 

tests/compile-fail/identity_op.rs

@@ -5,7 +5,7 @@ const ONE : i64 = 1;
 const NEG_ONE : i64 = -1;
 const ZERO : i64 = 0;
 
-#[allow(eq_op, no_effect)]
+#[allow(eq_op, no_effect, unnecessary_operation)]
 #[deny(identity_op)]
 fn main() {
     let x = 0;

tests/compile-fail/invalid_upcast_comparisons.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(invalid_upcast_comparisons)]
-#![allow(unused, eq_op, no_effect)]
+#![allow(unused, eq_op, no_effect, unnecessary_operation)]
 fn main() {
     let zero: u32 = 0;
     let u8_max: u8 = 255;

tests/compile-fail/methods.rs

@@ -341,6 +341,7 @@ struct MyErrorWithParam<T> {
     x: T
 }
 
+#[allow(unnecessary_operation)]
 fn starts_with() {
     "".chars().next() == Some(' ');
     //~^ ERROR starts_with

tests/compile-fail/modulo_one.rs

@@ -1,7 +1,7 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 #![deny(modulo_one)]
-#![allow(no_effect)]
+#![allow(no_effect, unnecessary_operation)]
 
 fn main() {
     10 % 1; //~ERROR any number modulo 1 will be 0

tests/compile-fail/mut_mut.rs

@@ -1,7 +1,7 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 
-#![allow(unused, no_effect)]
+#![allow(unused, no_effect, unnecessary_operation)]
 
 //#![plugin(regex_macros)]
 //extern crate regex;

tests/compile-fail/neg_multiply.rs

@@ -2,23 +2,23 @@
 
 #![plugin(clippy)]
 #![deny(neg_multiply)]
-#![allow(no_effect)]
+#![allow(no_effect, unnecessary_operation)]
 
 use std::ops::Mul;
 
 struct X;
 
 impl Mul<isize> for X {
     type Output = X;
-    
+
     fn mul(self, _r: isize) -> Self {
         self
     }
 }
 
 impl Mul<X> for isize {
     type Output = X;
-    
+
     fn mul(self, _r: X) -> X {
         X
     }
@@ -34,7 +34,7 @@ fn main() {
     //~^ ERROR Negation by multiplying with -1
 
     -1 * -1; // should be ok
-    
+
     X * -1; // should be ok
     -1 * X; // should also be ok
 }