detect if comparison chain that could be a tuple comparison instead

[MBons]

The if_same_then_else lint triggered on the following code I use to sort some stuff:

positions.sort_by(|&(x1, y1, z1), &(x2, y2, z2)| {
    use std::cmp::Ordering::*;
    if z1 < z2 {
        Less
    } else if z1 > z2 {
        Greater
    } else if x1 > x2 {
        Less
    } else if x1 < x2 {
        Greater
    } else if y1 < y2 {
        Less
    } else {
        Greater
    }
});

I'm not sure if Clippy could reason about this or if it's even worth the trouble thinking about that but perhaps this is worthy of a footnote in the wiki?

[mcarton]

Isn't that (z1, x2, y1) < (z2, x1, y2) or something?

[MBons]

I wasn't aware comparing tuples worked like that, thanks.

[oli-obk]

We should be detecting this pattern and suggesting the tuple comparison

[marhoily]

Also when chain if ... else if ... blocks the "if_same_then_else" is inapplicable, because "else if" has one more condition, that means that if and else branches are not the same. My particular sample is here. Should I create another issue to detect "else if"?

[mcarton]

My view on that is that the lint catches what is most of the time a copy-and-paste error and reports it as wanted. For the few cases where that behavior is actually wanted, explicitly #[allow]ing the lint is acceptable.
This is similar to how unsafe works: most of the time unsafe stuffs are bad, so it's forbidden by default. For the few cases where you know what you are doing, you can explicitly add unsafe.

[marhoily]

I'm sorry, but I just don't understand.

When I say if something { OK } else { OK } then I understand why it looks suspicious. Why does it look suspicious when I say if something { OK } else if something_else { OK } else ... ?

I'm not asking why false positives are possible. I'm asking for an example when ...else if... looks suspicious. Can you come up with any counter example?

[mcarton]

if something { OK } else if something_else { OK } else ... can be written if something || something_else { OK } else .... In you case it couldn't be rewritten that easily though.
The point of the lint is catching people copy-pasting code in potentially long chains of ifs, make a small variation somewhere, and forget to update one case. Viva64 has published a list of real gotchas found by a similar lint (look at MySQL or Trans-Proteomic Pipeline).

[marhoily]

I understand you position now, though it is not correct about something || something_else because I've a bit more convoluted case.

Would you at least consider making it 2 different lints so that we could keep disagreeing? one that says "if and else branches are the same" and the other one "there are 2 identical branches in a chain of else-ifs"?

[mathstuf]

I think that the lint should be silent for:

if a {
    X
} else if b {
    Y
} else {
    X
}

The use case is that a checks that some kind of filtering is even applicable, b checks whether the filtering can be done and does it, and then if we reach the end, there's nothing to do, so we do what would have been done without any filtering.