Auto merge of #111818 - Urgau:uplift_cmp_nan, r=cjgillot

Uplift `clippy::cmp_nan` lint

This PR aims at uplifting the `clippy::cmp_nan` lint into rustc.

## `invalid_nan_comparisons`

~~(deny-by-default)~~ (warn-by-default)

The `invalid_nan_comparisons` lint checks comparison with `f32::NAN` or `f64::NAN` as one of the operand.

### Example

```rust,compile_fail
let a = 2.3f32;
if a == f32::NAN {}
```

### Explanation

NaN does not compare meaningfully to anything – not even itself – so those comparisons are always false.

-----

Mostly followed the instructions for uplifting a clippy lint described here: #99696 (review)

`@rustbot` label: +I-lang-nominated
r? compiler

Author: bors

compiler/rustc_lint/messages.ftl

@@ -314,6 +314,11 @@ lint_invalid_from_utf8_checked = calls to `{$method}` with a invalid literal alw
 lint_invalid_from_utf8_unchecked = calls to `{$method}` with a invalid literal are undefined behavior
     .label = the literal was valid UTF-8 up to the {$valid_up_to} bytes
 
+lint_invalid_nan_comparisons_eq_ne = incorrect NaN comparison, NaN cannot be directly compared to itself
+    .suggestion = use `f32::is_nan()` or `f64::is_nan()` instead
+
+lint_invalid_nan_comparisons_lt_le_gt_ge = incorrect NaN comparison, NaN is not orderable
+
 lint_lintpass_by_hand = implementing `LintPass` by hand
     .help = try using `declare_lint_pass!` or `impl_lint_pass!` instead
 

compiler/rustc_lint/src/lints.rs

@@ -1434,6 +1434,36 @@ pub struct OverflowingLiteral<'a> {
 #[diag(lint_unused_comparisons)]
 pub struct UnusedComparisons;
 
+#[derive(LintDiagnostic)]
+pub enum InvalidNanComparisons {
+    #[diag(lint_invalid_nan_comparisons_eq_ne)]
+    EqNe {
+        #[subdiagnostic]
+        suggestion: InvalidNanComparisonsSuggestion,
+    },
+    #[diag(lint_invalid_nan_comparisons_lt_le_gt_ge)]
+    LtLeGtGe,
+}
+
+#[derive(Subdiagnostic)]
+pub enum InvalidNanComparisonsSuggestion {
+    #[multipart_suggestion(
+        lint_suggestion,
+        style = "verbose",
+        applicability = "machine-applicable"
+    )]
+    Spanful {
+        #[suggestion_part(code = "!")]
+        neg: Option<Span>,
+        #[suggestion_part(code = ".is_nan()")]
+        float: Span,
+        #[suggestion_part(code = "")]
+        nan_plus_binop: Span,
+    },
+    #[help(lint_suggestion)]
+    Spanless,
+}
+
 pub struct ImproperCTypes<'a> {
     pub ty: Ty<'a>,
     pub desc: &'a str,

compiler/rustc_lint/src/types.rs

@@ -2,10 +2,10 @@ use crate::{
     fluent_generated as fluent,
     lints::{
         AtomicOrderingFence, AtomicOrderingLoad, AtomicOrderingStore, ImproperCTypes,
-        InvalidAtomicOrderingDiag, OnlyCastu8ToChar, OverflowingBinHex, OverflowingBinHexSign,
-        OverflowingBinHexSub, OverflowingInt, OverflowingIntHelp, OverflowingLiteral,
-        OverflowingUInt, RangeEndpointOutOfRange, UnusedComparisons, UseInclusiveRange,
-        VariantSizeDifferencesDiag,
+        InvalidAtomicOrderingDiag, InvalidNanComparisons, InvalidNanComparisonsSuggestion,
+        OnlyCastu8ToChar, OverflowingBinHex, OverflowingBinHexSign, OverflowingBinHexSub,
+        OverflowingInt, OverflowingIntHelp, OverflowingLiteral, OverflowingUInt,
+        RangeEndpointOutOfRange, UnusedComparisons, UseInclusiveRange, VariantSizeDifferencesDiag,
     },
 };
 use crate::{LateContext, LateLintPass, LintContext};
@@ -113,13 +113,35 @@ declare_lint! {
     "detects enums with widely varying variant sizes"
 }
 
