enhance [ifs_same_cond] to warn same immutable method calls as well

[J-ZhengLi]

fixes: #10272

---

changelog: Enhancement: [ifs_same_cond]: Now also detects immutable method calls.
#10350

[rustbot]

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

[J-ZhengLi]

feel very insecure about this 🤦‍♂️ , is there any side-effect of doing this that I'm not aware of?

[PatchMixolydic]

I'm not familiar with the method behind this check, but if it only checks the mutability of the binding, this might lead to false positives with interior mutable types:

use std::cell::Cell;

fn main() {
    let x = Cell::new(true);

    if !x.get() {
        unreachable!("x should be true");
    } else if !x.take() {
        unreachable!("x should've been true");
    } else if !x.get() {
        println!("OK");
    } else {
        unreachable!("x should be false");
    }
}

(Of course, it's possible that this is covered by SpanlessEq::eq_expr and I completely missed it ^^;)

[J-ZhengLi]

I'm not familiar with the method behind this check, but if it only checks the mutability of the binding, this might lead to false positives with interior mutable types:

use std::cell::Cell;

fn main() {
    let x = Cell::new(true);

    if !x.get() {
        unreachable!("x should be true");
    } else if !x.take() {
        unreachable!("x should've been true");
    } else if !x.get() {
        println!("OK");
    } else {
        unreachable!("x should be false");
    }
}

(Of course, it's possible that this is covered by SpanlessEq::eq_expr and I completely missed it ^^;)

Thank you~ I just tested your example but it didn't trigger any warning, so I guess eq_expr somehow does cover this scenario. 😄

Oops, my bad, it doesn't, I'll try to fix it

[xFrednet]

Hey, thank you for the PR. I would have also brought up the point of inner mutability. Thank you, @PatchMixolydic, for the comment.

I see three solutions:

1. Scan the called method for inner mutability. This will only work for local methods, as external ones are not loaded by rustc AFAIK, and will be complex with little benefit.
2. Create an allow list, which would avoid false positives, but would also be less flexible
3. Add a configuration value that lints these methods by default, but can be turned off, if the project uses a lot of inner mutability.

From these options, I recommend the last one. You can take a look at this link, if you want to implement this solution: https://doc.rust-lang.org/clippy/development/adding_lints.html#adding-configuration-to-a-lint

You are welcome to reach out if you have any questions :)

[J-ZhengLi]

Hey, thank you for the PR. I would have also brought up the point of inner mutability. Thank you, @PatchMixolydic, for the comment.

I see three solutions:

1. Scan the called method for inner mutability. This will only work for local methods, as external ones are not loaded by rustc AFAIK, and will be complex with little benefit.

2. Create an allow list, which would avoid false positives, but would also be less flexible

3. Add a configuration value that lints these methods by default, but can be turned off, if the project uses a lot of inner mutability.

From these options, I recommend the last one. You can take a look at this link, if you want to implement this solution: https://doc.rust-lang.org/clippy/development/adding_lints.html#adding-configuration-to-a-lint

You are welcome to reach out if you have any questions :)

well I do have a question, what if get the type of the caller instead and skip the check of Cell, RefCell types and mutable pointers completely, would that be achievable?

update: I noticed that mutable_key_type already got a configuration called ignore_interior_mutability, which seems allowing people to "whitelisting" certain types, maybe I could try using the same conf directly.

[xFrednet]

what if get the type of the caller instead

I believe this is already how the lint operates. I thought the question was about structs with field with internal mutability.

update: I noticed that mutable_key_type already got a configuration called ignore_interior_mutability, which seems allowing people to "whitelisting" certain types, maybe I could try using the same conf directly.

That sounds like a perfect approach, IMO 👍

[xFrednet]

The implementation looks good overall. I have some small suggestions regarding the style and type resolution, but they should hopefully be simple to fix. :)

[xFrednet]

Could you explain this check? I understand that it checks if the caller is a local value and if an init expression is present. But why should this be ignored if this is the case?

[J-ZhengLi]

Because the documentation of find_binding_init says: ... Return None if the binding is mutable..., I thought it was pretty convenient lol, unless there are more optimized way to check whether a variable is mutable or not

[xFrednet]

Ah, okay, that's interesting. I understand why. I guess this is a good heuristic. It will have some false negatives, but those are better than false positives :)

[xFrednet]

The docs imply to me that Arc and other pointer types like Cell are included in the list, even if they aren't part of the default configuration. Do you maybe want to add them?

[J-ZhengLi]

Oh wait, I think I did the opposite of what that config was designed to do... the name of that config implies: "these types has inner mutability but will be ignored, so lint those anyway".

I got confused by the name lol, I thought it means skiping lint checks for them.

I realize it when the uitests for [mut_key] fails after I added them.

[xFrednet]

Now that you say it, it's a bit confusing 😅. I guess we'll leave it like this now, since it's not new.

[xFrednet]

Looks good to me, thank you for the change and nice refactoring. :)

@bors r+

[bors]

📌 Commit f4ccb06 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit f4ccb06 with merge 2c9f74c...

[bors]

💔 Test failed - checks-action_test

[J-ZhengLi]

Ahh, looks like some book test is failing, that's new for me, what should I do 😱

[xFrednet]

Right, we've added that recently, and I still forget about mentioning that. Luckily we have bors as our bouncer ^^. Thank you for fixing the CI :)

@bors r+

[bors]

📌 Commit 011bb46 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit 011bb46 with merge ff843ac...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing ff843ac to master...