Skip to content

Commit c473a3e

Browse files
popzxcpopzxc
committed
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
1 parent f54a557 commit c473a3e

File tree

7 files changed

+244
-3
lines changed

7 files changed

+244
-3
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1030,6 +1030,7 @@ Released 2018-09-13
10301030
[`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else
10311031
[`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
10321032
[`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
1033+
[`ifs_same_cond_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond_fn
10331034
[`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher
10341035
[`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return
10351036
[`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
88

9-
[There are 333 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
9+
[There are 334 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
1010

1111
We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
1212

‎clippy_lints/src/copies.rs‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,53 @@ declare_clippy_lint! {
4040
"consecutive `ifs` with the same condition"
4141
}
4242

43+
declare_clippy_lint! {
44+
/// **What it does:** Checks for consecutive `if`s with the same function call.
45+
///
46+
/// **Why is this bad?** This is probably a copy & paste error.
47+
/// Despite the fact that function can have side effects and `if` works as
48+
/// intended, such an approach is implicit and can be considered a "code smell".
49+
///
50+
/// **Known problems:** Hopefully none.
51+
///
52+
/// **Example:**
53+
/// ```ignore
54+
/// if foo() == bar {
55+
/// …
56+
/// } else if foo() == bar {
57+
/// …
58+
/// }
59+
/// ```
60+
///
61+
/// This probably should be:
62+
/// ```ignore
63+
/// if foo() == bar {
64+
/// …
65+
/// } else if foo() == baz {
66+
/// …
67+
/// }
68+
/// ```
69+
///
70+
/// or if the original code was not a typo and called function mutates a state,
71+
/// consider move the mutation out of the `if` condition to avoid similarity to
72+
/// a copy & paste error:
73+
///
74+
/// ```ignore
75+
/// let first = foo();
76+
/// if first == bar {
77+
/// …
78+
/// } else {
79+
/// let second = foo();
80+
/// if second == bar {
81+
/// …
82+
/// }
83+
/// }
84+
/// ```
85+
pub SAME_FUNCTIONS_IN_IF_CONDITION,
86+
pedantic,
87+
"consecutive `ifs` with the same function call"
88+
}
89+
4390
declare_clippy_lint! {
4491
/// **What it does:** Checks for `if/else` with the same body as the *then* part
4592
/// and the *else* part.
@@ -102,7 +149,7 @@ declare_clippy_lint! {
102149
"`match` with identical arm bodies"
103150
}
104151

105-
declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]);
152+
declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]);
106153

107154
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CopyAndPaste {
108155
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
@@ -119,6 +166,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CopyAndPaste {
119166
let (conds, blocks) = if_sequence(expr);
120167
lint_same_then_else(cx, &blocks);
121168
lint_same_cond(cx, &conds);
169+
lint_same_fns_in_if_cond(cx, &conds);
122170
lint_match_arms(cx, expr);
123171
}
124172
}
@@ -163,6 +211,34 @@ fn lint_same_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) {
163211
}
164212
}
165213

214+
/// Implementation of `SAME_FUNCTIONS_IN_IF_CONDITION`.
215+
fn lint_same_fns_in_if_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) {
216+
let hash: &dyn Fn(&&Expr) -> u64 = &|expr| -> u64 {
217+
let mut h = SpanlessHash::new(cx, cx.tables);
218+
h.hash_expr(expr);
219+
h.finish()
220+
};
221+
222+
let eq: &dyn Fn(&&Expr, &&Expr) -> bool = &|&lhs, &rhs| -> bool {
223+
// Do not spawn warning if `IFS_SAME_COND` already produced it.
224+
if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs) {
225+
return false;
226+
}
227+
SpanlessEq::new(cx).eq_expr(lhs, rhs)
228+
};
229+
230+
for (i, j) in search_same(conds, hash, eq) {
231+
span_note_and_lint(
232+
cx,
233+
SAME_FUNCTIONS_IN_IF_CONDITION,
234+
j.span,
235+
"this `if` has the same function call as a previous if",
236+
i.span,
237+
"same as this",
238+
);
239+
}
240+
}
241+
166242
/// Implementation of `MATCH_SAME_ARMS`.
167243
fn lint_match_arms<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &Expr) {
168244
fn same_bindings<'tcx>(

‎clippy_lints/src/lib.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -471,6 +471,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
471471
&collapsible_if::COLLAPSIBLE_IF,
472472
&comparison_chain::COMPARISON_CHAIN,
473473
&copies::IFS_SAME_COND,
474+
&copies::SAME_FUNCTIONS_IN_IF_CONDITION,
474475
&copies::IF_SAME_THEN_ELSE,
475476
&copies::MATCH_SAME_ARMS,
476477
&copy_iterator::COPY_ITERATOR,
@@ -989,6 +990,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
989990
store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
990991
LintId::of(&attrs::INLINE_ALWAYS),
991992
LintId::of(&checked_conversions::CHECKED_CONVERSIONS),
993+
LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION),
992994
LintId::of(&copies::MATCH_SAME_ARMS),
993995
LintId::of(&copy_iterator::COPY_ITERATOR),
994996
LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS),

‎src/lintlist/mod.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ pub use lint::Lint;
66
pub use lint::LINT_LEVELS;
77

