New lint: Implement ifs_same_cond_fn

[popzxc]

changelog:

* New lints:
  * [`ifs_same_cond_fn`] [#4814](https://github.com/rust-lang/rust-clippy/pull/4814)

Current ifs_same_cond lint does not check function calls, since they can have side effects.

However, such a code can be considered too implicit and counterintuitive, so I believe that a lint to detect such scenarios may be useful.

[ghost]

What code would you suggest as a replacement? People will want to know how to change their code to silence the error.

[popzxc]

Good question. However, I believe that there is no obvious answer here.

In my opinion, implicit mutating within if is pretty dangerous as a concept, since usually you don't expect some check to mutate state.
Of course, there are examples of well-defined mutating interfaces which can be used without confusion (e.g. Iterator::next), but in general case
when you see code like:

if a.method() == some_value {
    // ...
} else if a.method() == some_value {
    // ...
}

or even

if a.method() {
    // ...
} else if a.method() {
    // ...
}

you won't think that a changes here in the first place.

However, the code like

let a_value = a.method();
if a_value == some_value {
    /// ...
}

let b_value = a.method();
if b_value == some_value {
    /// ...
}

is more likely to be understood correctly.

I've created a snippet on the Rust playground to explain what I mean in the code.

Surely, this check won't suite everyones needs and there are cases when mutating within if is good, but the same applies for e.g. match_same_arms lint.

That's why this lint is pedantic: it won't affect anybody who don't want it explicitly.

[popzxc]

And one more small note: even taking mutating functions into account, this case is pretty rare, and most of the time this lint will complain about situations like:

if some_vec.len() == 1 {
  // ...
} else if some_vec.len() == 1 {
 // ...
}

and that's the main purpose of the lint.

I guess it will be obvious to user that it's an error and he'll be able to resolve this issue himself :)

[popzxc]

@flip1995 r?

[flip1995]

Impl LGTM. Can you add a test, where the arguments of the function/method calls differ?

[flip1995]

This has to be done anyway for many more PRs, so just write the changelog in the PR body.

[flip1995]

SAME_FUNCTIONS_IN_IF_CONDITION, as per naming conventions. (putting allow before the lint name should produce an english sentence (more or less))

[flip1995]

Can you add a example to the lint doc, that shows how to rewrite such a if chain?

[popzxc]

Weird: travis fails, but locally cargo test passes just fine.

[flip1995]

Travis probably fails because of #4825. Don't worry about this.

[flip1995]

Thanks! Waiting for the rustup.