Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clippy::comparison_chain false positive on PartialOrd #4827

Closed
timbodeit opened this issue Nov 18, 2019 · 1 comment · Fixed by #4842
Closed

clippy::comparison_chain false positive on PartialOrd #4827

timbodeit opened this issue Nov 18, 2019 · 1 comment · Fixed by #4842
Labels
A-lint Area: New lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy

Comments

@timbodeit
Copy link
Contributor

The comparison_chain lint is defined as:

Checks comparison chains written with if that can be rewritten with match and cmp.

However it also triggers on comparison chains written with if who's compared type does not implement Ord and doesn't offer cmp.

One such example would be f64. This type implements PartialOrd but not Ord. It provides < and > operators without providing cmp.
While in theory the same concept of using match could also be applied to the result of partial_cmp, I personally find matching over Optional<Ordering> much less appealing than matching over Ordering.

Should we apply the same lint to all PartialOrd cases and change the documentation to make this the official desired behavior? Or should we consider this a bug and change the implementation, so that the lint only fires if cmp is available on the relevant type (as specified in the documentation)?

cc @james9909


Example implementation where clippy::comparison_chain fires, eventough cmp is not available:

fn f(x: f64, y: f64) {
    if x > y {
        a()
    } else if x < y {
        b()
    } else {
        c()
    }
}

Reproduced on:
clippy 0.0.212 (3aea860 2019-09-03)
clippy 0.0.212 (4e7e71b 2019-10-11)

@flip1995
Copy link
Member

flip1995 commented Nov 19, 2019

We should only lint for types, that implement Ord. It would be possible to implement the same lint for PartialOrd, but put it in the pedantic group.

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages A-lint Area: New lints labels Nov 19, 2019
@timbodeit timbodeit changed the title clippy::comparison_chain false positive on PartialOrd or desired behavior clippy::comparison_chain false positive on PartialOrd Nov 19, 2019
timbodeit added a commit to timbodeit/rust-clippy that referenced this issue Nov 23, 2019
Only emit lint, if `cmp` is actually available on the type being
compared. Don't emit lint in cases where only `PartialOrd` is
implemented.
timbodeit added a commit to timbodeit/rust-clippy that referenced this issue Nov 24, 2019
Only emit lint, if `cmp` is actually available on the type being
compared. Don't emit lint in cases where only `PartialOrd` is
implemented.
bors added a commit that referenced this issue Nov 28, 2019
Rollup of 3 pull requests

Successful merges:

 - #4832 (Add some positive examples to lint docs)
 - #4842 ([comparison_chain] #4827 Check `core::cmp::Ord` is implemented)
 - #4847 (fixing a typo)

Failed merges:

changelog: none

r? @ghost
@bors bors closed this as completed in 1165176 Nov 28, 2019
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue May 5, 2020
Changes:
````
Normalize custom ICE test
Rustup to rust-lang/rust#64736
Use assert_crate_local for a more explicit error
Rustup to rust-lang/rust#66789
account for external macro in MISSING_INLINE_IN_PUBLIC_ITEMS lint
build(tests/fmt): use shared target dir
chore: fix and split some ui tests on 32bit system
build: set up build job for i686 targets
remove needless my_lint ui test
git quiet
deploy: cd to out/ before adding files to git
Less needless_doctest_main false positives
fmt
Feed the dog
Use rustc_env instead of exec_env for test
Make triggering this lint less likely 📎
Use exec_env to set backtrace level and normalize output
Update custom ICE function with latest rustc
Use Clippy version in ICE message
Add custom ICE message that points to Clippy repo
Fix master deployment
Run update_lints
Add projections check to EUV for escape analysis
Use infer_ctxt
Move use_self to nursery
Use `println!` on success instead of `eprintln!`
Revert "Disable chalk integration test. Output too large"
Remove the old integration-tests.sh script
Use rust implementation for integration tests in CI
Rust implementation of integration test
Don't error on clippy.toml of dependencies
Fix categorizations
Fix arguments on ExprUseVisitor::new
euv moved from middle to typeck
cmt_ -> Place
build: check if RTIM is not installed
make use of Result::map_or
trigger string_lit_as_bytes when literal has escapes
Remove negative float literal checks.
Enable deny-warnings feature everywhere in CI
Remove unused debugging feature
implemented `as_conversions` lint
fixing a typo
[comparison_chain] rust-lang#4827 Check `core::cmp::Ord` is implemented
add a good example for the approx_const lint
Add suggested good cases in docs for lifetimes lint
````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants