From 2787a60fc23accc81c9f9c7350b62c73f1c368a4 Mon Sep 17 00:00:00 2001 From: clippered Date: Sat, 4 Nov 2017 19:32:58 +1100 Subject: [PATCH 1/4] 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 5549c98aaf35..eea590f033fe 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 18d7f7230a82..dfe1187916f1 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 000000000000..12ffb5b3301a --- /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 000000000000..0bdbb6770dc9 --- /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; + | ^^^^^^^^ + From cd3106d99fd95390a870622e22eef0cedbefaac9 Mon Sep 17 00:00:00 2001 From: clippered Date: Mon, 6 Nov 2017 20:02:42 +1100 Subject: [PATCH 2/4] add more negative tests --- tests/ui/float_cmp_const.rs | 13 +++++++++ tests/ui/float_cmp_const.stderr | 49 +++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/tests/ui/float_cmp_const.rs b/tests/ui/float_cmp_const.rs index 12ffb5b3301a..fe69eeeed0a2 100644 --- a/tests/ui/float_cmp_const.rs +++ b/tests/ui/float_cmp_const.rs @@ -28,4 +28,17 @@ fn main() { v > ONE; v <= ONE; v >= ONE; + + // has float_cmp warns (as expected), no float constants + let w = 1.1; + v == w; + v != w; + v == 1.0; + v != 1.0; + + // no errors, zero and infinity values + ONE != 0f32; + TWO == 0f32; + ONE != ::std::f32::INFINITY; + ONE == ::std::f32::NEG_INFINITY; } diff --git a/tests/ui/float_cmp_const.stderr b/tests/ui/float_cmp_const.stderr index 0bdbb6770dc9..f8933938ed8b 100644 --- a/tests/ui/float_cmp_const.stderr +++ b/tests/ui/float_cmp_const.stderr @@ -83,3 +83,52 @@ note: std::f32::EPSILON and std::f64::EPSILON are available. 24 | v != ONE; | ^^^^^^^^ +error: strict comparison of f32 or f64 + --> $DIR/float_cmp_const.rs:34:5 + | +34 | v == w; + | ^^^^^^ help: consider comparing them within some error: `(v - w).abs() < error` + | + = note: `-D float-cmp` implied by `-D warnings` +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:34:5 + | +34 | v == w; + | ^^^^^^ + +error: strict comparison of f32 or f64 + --> $DIR/float_cmp_const.rs:35:5 + | +35 | v != w; + | ^^^^^^ help: consider comparing them within some error: `(v - w).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:35:5 + | +35 | v != w; + | ^^^^^^ + +error: strict comparison of f32 or f64 + --> $DIR/float_cmp_const.rs:36:5 + | +36 | v == 1.0; + | ^^^^^^^^ help: consider comparing them within some error: `(v - 1.0).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:36:5 + | +36 | v == 1.0; + | ^^^^^^^^ + +error: strict comparison of f32 or f64 + --> $DIR/float_cmp_const.rs:37:5 + | +37 | v != 1.0; + | ^^^^^^^^ help: consider comparing them within some error: `(v - 1.0).abs() < error` + | +note: std::f32::EPSILON and std::f64::EPSILON are available. + --> $DIR/float_cmp_const.rs:37:5 + | +37 | v != 1.0; + | ^^^^^^^^ + From ddaf8580d58b859701a94695368120023ba0ef34 Mon Sep 17 00:00:00 2001 From: clippered Date: Mon, 6 Nov 2017 20:23:18 +1100 Subject: [PATCH 3/4] remove duplicate tests with float_cmp --- tests/ui/float_cmp_const.rs | 10 ++----- tests/ui/float_cmp_const.stderr | 49 --------------------------------- 2 files changed, 3 insertions(+), 56 deletions(-) diff --git a/tests/ui/float_cmp_const.rs b/tests/ui/float_cmp_const.rs index fe69eeeed0a2..990a83730681 100644 --- a/tests/ui/float_cmp_const.rs +++ b/tests/ui/float_cmp_const.rs @@ -29,16 +29,12 @@ fn main() { v <= ONE; v >= ONE; - // has float_cmp warns (as expected), no float constants - let w = 1.1; - v == w; - v != w; - v == 1.0; - v != 1.0; - // no errors, zero and infinity values ONE != 0f32; TWO == 0f32; ONE != ::std::f32::INFINITY; ONE == ::std::f32::NEG_INFINITY; + + // Note: float_cmp will warn as expected on cases where there are no float constants + // e.g. v == 1.0 } diff --git a/tests/ui/float_cmp_const.stderr b/tests/ui/float_cmp_const.stderr index f8933938ed8b..0bdbb6770dc9 100644 --- a/tests/ui/float_cmp_const.stderr +++ b/tests/ui/float_cmp_const.stderr @@ -83,52 +83,3 @@ note: std::f32::EPSILON and std::f64::EPSILON are available. 24 | v != ONE; | ^^^^^^^^ -error: strict comparison of f32 or f64 - --> $DIR/float_cmp_const.rs:34:5 - | -34 | v == w; - | ^^^^^^ help: consider comparing them within some error: `(v - w).abs() < error` - | - = note: `-D float-cmp` implied by `-D warnings` -note: std::f32::EPSILON and std::f64::EPSILON are available. - --> $DIR/float_cmp_const.rs:34:5 - | -34 | v == w; - | ^^^^^^ - -error: strict comparison of f32 or f64 - --> $DIR/float_cmp_const.rs:35:5 - | -35 | v != w; - | ^^^^^^ help: consider comparing them within some error: `(v - w).abs() < error` - | -note: std::f32::EPSILON and std::f64::EPSILON are available. - --> $DIR/float_cmp_const.rs:35:5 - | -35 | v != w; - | ^^^^^^ - -error: strict comparison of f32 or f64 - --> $DIR/float_cmp_const.rs:36:5 - | -36 | v == 1.0; - | ^^^^^^^^ help: consider comparing them within some error: `(v - 1.0).abs() < error` - | -note: std::f32::EPSILON and std::f64::EPSILON are available. - --> $DIR/float_cmp_const.rs:36:5 - | -36 | v == 1.0; - | ^^^^^^^^ - -error: strict comparison of f32 or f64 - --> $DIR/float_cmp_const.rs:37:5 - | -37 | v != 1.0; - | ^^^^^^^^ help: consider comparing them within some error: `(v - 1.0).abs() < error` - | -note: std::f32::EPSILON and std::f64::EPSILON are available. - --> $DIR/float_cmp_const.rs:37:5 - | -37 | v != 1.0; - | ^^^^^^^^ - From 66bc12564a5c7494cb17ac816255ac87b6f4016b Mon Sep 17 00:00:00 2001 From: clippered Date: Mon, 6 Nov 2017 21:34:30 +1100 Subject: [PATCH 4/4] put back negative tests but allow float_cmp --- tests/ui/float_cmp_const.rs | 9 ++++- tests/ui/float_cmp_const.stderr | 66 ++++++++++++++++----------------- 2 files changed, 40 insertions(+), 35 deletions(-) diff --git a/tests/ui/float_cmp_const.rs b/tests/ui/float_cmp_const.rs index 990a83730681..adf2ab70368a 100644 --- a/tests/ui/float_cmp_const.rs +++ b/tests/ui/float_cmp_const.rs @@ -2,6 +2,7 @@ #![warn(float_cmp_const)] +#![allow(float_cmp)] #![allow(unused, no_effect, unnecessary_operation)] const ONE: f32 = 1.0; @@ -35,6 +36,10 @@ fn main() { ONE != ::std::f32::INFINITY; ONE == ::std::f32::NEG_INFINITY; - // Note: float_cmp will warn as expected on cases where there are no float constants - // e.g. v == 1.0 + // no errors, but will warn float_cmp if '#![allow(float_cmp)]' above is removed + let w = 1.1; + v == w; + v != w; + v == 1.0; + v != 1.0; } diff --git a/tests/ui/float_cmp_const.stderr b/tests/ui/float_cmp_const.stderr index 0bdbb6770dc9..fe277de28dd5 100644 --- a/tests/ui/float_cmp_const.stderr +++ b/tests/ui/float_cmp_const.stderr @@ -1,85 +1,85 @@ error: strict comparison of f32 or f64 constant - --> $DIR/float_cmp_const.rs:16:5 + --> $DIR/float_cmp_const.rs:17:5 | -16 | 1f32 == ONE; +17 | 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 + --> $DIR/float_cmp_const.rs:17:5 | -16 | 1f32 == ONE; +17 | 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; +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; +18 | TWO == ONE; | ^^^^^^^^^^ error: strict comparison of f32 or f64 constant --> $DIR/float_cmp_const.rs:19:5 | -19 | ONE + ONE == TWO; +19 | 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:19:5 + | +19 | TWO != ONE; + | ^^^^^^^^^^ + +error: strict comparison of f32 or f64 constant + --> $DIR/float_cmp_const.rs:20:5 + | +20 | 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 + --> $DIR/float_cmp_const.rs:20:5 | -19 | ONE + ONE == TWO; +20 | ONE + ONE == TWO; | ^^^^^^^^^^^^^^^^ error: strict comparison of f32 or f64 constant - --> $DIR/float_cmp_const.rs:20:5 + --> $DIR/float_cmp_const.rs:21:5 | -20 | 1 as f32 == ONE; +21 | 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 + --> $DIR/float_cmp_const.rs:21:5 | -20 | 1 as f32 == ONE; +21 | 1 as f32 == ONE; | ^^^^^^^^^^^^^^^ error: strict comparison of f32 or f64 constant - --> $DIR/float_cmp_const.rs:23:5 + --> $DIR/float_cmp_const.rs:24:5 | -23 | v == ONE; +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:23:5 + --> $DIR/float_cmp_const.rs:24:5 | -23 | v == ONE; +24 | v == ONE; | ^^^^^^^^ error: strict comparison of f32 or f64 constant - --> $DIR/float_cmp_const.rs:24:5 + --> $DIR/float_cmp_const.rs:25:5 | -24 | v != ONE; +25 | 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 + --> $DIR/float_cmp_const.rs:25:5 | -24 | v != ONE; +25 | v != ONE; | ^^^^^^^^