From bbb8cd4fbbed4b82b7d1b61206bf6c70fd75e665 Mon Sep 17 00:00:00 2001 From: Igor Aleksanov Date: Thu, 14 Nov 2019 08:06:34 +0300 Subject: [PATCH] Implement if_same_cond_fn lint Run ./util/dev Revert changelog entry Rename lint to same_functions_in_if_condition and add a doc example Add testcases with different arg in fn invocation --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/copies.rs | 78 +++++++++++++++++- clippy_lints/src/lib.rs | 2 + src/lintlist/mod.rs | 9 ++- tests/ui/same_functions_in_if_condition.rs | 80 +++++++++++++++++++ .../ui/same_functions_in_if_condition.stderr | 75 +++++++++++++++++ 7 files changed, 244 insertions(+), 3 deletions(-) create mode 100644 tests/ui/same_functions_in_if_condition.rs create mode 100644 tests/ui/same_functions_in_if_condition.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 30d3108e0be..a10706c3fc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1030,6 +1030,7 @@ Released 2018-09-13 [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond +[`ifs_same_cond_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond_fn [`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher [`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping diff --git a/README.md b/README.md index 922dbcd1138..c5106e074cb 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 333 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 334 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index f514068fb10..9cbff066f46 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -40,6 +40,53 @@ declare_clippy_lint! { "consecutive `ifs` with the same condition" } +declare_clippy_lint! { + /// **What it does:** Checks for consecutive `if`s with the same function call. + /// + /// **Why is this bad?** This is probably a copy & paste error. + /// Despite the fact that function can have side effects and `if` works as + /// intended, such an approach is implicit and can be considered a "code smell". + /// + /// **Known problems:** Hopefully none. + /// + /// **Example:** + /// ```ignore + /// if foo() == bar { + /// … + /// } else if foo() == bar { + /// … + /// } + /// ``` + /// + /// This probably should be: + /// ```ignore + /// if foo() == bar { + /// … + /// } else if foo() == baz { + /// … + /// } + /// ``` + /// + /// or if the original code was not a typo and called function mutates a state, + /// consider move the mutation out of the `if` condition to avoid similarity to + /// a copy & paste error: + /// + /// ```ignore + /// let first = foo(); + /// if first == bar { + /// … + /// } else { + /// let second = foo(); + /// if second == bar { + /// … + /// } + /// } + /// ``` + pub SAME_FUNCTIONS_IN_IF_CONDITION, + pedantic, + "consecutive `ifs` with the same function call" +} + declare_clippy_lint! { /// **What it does:** Checks for `if/else` with the same body as the *then* part /// and the *else* part. @@ -102,7 +149,7 @@ declare_clippy_lint! { "`match` with identical arm bodies" } -declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]); +declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CopyAndPaste { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { @@ -119,6 +166,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CopyAndPaste { let (conds, blocks) = if_sequence(expr); lint_same_then_else(cx, &blocks); lint_same_cond(cx, &conds); + lint_same_fns_in_if_cond(cx, &conds); lint_match_arms(cx, expr); } } @@ -163,6 +211,34 @@ fn lint_same_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) { } } +/// Implementation of `SAME_FUNCTIONS_IN_IF_CONDITION`. +fn lint_same_fns_in_if_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) { + let hash: &dyn Fn(&&Expr) -> u64 = &|expr| -> u64 { + let mut h = SpanlessHash::new(cx, cx.tables); + h.hash_expr(expr); + h.finish() + }; + + let eq: &dyn Fn(&&Expr, &&Expr) -> bool = &|&lhs, &rhs| -> bool { + // Do not spawn warning if `IFS_SAME_COND` already produced it. + if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs) { + return false; + } + SpanlessEq::new(cx).eq_expr(lhs, rhs) + }; + + for (i, j) in search_same(conds, hash, eq) { + span_note_and_lint( + cx, + SAME_FUNCTIONS_IN_IF_CONDITION, + j.span, + "this `if` has the same function call as a previous if", + i.span, + "same as this", + ); + } +} + /// Implementation of `MATCH_SAME_ARMS`. fn lint_match_arms<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &Expr) { fn same_bindings<'tcx>( diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c22693aee3b..07feb25e735 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -471,6 +471,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf &collapsible_if::COLLAPSIBLE_IF, &comparison_chain::COMPARISON_CHAIN, &copies::IFS_SAME_COND, + &copies::SAME_FUNCTIONS_IN_IF_CONDITION, &copies::IF_SAME_THEN_ELSE, &copies::MATCH_SAME_ARMS, ©_iterator::COPY_ITERATOR, @@ -989,6 +990,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(&attrs::INLINE_ALWAYS), LintId::of(&checked_conversions::CHECKED_CONVERSIONS), + LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION), LintId::of(&copies::MATCH_SAME_ARMS), LintId::of(©_iterator::COPY_ITERATOR), LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 0b301b6be96..b033da45ecb 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 333] = [ +pub const ALL_LINTS: [Lint; 334] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -714,6 +714,13 @@ pub const ALL_LINTS: [Lint; 333] = [ deprecation: None, module: "copies", }, + Lint { + name: "ifs_same_cond_fn", + group: "pedantic", + desc: "consecutive `ifs` with the same function call", + deprecation: None, + module: "copies", + }, Lint { name: "implicit_hasher", group: "style", diff --git a/tests/ui/same_functions_in_if_condition.rs b/tests/ui/same_functions_in_if_condition.rs new file mode 100644 index 00000000000..686867cf5c6 --- /dev/null +++ b/tests/ui/same_functions_in_if_condition.rs @@ -0,0 +1,80 @@ +#![warn(clippy::same_functions_in_if_condition)] +#![allow(clippy::ifs_same_cond)] // This warning is different from `ifs_same_cond`. +#![allow(clippy::if_same_then_else, clippy::comparison_chain)] // all empty blocks + +fn function() -> bool { + true +} + +fn fn_arg(_arg: u8) -> bool { + true +} + +struct Struct; + +impl Struct { + fn method(&self) -> bool { + true + } + fn method_arg(&self, _arg: u8) -> bool { + true + } +} + +fn ifs_same_cond_fn() { + let a = 0; + let obj = Struct; + + if function() { + } else if function() { + //~ ERROR ifs same condition + } + + if fn_arg(a) { + } else if fn_arg(a) { + //~ ERROR ifs same condition + } + + if obj.method() { + } else if obj.method() { + //~ ERROR ifs same condition + } + + if obj.method_arg(a) { + } else if obj.method_arg(a) { + //~ ERROR ifs same condition + } + + let mut v = vec![1]; + if v.pop() == None { + //~ ERROR ifs same condition + } else if v.pop() == None { + } + + if v.len() == 42 { + //~ ERROR ifs same condition + } else if v.len() == 42 { + } + + if v.len() == 1 { + // ok, different conditions + } else if v.len() == 2 { + } + + if fn_arg(0) { + // ok, different arguments. + } else if fn_arg(1) { + } + + if obj.method_arg(0) { + // ok, different arguments. + } else if obj.method_arg(1) { + } + + if a == 1 { + // ok, warning is on `ifs_same_cond` behalf. + } else if a == 1 { + } +} + +fn main() {} diff --git a/tests/ui/same_functions_in_if_condition.stderr b/tests/ui/same_functions_in_if_condition.stderr new file mode 100644 index 00000000000..214f1a9e7c8 --- /dev/null +++ b/tests/ui/same_functions_in_if_condition.stderr @@ -0,0 +1,75 @@ +error: this `if` has the same function call as a previous if + --> $DIR/same_functions_in_if_condition.rs:29:15 + | +LL | } else if function() { + | ^^^^^^^^^^ + | + = note: `-D clippy::same-functions-in-if-condition` implied by `-D warnings` +note: same as this + --> $DIR/same_functions_in_if_condition.rs:28:8 + | +LL | if function() { + | ^^^^^^^^^^ + +error: this `if` has the same function call as a previous if + --> $DIR/same_functions_in_if_condition.rs:34:15 + | +LL | } else if fn_arg(a) { + | ^^^^^^^^^ + | +note: same as this + --> $DIR/same_functions_in_if_condition.rs:33:8 + | +LL | if fn_arg(a) { + | ^^^^^^^^^ + +error: this `if` has the same function call as a previous if + --> $DIR/same_functions_in_if_condition.rs:39:15 + | +LL | } else if obj.method() { + | ^^^^^^^^^^^^ + | +note: same as this + --> $DIR/same_functions_in_if_condition.rs:38:8 + | +LL | if obj.method() { + | ^^^^^^^^^^^^ + +error: this `if` has the same function call as a previous if + --> $DIR/same_functions_in_if_condition.rs:44:15 + | +LL | } else if obj.method_arg(a) { + | ^^^^^^^^^^^^^^^^^ + | +note: same as this + --> $DIR/same_functions_in_if_condition.rs:43:8 + | +LL | if obj.method_arg(a) { + | ^^^^^^^^^^^^^^^^^ + +error: this `if` has the same function call as a previous if + --> $DIR/same_functions_in_if_condition.rs:51:15 + | +LL | } else if v.pop() == None { + | ^^^^^^^^^^^^^^^ + | +note: same as this + --> $DIR/same_functions_in_if_condition.rs:49:8 + | +LL | if v.pop() == None { + | ^^^^^^^^^^^^^^^ + +error: this `if` has the same function call as a previous if + --> $DIR/same_functions_in_if_condition.rs:56:15 + | +LL | } else if v.len() == 42 { + | ^^^^^^^^^^^^^ + | +note: same as this + --> $DIR/same_functions_in_if_condition.rs:54:8 + | +LL | if v.len() == 42 { + | ^^^^^^^^^^^^^ + +error: aborting due to 6 previous errors +