From eb0408ea65d9e93884269fec502bac1966b1565f Mon Sep 17 00:00:00 2001 From: Krishna Veera Reddy Date: Tue, 17 Dec 2019 18:51:30 -0800 Subject: [PATCH 1/2] Detect comparisons with NAN constants Currently `cmp_nan` lint doesn't detect comparisons with NaN's if the operands are consts variables so to fix this we evaluate the const variables first before testing for NaN. --- clippy_lints/src/misc.rs | 34 +++++++------- tests/ui/cmp_nan.rs | 15 ++++++ tests/ui/cmp_nan.stderr | 98 ++++++++++++++++++++++++++++++++++------ 3 files changed, 117 insertions(+), 30 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 07a9d4b33..434acfe66 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -343,12 +343,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints { ExprKind::Binary(ref cmp, ref left, ref right) => { let op = cmp.node; if op.is_comparison() { - if let ExprKind::Path(QPath::Resolved(_, ref path)) = left.kind { - check_nan(cx, path, expr); - } - if let ExprKind::Path(QPath::Resolved(_, ref path)) = right.kind { - check_nan(cx, path, expr); - } + check_nan(cx, left, expr.span); + check_nan(cx, right, expr.span); check_to_owned(cx, left, right); check_to_owned(cx, right, left); } @@ -444,17 +440,21 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints { } } -fn check_nan(cx: &LateContext<'_, '_>, path: &Path, expr: &Expr) { - if !in_constant(cx, expr.hir_id) { - if let Some(seg) = path.segments.last() { - if seg.ident.name == sym!(NAN) { - span_lint( - cx, - CMP_NAN, - expr.span, - "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead", - ); - } +fn check_nan(cx: &LateContext<'_, '_>, expr: &Expr, cmp_span: Span) { + if let Some((value, _)) = constant(cx, cx.tables, expr) { + let needs_lint = match value { + Constant::F32(num) => num.is_nan(), + Constant::F64(num) => num.is_nan(), + _ => false, + }; + + if needs_lint { + span_lint( + cx, + CMP_NAN, + cmp_span, + "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead", + ); } } } diff --git a/tests/ui/cmp_nan.rs b/tests/ui/cmp_nan.rs index 33e039308..f89ccddbf 100644 --- a/tests/ui/cmp_nan.rs +++ b/tests/ui/cmp_nan.rs @@ -1,3 +1,6 @@ +const NAN_F32: f32 = std::f32::NAN; +const NAN_F64: f64 = std::f64::NAN; + #[warn(clippy::cmp_nan)] #[allow(clippy::float_cmp, clippy::no_effect, clippy::unnecessary_operation)] fn main() { @@ -8,6 +11,12 @@ fn main() { x > std::f32::NAN; x <= std::f32::NAN; x >= std::f32::NAN; + x == NAN_F32; + x != NAN_F32; + x < NAN_F32; + x > NAN_F32; + x <= NAN_F32; + x >= NAN_F32; let y = 0f64; y == std::f64::NAN; @@ -16,4 +25,10 @@ fn main() { y > std::f64::NAN; y <= std::f64::NAN; y >= std::f64::NAN; + y == NAN_F64; + y != NAN_F64; + y < NAN_F64; + y > NAN_F64; + y <= NAN_F64; + y >= NAN_F64; } diff --git a/tests/ui/cmp_nan.stderr b/tests/ui/cmp_nan.stderr index 421f34518..4ec92716a 100644 --- a/tests/ui/cmp_nan.stderr +++ b/tests/ui/cmp_nan.stderr @@ -1,5 +1,5 @@ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:5:5 + --> $DIR/cmp_nan.rs:8:5 | LL | x == std::f32::NAN; | ^^^^^^^^^^^^^^^^^^ @@ -7,70 +7,142 @@ LL | x == std::f32::NAN; = note: `-D clippy::cmp-nan` implied by `-D warnings` error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:6:5 + --> $DIR/cmp_nan.rs:9:5 | LL | x != std::f32::NAN; | ^^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:7:5 + --> $DIR/cmp_nan.rs:10:5 | LL | x < std::f32::NAN; | ^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:8:5 + --> $DIR/cmp_nan.rs:11:5 | LL | x > std::f32::NAN; | ^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:9:5 + --> $DIR/cmp_nan.rs:12:5 | LL | x <= std::f32::NAN; | ^^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:10:5 + --> $DIR/cmp_nan.rs:13:5 | LL | x >= std::f32::NAN; | ^^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:13:5 + --> $DIR/cmp_nan.rs:14:5 + | +LL | x == NAN_F32; + | ^^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:15:5 + | +LL | x != NAN_F32; + | ^^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:16:5 + | +LL | x < NAN_F32; + | ^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:17:5 + | +LL | x > NAN_F32; + | ^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:18:5 + | +LL | x <= NAN_F32; + | ^^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:19:5 + | +LL | x >= NAN_F32; + | ^^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:22:5 | LL | y == std::f64::NAN; | ^^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:14:5 + --> $DIR/cmp_nan.rs:23:5 | LL | y != std::f64::NAN; | ^^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:15:5 + --> $DIR/cmp_nan.rs:24:5 | LL | y < std::f64::NAN; | ^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:16:5 + --> $DIR/cmp_nan.rs:25:5 | LL | y > std::f64::NAN; | ^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:17:5 + --> $DIR/cmp_nan.rs:26:5 | LL | y <= std::f64::NAN; | ^^^^^^^^^^^^^^^^^^ error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead - --> $DIR/cmp_nan.rs:18:5 + --> $DIR/cmp_nan.rs:27:5 | LL | y >= std::f64::NAN; | ^^^^^^^^^^^^^^^^^^ -error: aborting due to 12 previous errors +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:28:5 + | +LL | y == NAN_F64; + | ^^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:29:5 + | +LL | y != NAN_F64; + | ^^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:30:5 + | +LL | y < NAN_F64; + | ^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:31:5 + | +LL | y > NAN_F64; + | ^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:32:5 + | +LL | y <= NAN_F64; + | ^^^^^^^^^^^^ + +error: doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead + --> $DIR/cmp_nan.rs:33:5 + | +LL | y >= NAN_F64; + | ^^^^^^^^^^^^ + +error: aborting due to 24 previous errors From 460d5a3b5a0c657bce8d00661b1546e78cafe2b1 Mon Sep 17 00:00:00 2001 From: Krishna Veera Reddy Date: Tue, 17 Dec 2019 19:18:42 -0800 Subject: [PATCH 2/2] Prevent `cmp_nan` when inside constants `std::{f32,f64}::is_nan` isn't a const fn so prevent `cmp_nan` lint from running within constant comparisons. --- clippy_lints/src/misc.rs | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 434acfe66..687776c67 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -343,8 +343,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints { ExprKind::Binary(ref cmp, ref left, ref right) => { let op = cmp.node; if op.is_comparison() { - check_nan(cx, left, expr.span); - check_nan(cx, right, expr.span); + check_nan(cx, left, expr); + check_nan(cx, right, expr); check_to_owned(cx, left, right); check_to_owned(cx, right, left); } @@ -440,21 +440,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints { } } -fn check_nan(cx: &LateContext<'_, '_>, expr: &Expr, cmp_span: Span) { - if let Some((value, _)) = constant(cx, cx.tables, expr) { - let needs_lint = match value { - Constant::F32(num) => num.is_nan(), - Constant::F64(num) => num.is_nan(), - _ => false, - }; +fn check_nan(cx: &LateContext<'_, '_>, expr: &Expr, cmp_expr: &Expr) { + if_chain! { + if !in_constant(cx, cmp_expr.hir_id); + if let Some((value, _)) = constant(cx, cx.tables, expr); + then { + let needs_lint = match value { + Constant::F32(num) => num.is_nan(), + Constant::F64(num) => num.is_nan(), + _ => false, + }; - if needs_lint { - span_lint( - cx, - CMP_NAN, - cmp_span, - "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead", - ); + if needs_lint { + span_lint( + cx, + CMP_NAN, + cmp_expr.span, + "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead", + ); + } } } }