From: Oliver Schneider Date: Fri, 13 May 2016 14:43:47 +0000 (+0200) Subject: add a companion lint to `no_effect` with suggestions for partially effective statements X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=1e897f1552aa490f821b7ebef949fa6ee873a04f;p=rust.git add a companion lint to `no_effect` with suggestions for partially effective statements --- diff --git a/CHANGELOG.md b/CHANGELOG.md index e442c4d7b1d..6e27b3909d3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 658e2eb63a5..c7a839dc803 100644 --- a/README.md +++ b/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 diff --git a/src/lib.rs b/src/lib.rs index 37464e766b0..f1c815f0df0 100644 --- a/src/lib.rs +++ b/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, diff --git a/src/no_effect.rs b/src/no_effect.rs index d928de41578..593a6c4ad59 100644 --- a/src/no_effect.rs +++ b/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 @@ "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 @@ fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool { impl LintPass for NoEffectPass { fn get_lints(&self) -> LintArray { - lint_array!(NO_EFFECT) + lint_array!(NO_EFFECT, UNNECESSARY_OPERATION) } } @@ -77,7 +91,65 @@ fn check_stmt(&mut self, cx: &LateContext, stmt: &Stmt) { 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> { + 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, } } diff --git a/tests/compile-fail/absurd-extreme-comparisons.rs b/tests/compile-fail/absurd-extreme-comparisons.rs index 7e2ad1fede5..f1e4a692800 100644 --- a/tests/compile-fail/absurd-extreme-comparisons.rs +++ b/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; diff --git a/tests/compile-fail/arithmetic.rs b/tests/compile-fail/arithmetic.rs index 54ac65970ae..5479c55e11e 100644 --- a/tests/compile-fail/arithmetic.rs +++ b/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 diff --git a/tests/compile-fail/array_indexing.rs b/tests/compile-fail/array_indexing.rs index 35fadf8c1e4..dacb72ee8ac 100644 --- a/tests/compile-fail/array_indexing.rs +++ b/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]; diff --git a/tests/compile-fail/bit_masks.rs b/tests/compile-fail/bit_masks.rs index 98135295862..79772840c73 100644 --- a/tests/compile-fail/bit_masks.rs +++ b/tests/compile-fail/bit_masks.rs @@ -5,7 +5,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; diff --git a/tests/compile-fail/cast.rs b/tests/compile-fail/cast.rs index 0f44fa2c1fd..d0ea5f40789 100644 --- a/tests/compile-fail/cast.rs +++ b/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) diff --git a/tests/compile-fail/cmp_nan.rs b/tests/compile-fail/cmp_nan.rs index d2188130a61..8d173665a24 100644 --- a/tests/compile-fail/cmp_nan.rs +++ b/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 diff --git a/tests/compile-fail/cmp_owned.rs b/tests/compile-fail/cmp_owned.rs index eb4070d8fd6..f7c7824e9d1 100644 --- a/tests/compile-fail/cmp_owned.rs +++ b/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(); diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs index 68756a57cc7..bbdd73dc0c8 100644 --- a/tests/compile-fail/copies.rs +++ b/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)] diff --git a/tests/compile-fail/eq_op.rs b/tests/compile-fail/eq_op.rs index 443bbbaacd3..768eadd00eb 100644 --- a/tests/compile-fail/eq_op.rs +++ b/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 diff --git a/tests/compile-fail/float_cmp.rs b/tests/compile-fail/float_cmp.rs index d1ecb37cdd5..85df1ded5ac 100644 --- a/tests/compile-fail/float_cmp.rs +++ b/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; diff --git a/tests/compile-fail/identity_op.rs b/tests/compile-fail/identity_op.rs index 28873ee6b73..329c4a6bbf4 100644 --- a/tests/compile-fail/identity_op.rs +++ b/tests/compile-fail/identity_op.rs @@ -5,7 +5,7 @@ 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; diff --git a/tests/compile-fail/invalid_upcast_comparisons.rs b/tests/compile-fail/invalid_upcast_comparisons.rs index 443dd89aab9..9635f3afede 100644 --- a/tests/compile-fail/invalid_upcast_comparisons.rs +++ b/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; diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 0a943840e17..88a1e7c4cf2 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -341,6 +341,7 @@ struct MyErrorWithParam { x: T } +#[allow(unnecessary_operation)] fn starts_with() { "".chars().next() == Some(' '); //~^ ERROR starts_with diff --git a/tests/compile-fail/modulo_one.rs b/tests/compile-fail/modulo_one.rs index e84209a6d1e..496c1c60d5f 100644 --- a/tests/compile-fail/modulo_one.rs +++ b/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 diff --git a/tests/compile-fail/mut_mut.rs b/tests/compile-fail/mut_mut.rs index 0db9cb3bdef..865574eaec0 100644 --- a/tests/compile-fail/mut_mut.rs +++ b/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; diff --git a/tests/compile-fail/neg_multiply.rs b/tests/compile-fail/neg_multiply.rs index 9deb38920de..90c63c5f263 100644 --- a/tests/compile-fail/neg_multiply.rs +++ b/tests/compile-fail/neg_multiply.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(neg_multiply)] -#![allow(no_effect)] +#![allow(no_effect, unnecessary_operation)] use std::ops::Mul; @@ -10,7 +10,7 @@ impl Mul for X { type Output = X; - + fn mul(self, _r: isize) -> Self { self } @@ -18,7 +18,7 @@ fn mul(self, _r: isize) -> Self { impl Mul 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 } diff --git a/tests/compile-fail/no_effect.rs b/tests/compile-fail/no_effect.rs index 344c82f3307..ce6daa8d562 100644 --- a/tests/compile-fail/no_effect.rs +++ b/tests/compile-fail/no_effect.rs @@ -1,7 +1,7 @@ #![feature(plugin, box_syntax, inclusive_range_syntax)] #![plugin(clippy)] -#![deny(no_effect)] +#![deny(no_effect, unnecessary_operation)] #![allow(dead_code)] #![allow(path_statements)] @@ -50,22 +50,59 @@ fn main() { // Do not warn get_number(); - Tuple(get_number()); - Struct { field: get_number() }; - Struct { ..get_struct() }; - Enum::Tuple(get_number()); - Enum::Struct { field: get_number() }; - 5 + get_number(); - *&get_number(); - &get_number(); - (5, 6, get_number()); - box get_number(); - get_number()..; - ..get_number(); - 5..get_number(); - [42, get_number()]; - [42, 55][get_number() as usize]; - (42, get_number()).1; - [get_number(); 55]; - [42; 55][get_number() as usize]; + + Tuple(get_number()); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + Struct { field: get_number() }; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + Struct { ..get_struct() }; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + Enum::Tuple(get_number()); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + Enum::Struct { field: get_number() }; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + 5 + get_number(); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION 5;get_number(); + *&get_number(); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION &get_number(); + &get_number(); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + (5, 6, get_number()); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION 5;6;get_number(); + box get_number(); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + get_number()..; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + ..get_number(); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + 5..get_number(); //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION 5;get_number(); + [42, get_number()]; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION 42;get_number(); + [42, 55][get_number() as usize]; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION [42, 55];get_number() as usize; + (42, get_number()).1; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION 42;get_number(); + [get_number(); 55]; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION get_number(); + [42; 55][get_number() as usize]; //~ERROR statement can be reduced + //~^HELP replace it with + //~|SUGGESTION [42; 55];get_number() as usize; } diff --git a/tests/compile-fail/unit_cmp.rs b/tests/compile-fail/unit_cmp.rs index 1a28953ace1..13095ee6bfb 100644 --- a/tests/compile-fail/unit_cmp.rs +++ b/tests/compile-fail/unit_cmp.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(unit_cmp)] -#![allow(no_effect)] +#![allow(no_effect, unnecessary_operation)] #[derive(PartialEq)] pub struct ContainsUnit(()); // should be fine