Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Added lint to avoid negated comparisions on partially ordered types.
- Loading branch information
Showing
4 changed files
with
185 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
use rustc::hir::*; | ||
use rustc::lint::*; | ||
|
||
use crate::utils; | ||
|
||
const ORD: [&str; 3] = ["core", "cmp", "Ord"]; | ||
const PARTIAL_ORD: [&str; 3] = ["core", "cmp", "PartialOrd"]; | ||
|
||
/// **What it does:** | ||
/// Checks for the usage of negated comparision operators on types which only implement | ||
/// `PartialOrd` (e.g. `f64`). | ||
/// | ||
/// **Why is this bad?** | ||
/// These operators make it easy to forget that the underlying types actually allow not only three | ||
/// potential Orderings (Less, Equal, Greater) but also a forth one (Uncomparable). Escpeccially if | ||
/// the operator based comparision result is negated it is easy to miss that fact. | ||
/// | ||
/// **Known problems:** None. | ||
/// | ||
/// **Example:** | ||
/// | ||
/// ```rust | ||
/// use core::cmp::Ordering; | ||
/// | ||
/// // Bad | ||
/// let a = 1.0; | ||
/// let b = std::f64::NAN; | ||
/// | ||
/// let _not_less_or_equal = !(a <= b); | ||
/// | ||
/// // Good | ||
/// let a = 1.0; | ||
/// let b = std::f64::NAN; | ||
/// | ||
/// let _not_less_or_equal = match a.partial_cmp(&b) { | ||
/// None | Some(Ordering::Greater) => true, | ||
/// _ => false, | ||
/// }; | ||
/// ``` | ||
declare_lint! { | ||
pub NEG_CMP_OP_ON_PARTIAL_ORD, Warn, | ||
"The use of negated comparision operators on partially orded types may produce confusing code." | ||
} | ||
|
||
pub struct NoNegCompOpForPartialOrd; | ||
|
||
impl LintPass for NoNegCompOpForPartialOrd { | ||
fn get_lints(&self) -> LintArray { | ||
lint_array!(NEG_CMP_OP_ON_PARTIAL_ORD) | ||
} | ||
} | ||
|
||
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NoNegCompOpForPartialOrd { | ||
|
||
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { | ||
if_chain! { | ||
|
||
if let Expr_::ExprUnary(UnOp::UnNot, ref inner) = expr.node; | ||
if let Expr_::ExprBinary(ref op, ref left, _) = inner.node; | ||
if let BinOp_::BiLe | BinOp_::BiGe | BinOp_::BiLt | BinOp_::BiGt = op.node; | ||
|
||
then { | ||
|
||
let ty = cx.tables.expr_ty(left); | ||
|
||
let implements_ord = { | ||
if let Some(id) = utils::get_trait_def_id(cx, &ORD) { | ||
utils::implements_trait(cx, ty, id, &[]) | ||
} else { | ||
return; | ||
} | ||
}; | ||
|
||
let implements_partial_ord = { | ||
if let Some(id) = utils::get_trait_def_id(cx, &PARTIAL_ORD) { | ||
utils::implements_trait(cx, ty, id, &[]) | ||
} else { | ||
return; | ||
} | ||
}; | ||
|
||
if implements_partial_ord && !implements_ord { | ||
cx.span_lint( | ||
NEG_CMP_OP_ON_PARTIAL_ORD, | ||
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\ | ||
that the two values could be incomparable." | ||
) | ||
} | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/// This test case utilizes `f64` an easy example for `PartialOrd` only types | ||
/// but the lint itself actually validates any expression where the left | ||
/// operand implements `PartialOrd` but not `Ord`. | ||
|
||
use std::cmp::Ordering; | ||
|
||
#[allow(nonminimal_bool)] | ||
#[warn(neg_cmp_op_on_partial_ord)] | ||
fn main() { | ||
|
||
let a_value = 1.0; | ||
let another_value = 7.0; | ||
|
||
// --- Bad --- | ||
|
||
|
||
// Not Less but potentially Greater, Equal or Uncomparable. | ||
let _not_less = !(a_value < another_value); | ||
|
||
// Not Less or Equal but potentially Greater or Uncomparable. | ||
let _not_less_or_equal = !(a_value <= another_value); | ||
|
||
// Not Greater but potentially Less, Equal or Uncomparable. | ||
let _not_greater = !(a_value > another_value); | ||
|
||
// Not Greater or Equal but potentially Less or Uncomparable. | ||
let _not_greater_or_equal = !(a_value >= another_value); | ||
|
||
|
||
// --- Good --- | ||
|
||
|
||
let _not_less = match a_value.partial_cmp(&another_value) { | ||
None | Some(Ordering::Greater) | Some(Ordering::Equal) => true, | ||
_ => false, | ||
}; | ||
let _not_less_or_equal = match a_value.partial_cmp(&another_value) { | ||
None | Some(Ordering::Greater) => true, | ||
_ => false, | ||
}; | ||
let _not_greater = match a_value.partial_cmp(&another_value) { | ||
None | Some(Ordering::Less) | Some(Ordering::Equal) => true, | ||
_ => false, | ||
}; | ||
let _not_greater_or_equal = match a_value.partial_cmp(&another_value) { | ||
None | Some(Ordering::Less) => true, | ||
_ => false, | ||
}; | ||
|
||
|
||
// --- Should not trigger --- | ||
|
||
|
||
let _ = a_value < another_value; | ||
let _ = a_value <= another_value; | ||
let _ = a_value > another_value; | ||
let _ = a_value >= another_value; | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
error: The use of negated comparision operators on partially ordedtypes produces code that is hard to read and refactor. Pleaseconsider to use the partial_cmp() instead, to make it clearthat the two values could be incomparable. | ||
--> $DIR/neg_cmp_op_on_partial_ord.rs:18:21 | ||
| | ||
18 | let _not_less = !(a_value < another_value); | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: `-D neg-cmp-op-on-partial-ord` implied by `-D warnings` | ||
|
||
error: The use of negated comparision operators on partially ordedtypes produces code that is hard to read and refactor. Pleaseconsider to use the partial_cmp() instead, to make it clearthat the two values could be incomparable. | ||
--> $DIR/neg_cmp_op_on_partial_ord.rs:21:30 | ||
| | ||
21 | let _not_less_or_equal = !(a_value <= another_value); | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: The use of negated comparision operators on partially ordedtypes produces code that is hard to read and refactor. Pleaseconsider to use the partial_cmp() instead, to make it clearthat the two values could be incomparable. | ||
--> $DIR/neg_cmp_op_on_partial_ord.rs:24:24 | ||
| | ||
24 | let _not_greater = !(a_value > another_value); | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: The use of negated comparision operators on partially ordedtypes produces code that is hard to read and refactor. Pleaseconsider to use the partial_cmp() instead, to make it clearthat the two values could be incomparable. | ||
--> $DIR/neg_cmp_op_on_partial_ord.rs:27:33 | ||
| | ||
27 | let _not_greater_or_equal = !(a_value >= another_value); | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: aborting due to 4 previous errors | ||
|