Added lint to avoid negated comparisions on partially ordered types. (fixes #2626) #2827
Conversation
0ndorio
force-pushed
the
lint/cmp_operators_on_partial_cmp
branch
from
5bcd6e0 to
4b4d617
Compare
| cx.span_lint( | ||
| NEG_CMP_OP_ON_PARTIAL_ORD, | ||
| expr.span, | ||
| "The use of negated comparision operators on partially orded\ |
| expr.span, | ||
| "The use of negated comparision operators on partially orded\ | ||
| types produces code that is hard to read and refactor. Please\ | ||
| consider to use the partial_cmp() instead, to make it clear\ |
There was a problem hiding this comment.
"the partial_cmp()" should be just partial_cmp
There was a problem hiding this comment.
Just partial_cmp or or also surrounded with backticks?
|
|
||
| let _not_less = match a_value.partial_cmp(&another_value) { | ||
| None | Some(Ordering::Greater) | Some(Ordering::Equal) => true, | ||
| _ => false, |
There was a problem hiding this comment.
weird indent and/or tabs instead of spaces
0ndorio
changed the title
clippy_lints/src/booleans.rs
Outdated
| fn implements_ord<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, expr: &Expr) -> Option<bool> { | ||
| let ty = cx.tables.expr_ty(expr); | ||
|
|
||
| return if let Some(id) = get_trait_def_id(cx, &paths::ORD) { |
There was a problem hiding this comment.
this can be written as get_trait_def_id(cx, &paths::ORD).map_or(false, implements_trait(cx, ty, id, &[]))
clippy_lints/src/booleans.rs
Outdated
| } | ||
|
|
||
|
|
||
| fn implements_ord<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, expr: &Expr) -> Option<bool> { |
There was a problem hiding this comment.
I think this should just return bool and false if the trait cannot be found (that would only happen in libcore to best of my knowledge)
clippy_lints/src/booleans.rs
Outdated
| None | ||
| }; | ||
| get_trait_def_id(cx, &paths::ORD) | ||
| .map(|id| implements_trait(cx, ty, id, &[])) |
There was a problem hiding this comment.
heads up: miri dogfood will tell you to turn this into map_or(false, |id|....
There was a problem hiding this comment.
Is this a hint miri directly outputs if you run it on this repo? I spent a few minutes to check if I can get it running but even if I use the intermediate xargo step it seems like I miss something as miri still fails with no mir for std::env::current_dir for cargo +nightly miri test.
…artially orded types.
0ndorio
force-pushed
the
lint/cmp_operators_on_partial_cmp
branch
from
2cc6cb1 to
b803f85
Compare
|
Simplified the map().unwrap() chain as you mentioned, added the missing test calls and rebased everything on top of latest master. |
0ndorio
force-pushed
the
lint/cmp_operators_on_partial_cmp
branch
from
b803f85 to
28f735b
Compare
|
Thanks! Looks great now |
Added a lint to warn about the usage of negated comparisons on partial ordered types as they always implicitly include the incomparable case (based on issue #2626).
Even if the lint itself seems to work, this isn't done yet as the
nonminimal_boollint still triggers on these cases. I am still a bit clueless which is best spot to inject the necessary checks into it so if anyone has a hint I would be glad to fix that.