Merge pull request #47 from Manishearth/float_cmp

Added new 'float_cmp' lint (see issue #46)

README.md

@@ -16,6 +16,15 @@ Lints included in this crate:
  - `needless_bool` : Warns on if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }`
  - `ptr_arg`: Warns on fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively
  - `approx_constant`: Warns if the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found and suggests to use the constant
+ - `cmp_nan`: Denies comparisons to NAN (which will always return false, which is probably not intended)
+ - `float_cmp`: Warns on `==` or `!=` comparisons of floaty typed values. As floating-point operations usually involve rounding errors, it is always better to check for approximate equality within some small bounds
+
+To use, add the following lines to your Cargo.toml:
+
+```
+[dev-dependencies.rust-clippy]
+git = "https://github.com/Manishearth/rust-clippy"
+```
 
 In your code, you may add `#![plugin(clippy)]` to use it (you may also need to include a `#![feature(plugin)]` line)
 

src/lib.rs

@@ -34,6 +34,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_lint_pass(box ptr_arg::PtrArg as LintPassObject);
     reg.register_lint_pass(box needless_bool::NeedlessBool as LintPassObject);
     reg.register_lint_pass(box approx_const::ApproxConstant as LintPassObject);
+    reg.register_lint_pass(box misc::FloatCmp as LintPassObject);
     
     reg.register_lint_group("clippy", vec![types::BOX_VEC, types::LINKEDLIST,
                                            misc::SINGLE_MATCH, misc::STR_TO_STRING,
@@ -41,6 +42,6 @@ pub fn plugin_registrar(reg: &mut Registry) {
                                            bit_mask::BAD_BIT_MASK, ptr_arg::PTR_ARG,
                                            needless_bool::NEEDLESS_BOOL,
                                            approx_const::APPROX_CONSTANT,
-                                           misc::CMP_NAN
+                                           misc::CMP_NAN, misc::FLOAT_CMP,
                                            ]);
 }

src/misc.rs

@@ -1,14 +1,22 @@
 use syntax::ptr::P;
 use syntax::ast;
 use syntax::ast::*;
-use syntax::ast_util::is_comparison_binop;
+use syntax::ast_util::{is_comparison_binop, binop_to_string};
 use syntax::visit::{FnKind};
 use rustc::lint::{Context, LintPass, LintArray, Lint, Level};
-use rustc::middle::ty::{self, expr_ty, ty_str, ty_ptr, ty_rptr};
+use rustc::middle::ty::{self, expr_ty, ty_str, ty_ptr, ty_rptr, ty_float};
 use syntax::codemap::Span;
 
+
 use types::span_note_and_lint;
 
+fn walk_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> {
+	match ty.sty {
+		ty_ptr(ref tm) | ty_rptr(_, ref tm) => walk_ty(tm.ty),
+		_ => ty
+	}
+}
+
 /// Handles uncategorized lints
 /// Currently handles linting of if-let-able matches
 #[allow(missing_copy_implementations)]
@@ -71,13 +79,6 @@ impl LintPass for StrToStringPass {
         }
 
         fn is_str(cx: &Context, expr: &ast::Expr) -> bool {
-            fn walk_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> {
-                //println!("{}: -> {}", depth, ty);
-                match ty.sty {
-                    ty_ptr(ref tm) | ty_rptr(_, ref tm) => walk_ty(tm.ty),
-                    _ => ty
-                }
-            }
             match walk_ty(expr_ty(cx.tcx, expr)).sty {
                 ty_str => true,
                 _ => false
@@ -110,7 +111,7 @@ impl LintPass for TopLevelRefPass {
     }
 }
 
-declare_lint!(pub CMP_NAN, Allow, "Deny comparisons to std::f32::NAN or std::f64::NAN");
+declare_lint!(pub CMP_NAN, Deny, "Deny comparisons to std::f32::NAN or std::f64::NAN");
 
 #[derive(Copy,Clone)]
 pub struct CmpNan;
@@ -139,3 +140,32 @@ fn check_nan(cx: &Context, path: &Path, span: Span) {
 		cx.span_lint(CMP_NAN, span, "Doomed comparison with NAN, use std::{f32,f64}::is_nan instead");
 	});
 }
+
+declare_lint!(pub FLOAT_CMP, Warn,
+			  "Warn on ==/!= comparison of floaty values");
+			  
+#[derive(Copy,Clone)]
+pub struct FloatCmp;
+
+impl LintPass for FloatCmp {
+	fn get_lints(&self) -> LintArray {
+        lint_array!(FLOAT_CMP)
+	}
+	
+	fn check_expr(&mut self, cx: &Context, expr: &Expr) {
+		if let ExprBinary(ref cmp, ref left, ref right) = expr.node {
+			let op = cmp.node;
+			if (op == BiEq || op == BiNe) && (is_float(cx, left) || is_float(cx, right)) {
+				let map = cx.sess().codemap();
+				cx.span_lint(FLOAT_CMP, expr.span, &format!(
+					"{}-Comparison of f32 or f64 detected. You may want to change this to 'abs({} - {}) < epsilon' for some suitable value of epsilon",
+					binop_to_string(op), &*map.span_to_snippet(left.span).unwrap_or("..".to_string()), 
+					&*map.span_to_snippet(right.span).unwrap_or("..".to_string())));
+			}
+		}
+	}
+}
+
+fn is_float(cx: &Context, expr: &Expr) -> bool {
+	if let ty_float(_) = walk_ty(expr_ty(cx.tcx, expr)).sty { true } else { false }
+}

tests/compile-fail/cmp_nan.rs

@@ -2,6 +2,7 @@
 #![plugin(clippy)]
 
 #[deny(cmp_nan)]
+#[allow(float_cmp)]
 fn main() {
 	let x = 5f32;
 	x == std::f32::NAN; //~ERROR

tests/compile-fail/float_cmp.rs

@@ -0,0 +1,35 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+use std::ops::Add;
+
+const ZERO : f32 = 0.0;
+const ONE : f32 = ZERO + 1.0;
+
+fn twice<T>(x : T) -> T where T : Add<T, Output = T>, T : Copy {
+	x + x
+}
+
+#[deny(float_cmp)]
+#[allow(unused)]
+fn main() {
+	ZERO == 0f32; //~ERROR
+	ZERO == 0.0; //~ERROR
+	ZERO + ZERO != 1.0; //~ERROR
+	
+	ONE != 0.0; //~ERROR
+	twice(ONE) != ONE; //~ERROR
+	ONE as f64 != 0.0; //~ERROR
+	
+	let x : f64 = 1.0;
+	
+	x == 1.0; //~ERROR
+	x != 0f64; //~ERROR
+	
+	twice(x) != twice(ONE as f64); //~ERROR
+	
+	x < 0.0;
+	x > 0.0;
+	x <= 0.0;
+	x >= 0.0;	
+}