Merge pull request #2827 from 0ndorio/lint/cmp_operators_on_partial_cmp

Added lint to avoid negated comparisions on partially ordered types. (fixes #2626)

Author: oli-obk

clippy_lints/src/booleans.rs

@@ -4,7 +4,7 @@ use rustc::hir::intravisit::*;
 use syntax::ast::{LitKind, NodeId, DUMMY_NODE_ID};
 use syntax::codemap::{dummy_spanned, Span, DUMMY_SP};
 use syntax::util::ThinVec;
-use crate::utils::{in_macro, paths, match_type, snippet_opt, span_lint_and_then, SpanlessEq};
+use crate::utils::{in_macro, paths, match_type, snippet_opt, span_lint_and_then, SpanlessEq, get_trait_def_id, implements_trait};
 
 /// **What it does:** Checks for boolean expressions that can be written more
 /// concisely.
@@ -122,6 +122,11 @@ impl<'a, 'tcx, 'v> Hir2Qmm<'a, 'tcx, 'v> {
             }
             let negated = match e.node {
                 ExprBinary(binop, ref lhs, ref rhs) => {
+ 
+                    if !implements_ord(self.cx, lhs) {
+                        continue;
+                    }
+
                     let mk_expr = |op| {
                         Expr {
                             id: DUMMY_NODE_ID,
@@ -174,6 +179,11 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
     fn simplify_not(&self, expr: &Expr) -> Option<String> {
         match expr.node {
             ExprBinary(binop, ref lhs, ref rhs) => {
+
+                if !implements_ord(self.cx, lhs) {
+                    return None;
+                }
+
                 match binop.node {
                     BiEq => Some(" != "),
                     BiNe => Some(" == "),
@@ -444,3 +454,10 @@ impl<'a, 'tcx> Visitor<'tcx> for NonminimalBoolVisitor<'a, 'tcx> {
         NestedVisitorMap::None
     }
 }
+
+
+fn implements_ord<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, expr: &Expr) -> bool {
+    let ty = cx.tables.expr_ty(expr);
+    get_trait_def_id(cx, &paths::ORD)
+        .map_or(false, |id| implements_trait(cx, ty, id, &[]))
+}

clippy_lints/src/lib.rs

@@ -169,6 +169,7 @@ pub mod needless_borrowed_ref;
 pub mod needless_continue;
 pub mod needless_pass_by_value;
 pub mod needless_update;
+pub mod neg_cmp_op_on_partial_ord;
 pub mod neg_multiply;
 pub mod new_without_default;
 pub mod no_effect;
@@ -419,7 +420,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box map_unit_fn::Pass);
     reg.register_late_lint_pass(box infallible_destructuring_match::Pass);
     reg.register_late_lint_pass(box inherent_impl::Pass::default());
-
+    reg.register_late_lint_pass(box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd);
 
     reg.register_lint_group("clippy_restriction", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -501,6 +502,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         booleans::LOGIC_BUG,
         booleans::NONMINIMAL_BOOL,
         bytecount::NAIVE_BYTECOUNT,
+        neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD,
         collapsible_if::COLLAPSIBLE_IF,
         const_static_lifetime::CONST_STATIC_LIFETIME,
         copies::IF_SAME_THEN_ELSE,

clippy_lints/src/neg_cmp_op_on_partial_ord.rs

@@ -0,0 +1,93 @@
+use rustc::hir::*;
+use rustc::lint::*;
+
+use crate::utils::{self, paths};
+
+/// **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_clippy_lint! {
+    pub NEG_CMP_OP_ON_PARTIAL_ORD,
+    complexity,
+    "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, &paths::ORD) {
+                        utils::implements_trait(cx, ty, id, &[])
+                    } else {
+                        return;
+                    }
+                };
+
+                let implements_partial_ord = {
+                    if let Some(id) = utils::get_trait_def_id(cx, &paths::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."
+                    )
+                }
+            }
+        }
+    }
+}

clippy_lints/src/utils/paths.rs

@@ -57,6 +57,8 @@ pub const OPS_MODULE: [&str; 2] = ["core", "ops"];
 pub const OPTION: [&str; 3] = ["core", "option", "Option"];
 pub const OPTION_NONE: [&str; 4] = ["core", "option", "Option", "None"];
 pub const OPTION_SOME: [&str; 4] = ["core", "option", "Option", "Some"];
+pub const ORD: [&str; 3] = ["core", "cmp", "Ord"];
+pub const PARTIAL_ORD: [&str; 3] = ["core", "cmp", "PartialOrd"];
 pub const PTR_NULL: [&str; 2] = ["ptr", "null"];
 pub const PTR_NULL_MUT: [&str; 2] = ["ptr", "null_mut"];
 pub const RANGE: [&str; 3] = ["core", "ops", "Range"];

tests/ui/booleans.rs

@@ -114,3 +114,13 @@ fn warn_for_built_in_methods_with_negation() {
     if !res.is_some() { }
     if !res.is_none() { }
 }
+
+#[allow(neg_cmp_op_on_partial_ord)]
+fn dont_warn_for_negated_partial_ord_comparision() {
+    let a: f64 = unimplemented!();
+    let b: f64 = unimplemented!();
+    let _ = !(a < b);
+    let _ = !(a <= b);
+    let _ = !(a > b);
+    let _ = !(a >= b);
+}

tests/ui/neg_cmp_op_on_partial_ord.rs

@@ -0,0 +1,58 @@
+/// 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;
+
+#[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;
+}
+

tests/ui/neg_cmp_op_on_partial_ord.stderr

@@ -0,0 +1,28 @@
+error: 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.
+  --> $DIR/neg_cmp_op_on_partial_ord.rs:17:21
+   |
+17 |     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 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.
+  --> $DIR/neg_cmp_op_on_partial_ord.rs:20:30
+   |
+20 |     let _not_less_or_equal = !(a_value <= another_value);
+   |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: 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.
+  --> $DIR/neg_cmp_op_on_partial_ord.rs:23:24
+   |
+23 |     let _not_greater = !(a_value > another_value);
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: 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.
+  --> $DIR/neg_cmp_op_on_partial_ord.rs:26:33
+   |
+26 |     let _not_greater_or_equal = !(a_value >= another_value);
+   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 4 previous errors
+