+declare_lint! {
+    /// The `invalid_nan_comparisons` lint checks comparison with `f32::NAN` or `f64::NAN`
+    /// as one of the operand.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// let a = 2.3f32;
+    /// if a == f32::NAN {}
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// NaN does not compare meaningfully to anything – not
+    /// even itself – so those comparisons are always false.
+    INVALID_NAN_COMPARISONS,
+    Warn,
+    "detects invalid floating point NaN comparisons"
+}
+
 #[derive(Copy, Clone)]
 pub struct TypeLimits {
     /// Id of the last visited negated expression
     negated_expr_id: Option<hir::HirId>,
 }
 
-impl_lint_pass!(TypeLimits => [UNUSED_COMPARISONS, OVERFLOWING_LITERALS]);
+impl_lint_pass!(TypeLimits => [UNUSED_COMPARISONS, OVERFLOWING_LITERALS, INVALID_NAN_COMPARISONS]);
 
 impl TypeLimits {
     pub fn new() -> TypeLimits {
@@ -486,6 +508,68 @@ fn lint_literal<'tcx>(
     }
 }
 
+fn lint_nan<'tcx>(
+    cx: &LateContext<'tcx>,
+    e: &'tcx hir::Expr<'tcx>,
+    binop: hir::BinOp,
+    l: &'tcx hir::Expr<'tcx>,
+    r: &'tcx hir::Expr<'tcx>,
+) {
+    fn is_nan(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool {
+        let expr = expr.peel_blocks().peel_borrows();
+        match expr.kind {
+            ExprKind::Path(qpath) => {
+                let Some(def_id) = cx.typeck_results().qpath_res(&qpath, expr.hir_id).opt_def_id() else { return false; };
+
+                matches!(cx.tcx.get_diagnostic_name(def_id), Some(sym::f32_nan | sym::f64_nan))
+            }
+            _ => false,
+        }
+    }
+
+    fn eq_ne(
+        e: &hir::Expr<'_>,
+        l: &hir::Expr<'_>,
+        r: &hir::Expr<'_>,
+        f: impl FnOnce(Span, Span) -> InvalidNanComparisonsSuggestion,
+    ) -> InvalidNanComparisons {
+        let suggestion =
+            if let Some(l_span) = l.span.find_ancestor_inside(e.span) &&
+                let Some(r_span) = r.span.find_ancestor_inside(e.span) {
+                f(l_span, r_span)
+            } else {
+                InvalidNanComparisonsSuggestion::Spanless
+            };
+
+        InvalidNanComparisons::EqNe { suggestion }
+    }
+
+    let lint = match binop.node {
+        hir::BinOpKind::Eq | hir::BinOpKind::Ne if is_nan(cx, l) => {
+            eq_ne(e, l, r, |l_span, r_span| InvalidNanComparisonsSuggestion::Spanful {
+                nan_plus_binop: l_span.until(r_span),
+                float: r_span.shrink_to_hi(),
+                neg: (binop.node == hir::BinOpKind::Ne).then(|| r_span.shrink_to_lo()),
+            })
+        }
+        hir::BinOpKind::Eq | hir::BinOpKind::Ne if is_nan(cx, r) => {
+            eq_ne(e, l, r, |l_span, r_span| InvalidNanComparisonsSuggestion::Spanful {
+                nan_plus_binop: l_span.shrink_to_hi().to(r_span),
+                float: l_span.shrink_to_hi(),
+                neg: (binop.node == hir::BinOpKind::Ne).then(|| l_span.shrink_to_lo()),
+            })
+        }
+        hir::BinOpKind::Lt | hir::BinOpKind::Le | hir::BinOpKind::Gt | hir::BinOpKind::Ge
+            if is_nan(cx, l) || is_nan(cx, r) =>
+        {
+            InvalidNanComparisons::LtLeGtGe
+        }
+        _ => return,
+    };
+
+    cx.emit_spanned_lint(INVALID_NAN_COMPARISONS, e.span, lint);
+}
+
 impl<'tcx> LateLintPass<'tcx> for TypeLimits {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx hir::Expr<'tcx>) {
         match e.kind {
@@ -496,8 +580,12 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
                 }
             }
             hir::ExprKind::Binary(binop, ref l, ref r) => {
-                if is_comparison(binop) && !check_limits(cx, binop, &l, &r) {
-                    cx.emit_spanned_lint(UNUSED_COMPARISONS, e.span, UnusedComparisons);
+                if is_comparison(binop) {
+                    if !check_limits(cx, binop, &l, &r) {
+                        cx.emit_spanned_lint(UNUSED_COMPARISONS, e.span, UnusedComparisons);
+                    } else {
+                        lint_nan(cx, e, binop, l, r);
+                    }
                 }
             }
             hir::ExprKind::Lit(ref lit) => lint_literal(cx, self, e, lit),

compiler/rustc_span/src/symbol.rs

@@ -701,7 +701,9 @@ symbols! {
         f,
         f16c_target_feature,
         f32,
+        f32_nan,
         f64,
+        f64_nan,
         fabsf32,
         fabsf64,
         fadd_fast,

library/core/src/num/f32.rs

@@ -403,6 +403,7 @@ impl f32 {
     /// and the stability of its representation over Rust versions
     /// and target platforms isn't guaranteed.
     #[stable(feature = "assoc_int_consts", since = "1.43.0")]
+    #[rustc_diagnostic_item = "f32_nan"]
     pub const NAN: f32 = 0.0_f32 / 0.0_f32;
     /// Infinity (∞).
     #[stable(feature = "assoc_int_consts", since = "1.43.0")]

library/core/src/num/f64.rs

@@ -401,6 +401,7 @@ impl f64 {
     /// This constant isn't guaranteed to equal to any specific NaN bitpattern,
     /// and the stability of its representation over Rust versions
     /// and target platforms isn't guaranteed.
+    #[rustc_diagnostic_item = "f64_nan"]
     #[stable(feature = "assoc_int_consts", since = "1.43.0")]
     pub const NAN: f64 = 0.0_f64 / 0.0_f64;
     /// Infinity (∞).

src/tools/clippy/clippy_lints/src/declared_lints.rs

@@ -476,7 +476,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::operators::ARITHMETIC_SIDE_EFFECTS_INFO,
     crate::operators::ASSIGN_OP_PATTERN_INFO,
     crate::operators::BAD_BIT_MASK_INFO,
-    crate::operators::CMP_NAN_INFO,
     crate::operators::CMP_OWNED_INFO,
     crate::operators::DOUBLE_COMPARISONS_INFO,
     crate::operators::DURATION_SUBSEC_INFO,

src/tools/clippy/clippy_lints/src/operators/cmp_nan.rs

@@ -1,7 +1,6 @@
 mod absurd_extreme_comparisons;
 mod assign_op_pattern;
 mod bit_mask;
-mod cmp_nan;
 mod cmp_owned;
 mod double_comparison;
 mod duration_subsec;
@@ -485,31 +484,6 @@ declare_clippy_lint! {
     "integer division may cause loss of precision"
 }
 
-declare_clippy_lint! {
-    /// ### What it does
-    /// Checks for comparisons to NaN.
-    ///
-    /// ### Why is this bad?
-    /// NaN does not compare meaningfully to anything – not
-    /// even itself – so those comparisons are simply wrong.
-    ///
-    /// ### Example
-    /// ```rust
-    /// # let x = 1.0;
-    /// if x == f32::NAN { }
-    /// ```
-    ///
-    /// Use instead:
-    /// ```rust
-    /// # let x = 1.0f32;
-    /// if x.is_nan() { }
-    /// ```
-    #[clippy::version = "pre 1.29.0"]
-    pub CMP_NAN,
-    correctness,
-    "comparisons to `NAN`, which will always return false, probably not intended"
-}
-
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for conversions to owned values just for the sake
@@ -775,7 +749,6 @@ impl_lint_pass!(Operators => [
     FLOAT_EQUALITY_WITHOUT_ABS,
     IDENTITY_OP,
     INTEGER_DIVISION,
-    CMP_NAN,
     CMP_OWNED,
     FLOAT_CMP,
     FLOAT_CMP_CONST,
@@ -816,7 +789,6 @@ impl<'tcx> LateLintPass<'tcx> for Operators {
                 duration_subsec::check(cx, e, op.node, lhs, rhs);
                 float_equality_without_abs::check(cx, e, op.node, lhs, rhs);
                 integer_division::check(cx, e, op.node, lhs, rhs);
-                cmp_nan::check(cx, e, op.node, lhs, rhs);
                 cmp_owned::check(cx, op.node, lhs, rhs);
                 float_cmp::check(cx, e, op.node, lhs, rhs);
                 modulo_one::check(cx, e, op.node, rhs);

src/tools/clippy/clippy_lints/src/operators/mod.rs

@@ -33,6 +33,7 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
     ("clippy::zero_width_space", "clippy::invisible_characters"),
     ("clippy::cast_ref_to_mut", "cast_ref_to_mut"),
     ("clippy::clone_double_ref", "suspicious_double_ref_op"),
+    ("clippy::cmp_nan", "invalid_nan_comparisons"),
     ("clippy::drop_bounds", "drop_bounds"),
     ("clippy::drop_copy", "dropping_copy_types"),
     ("clippy::drop_ref", "dropping_references"),