Rust-clippy: suggest replacing if comparison chain with `cmp` match

[oli-cosmian]

If one writes

if x < y {
     ...
} else if x > y {
     ...
} else {
     ...
}

one should instead be writing

match x.cmp(&y) {
     Ordering::Less => ...,
     Ordering::Greater => ...,
     Ordering::Equal => ...,
}

[james9909]

I can take a stab at this issue

[flip1995]

Thanks! Let us know if you have any questions.

Also make sure to take a look at https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md. That's a really good introduction on writing lints.

[untitaker]

This requires an additional import and is harder to read. Why is this a default lint?

[oli-cosmian]

I don't think it's harder to read. imo this is much clearer what's going on, because you have the three cases very explicitly.

Also I don't consider additional or fewer imports to be relevant for whether a lint is useful or not

[untitaker]

In my book a < b will always be easier to read than a.cmp(b) == Ordering::Less. Irrespective of whether your proposed coding style provides more compile-time safety than a regular comparison (I don't really care), because that has nothing to do with readability.

Why would I import more traits and enums from std if I can achieve the same thing without? e.g. I don't import FromStr in favor of using .parse() (even though FromStr might be more explicit and less indirect)

FWIW this lint also triggers for code that does not intend to handle all three cases: https://github.com/getsentry/symbolic/commit/f928869b64f43112ec70ecd87aa24441ebc899e6#diff-526c79c0b4fc6984353f21705ba99a12R396

and it also triggers for code that wants to partialcmp instead of cmp.

[mitsuhiko]

In the case of symbolic in particular it’s especially weird because the type doesn’t even implement Ord but only PartialOrd which requires one to use partial_cmp which is extra ugly.

[oli-cosmian]

FWIW this lint also triggers for code that does not intend to handle all three cases:

That looks also fine to me. I think it should be warn by default.

Since some situations require partial_cmp, we should fix the lint of course.

To me this lint is a clear improvement (especially since it does help catch bugs).

[raphlinus]

FWIW, I dislike this lint, and find the direct comparisons easier to read. As a result, https://github.com/xi-editor/druid/pull/406 is adding #[allow(clippy:comparison_chain)].

[llogiq]

@raphlinus that's OK. Readability is not something that can be decided in the general case, but must be approached on a case-by-case basis.

[untitaker]

Cool, so you agree such a general lint in defaults is wrong?

[flip1995]

A check for Ord was implemented in #4827. Maybe it didn't hit stable/beta/nightly (Not sure where we're in the release cycle, since I was on vacation the last week) yet.

Cool, so you agree such a general lint in defaults is wrong?

This is a style lint, which is the "weakest" default group and is expected to have the most allows. I also agree, that an exhaustive match is more rusty, than an if with comparisons.

this lint also triggers for code that does not intend to handle all three cases

See #4725