88
// begin lint list, do not remove this comment, it’s used in `update_lints`
9-
pub const ALL_LINTS: [Lint; 333] = [
9+
pub const ALL_LINTS: [Lint; 334] = [
1010
Lint {
1111
name: "absurd_extreme_comparisons",
1212
group: "correctness",
@@ -714,6 +714,13 @@ pub const ALL_LINTS: [Lint; 333] = [
714714
deprecation: None,
715715
module: "copies",
716716
},
717+
Lint {
718+
name: "ifs_same_cond_fn",
719+
group: "pedantic",
720+
desc: "consecutive `ifs` with the same function call",
721+
deprecation: None,
722+
module: "copies",
723+
},
717724
Lint {
718725
name: "implicit_hasher",
719726
group: "style",

‎tests/ui/same_functions_in_if_condition.rs‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
#![warn(clippy::same_functions_in_if_condition)]
2+
#![allow(clippy::ifs_same_cond)] // This warning is different from `ifs_same_cond`.
3+
#![allow(clippy::if_same_then_else, clippy::comparison_chain)] // all empty blocks
4+
5+
fn function() -> bool {
6+
true
7+
}
8+
9+
fn fn_arg(_arg: u8) -> bool {
10+
true
11+
}
12+
13+
struct Struct;
14+
15+
impl Struct {
16+
fn method(&self) -> bool {
17+
true
18+
}
19+
fn method_arg(&self, _arg: u8) -> bool {
20+
true
21+
}
22+
}
23+
24+
fn ifs_same_cond_fn() {
25+
let a = 0;
26+
let obj = Struct;
27+
28+
if function() {
29+
} else if function() {
30+
//~ ERROR ifs same condition
31+
}
32+
33+
if fn_arg(a) {
34+
} else if fn_arg(a) {
35+
//~ ERROR ifs same condition
36+
}
37+
38+
if obj.method() {
39+
} else if obj.method() {
40+
//~ ERROR ifs same condition
41+
}
42+
43+
if obj.method_arg(a) {
44+
} else if obj.method_arg(a) {
45+
//~ ERROR ifs same condition
46+
}
47+
48+
let mut v = vec![1];
49+
if v.pop() == None {
50+
//~ ERROR ifs same condition
51+
} else if v.pop() == None {
52+
}
53+
54+
if v.len() == 42 {
55+
//~ ERROR ifs same condition
56+
} else if v.len() == 42 {
57+
}
58+
59+
if v.len() == 1 {
60+
// ok, different conditions
61+
} else if v.len() == 2 {
62+
}
63+
64+
if fn_arg(0) {
65+
// ok, different arguments.
66+
} else if fn_arg(1) {
67+
}
68+
69+
if obj.method_arg(0) {
70+
// ok, different arguments.
71+
} else if obj.method_arg(1) {
72+
}
73+
74+
if a == 1 {
75+
// ok, warning is on `ifs_same_cond` behalf.
76+
} else if a == 1 {
77+
}
78+
}
79+
80+
fn main() {}

‎tests/ui/same_functions_in_if_condition.stderr‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
error: this `if` has the same function call as a previous if
2+
--> $DIR/same_functions_in_if_condition.rs:29:15
3+
|
4+
LL | } else if function() {
5+
| ^^^^^^^^^^
6+
|
7+
= note: `-D clippy::same-functions-in-if-condition` implied by `-D warnings`
8+
note: same as this
9+
--> $DIR/same_functions_in_if_condition.rs:28:8
10+
|
11+
LL | if function() {
12+
| ^^^^^^^^^^
13+
14+
error: this `if` has the same function call as a previous if
15+
--> $DIR/same_functions_in_if_condition.rs:34:15
16+
|
17+
LL | } else if fn_arg(a) {
18+
| ^^^^^^^^^
19+
|
20+
note: same as this
21+
--> $DIR/same_functions_in_if_condition.rs:33:8
22+
|
23+
LL | if fn_arg(a) {
24+
| ^^^^^^^^^
25+
26+
error: this `if` has the same function call as a previous if
27+
--> $DIR/same_functions_in_if_condition.rs:39:15
28+
|
29+
LL | } else if obj.method() {
30+
| ^^^^^^^^^^^^
31+
|
32+
note: same as this
33+
--> $DIR/same_functions_in_if_condition.rs:38:8
34+
|
35+
LL | if obj.method() {
36+
| ^^^^^^^^^^^^
37+
38+
error: this `if` has the same function call as a previous if
39+
--> $DIR/same_functions_in_if_condition.rs:44:15
40+
|
41+
LL | } else if obj.method_arg(a) {
42+
| ^^^^^^^^^^^^^^^^^
43+
|
44+
note: same as this
45+
--> $DIR/same_functions_in_if_condition.rs:43:8
46+
|
47+
LL | if obj.method_arg(a) {
48+
| ^^^^^^^^^^^^^^^^^
49+
50+
error: this `if` has the same function call as a previous if
51+
--> $DIR/same_functions_in_if_condition.rs:51:15
52+
|
53+
LL | } else if v.pop() == None {
54+
| ^^^^^^^^^^^^^^^
55+
|
56+
note: same as this
57+
--> $DIR/same_functions_in_if_condition.rs:49:8
58+
|
59+
LL | if v.pop() == None {
60+
| ^^^^^^^^^^^^^^^
61+
62+
error: this `if` has the same function call as a previous if
63+
--> $DIR/same_functions_in_if_condition.rs:56:15
64+
|
65+
LL | } else if v.len() == 42 {
66+
| ^^^^^^^^^^^^^
67+
|
68+
note: same as this
69+
--> $DIR/same_functions_in_if_condition.rs:54:8
70+
|
71+
LL | if v.len() == 42 {
72+
| ^^^^^^^^^^^^^
73+
74+
error: aborting due to 6 previous errors
75+

0 commit comments

Comments
 (0)