From 2787a60fc23accc81c9f9c7350b62c73f1c368a4 Mon Sep 17 00:00:00 2001 From: clippered Date: Sat, 4 Nov 2017 19:32:58 +1100 Subject: [PATCH] Fix #1142 float constant comparison lint --- clippy_lints/src/lib.rs | 1 + clippy_lints/src/misc.rs | 40 +++++++++++++++- tests/ui/float_cmp_const.rs | 31 ++++++++++++ tests/ui/float_cmp_const.stderr | 85 +++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 2 deletions(-) create mode 100644 tests/ui/float_cmp_const.rs create mode 100644 tests/ui/float_cmp_const.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5549c98aaf3..eea590f033f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -357,6 +357,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { arithmetic::INTEGER_ARITHMETIC, array_indexing::INDEXING_SLICING, assign_ops::ASSIGN_OPS, + misc::FLOAT_CMP_CONST, ]); reg.register_lint_group("clippy_pedantic", vec![ diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 18d7f7230a8..dfe1187916f 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -13,6 +13,7 @@ use utils::{get_item_name, get_parent_expr, implements_trait, in_constant, in_ma span_lint_and_then, walk_ptrs_ty}; use utils::sugg::Sugg; use syntax::ast::{FloatTy, LitKind, CRATE_NODE_ID}; +use consts::constant; /// **What it does:** Checks for function arguments and let bindings denoted as /// `ref`. @@ -200,6 +201,27 @@ declare_lint! { "using 0 as *{const, mut} T" } +/// **What it does:** Checks for (in-)equality comparisons on floating-point +/// value and constant, except in functions called `*eq*` (which probably +/// implement equality for a type involving floats). +/// +/// **Why is this bad?** Floating point calculations are usually imprecise, so +/// asking if two values are *exactly* equal is asking for trouble. For a good +/// guide on what to do, see [the floating point +/// guide](http://www.floating-point-gui.de/errors/comparison). +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// const ONE == 1.00f64 +/// x == ONE // where both are floats +/// ``` +declare_restriction_lint! { + pub FLOAT_CMP_CONST, + "using `==` or `!=` on float constants instead of comparing difference with an epsilon" +} + #[derive(Copy, Clone)] pub struct Pass; @@ -214,7 +236,8 @@ impl LintPass for Pass { REDUNDANT_PATTERN, USED_UNDERSCORE_BINDING, SHORT_CIRCUIT_STATEMENT, - ZERO_PTR + ZERO_PTR, + FLOAT_CMP_CONST ) } } @@ -334,7 +357,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { return; } } - span_lint_and_then(cx, FLOAT_CMP, expr.span, "strict comparison of f32 or f64", |db| { + let (lint, msg) = if is_named_constant(cx, left) || is_named_constant(cx, right) { + (FLOAT_CMP_CONST, "strict comparison of f32 or f64 constant") + } else { + (FLOAT_CMP, "strict comparison of f32 or f64") + }; + span_lint_and_then(cx, lint, expr.span, msg, |db| { let lhs = Sugg::hir(cx, left, ".."); let rhs = Sugg::hir(cx, right, ".."); @@ -421,6 +449,14 @@ fn check_nan(cx: &LateContext, path: &Path, expr: &Expr) { } } +fn is_named_constant<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> bool { + if let Some((_, res)) = constant(cx, expr) { + res + } else { + false + } +} + fn is_allowed<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> bool { let parent_item = cx.tcx.hir.get_parent(expr.id); let parent_def_id = cx.tcx.hir.local_def_id(parent_item); diff --git a/tests/ui/float_cmp_const.rs b/tests/ui/float_cmp_const.rs new file mode 100644 index 00000000000..12ffb5b3301 --- /dev/null +++ b/tests/ui/float_cmp_const.rs @@ -0,0 +1,31 @@ + + + +#![warn(float_cmp_const)] +#![allow(unused, no_effect, unnecessary_operation)] + +const ONE: f32 = 1.0; +const TWO: f32 = 2.0; + +fn eq_one(x: f32) -> bool { + if x.is_nan() { false } else { x == ONE } // no error, inside "eq" fn +} + +fn main() { + // has errors + 1f32 == ONE; + TWO == ONE; + TWO != ONE; + ONE + ONE == TWO; + 1 as f32 == ONE; + + let v = 0.9; + v == ONE; + v != ONE; + + // no errors, lower than or greater than comparisons + v < ONE; + v > ONE; + v <= ONE; + v >= ONE; +} diff --git a/tests/ui/float_cmp_const.stderr b/tests/ui/float_cmp_const.stderr new file mode 100644 index 00000000000..0bdbb6770dc --- /dev/null +++ b/tests/ui/float_cmp_const.stderr @@ -0,0 +1,85 @@ +error: strict comparison of f32 or f64 constant + --> $DIR/float_cmp_const.rs:16:5 + | +16 | 1f32 == ONE; + | ^^^^^^^^^^^ help: consider comparing them within some error: `(1f32 - ONE).abs() < error` + | + = note: `-D float-cmp-const` implied by `-D warnings` +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:16:5 + | +16 | 1f32 == ONE; + | ^^^^^^^^^^^ + +error: strict comparison of f32 or f64 constant + --> $DIR/float_cmp_const.rs:17:5 + | +17 | TWO == ONE; + | ^^^^^^^^^^ help: consider comparing them within some error: `(TWO - ONE).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:17:5 + | +17 | TWO == ONE; + | ^^^^^^^^^^ + +error: strict comparison of f32 or f64 constant + --> $DIR/float_cmp_const.rs:18:5 + | +18 | TWO != ONE; + | ^^^^^^^^^^ help: consider comparing them within some error: `(TWO - ONE).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:18:5 + | +18 | TWO != ONE; + | ^^^^^^^^^^ + +error: strict comparison of f32 or f64 constant + --> $DIR/float_cmp_const.rs:19:5 + | +19 | ONE + ONE == TWO; + | ^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(ONE + ONE - TWO).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:19:5 + | +19 | ONE + ONE == TWO; + | ^^^^^^^^^^^^^^^^ + +error: strict comparison of f32 or f64 constant + --> $DIR/float_cmp_const.rs:20:5 + | +20 | 1 as f32 == ONE; + | ^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(1 as f32 - ONE).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:20:5 + | +20 | 1 as f32 == ONE; + | ^^^^^^^^^^^^^^^ + +error: strict comparison of f32 or f64 constant + --> $DIR/float_cmp_const.rs:23:5 + | +23 | v == ONE; + | ^^^^^^^^ help: consider comparing them within some error: `(v - ONE).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:23:5 + | +23 | v == ONE; + | ^^^^^^^^ + +error: strict comparison of f32 or f64 constant + --> $DIR/float_cmp_const.rs:24:5 + | +24 | v != ONE; + | ^^^^^^^^ help: consider comparing them within some error: `(v - ONE).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:24:5 + | +24 | v != ONE; + | ^^^^^^^^ +