New lint: non_canonical_total_float_cmp

Author: nik-rev

CHANGELOG.md

@@ -6947,6 +6947,7 @@ Released 2018-09-13
 [`non_ascii_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_ascii_literal
 [`non_canonical_clone_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
 [`non_canonical_partial_ord_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_partial_ord_impl
+[`non_canonical_total_float_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_total_float_cmp
 [`non_minimal_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_minimal_cfg
 [`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions
 [`non_send_fields_in_send_ty`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_send_fields_in_send_ty

clippy_lints/src/declared_lints.rs

@@ -443,6 +443,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::methods::NEEDLESS_SPLITN_INFO,
     crate::methods::NEW_RET_NO_SELF_INFO,
     crate::methods::NO_EFFECT_REPLACE_INFO,
+    crate::methods::NON_CANONICAL_TOTAL_FLOAT_CMP_INFO,
     crate::methods::NONSENSICAL_OPEN_OPTIONS_INFO,
     crate::methods::OBFUSCATED_IF_ELSE_INFO,
     crate::methods::OK_EXPECT_INFO,

clippy_lints/src/methods/mod.rs

@@ -85,6 +85,7 @@ mod needless_option_as_deref;
 mod needless_option_take;
 mod new_ret_no_self;
 mod no_effect_replace;
+mod non_canonical_float_cmp;
 mod obfuscated_if_else;
 mod ok_expect;
 mod open_options;
@@ -2708,6 +2709,72 @@ declare_clippy_lint! {
     "replace with no effect"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Checks for manual attempts to force a total ordering on floating-point numbers by
+    /// using `.partial_cmp(..).unwrap_or(Ordering)`.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// Floating-point numbers do not have a total order by default because of `NaN` values
+    /// and the existence of both `-0.0` and `0.0`. When developers use `.unwrap_or(..)`
+    /// with `partial_cmp`, they are usually trying to implement a total order manually,
+    /// likely because they do not know about the existence of `total_cmp`.
+    ///
+    /// This is problematic because:
+    ///
+    /// - Inconsistency: Manual unwrapping often fails to handle the nuances of IEEE 754
+    ///   total ordering (e.g., the sign of `NaN` or the difference between `-0.0` and `0.0`).
+    ///
+    /// - Broken invariants: In contexts like sorting or searching (e.g., `BTreeMap`),
+    ///   an inconsistent ordering function can lead to non-deterministic results,
+    ///   infinite loops, or panics.
+    ///
+    /// - Readability: `total_cmp` is the canonical, standard library method for this
+    ///   exact purpose, making the code more idiomatic and making the intent clear.
+    ///
+    /// While sorting is the most common place this pattern appears, it also affects
+    /// any logic relying on a stable comparison, such as finding a minimum/maximum
+    /// value in a collection.
+    ///
+    /// ### Known problems
+    ///
+    /// - Performance: On some architectures (like x86), `total_cmp` *may* be
+    ///   slightly slower than hardware-accelerated float comparisons if the values
+    ///   are already in floating-point registers.
+    ///
+    /// - Semantic Change: `total_cmp` considers `-0.0` to be strictly less than `0.0`.
+    ///   If your logic relies on them being equal (the default behavior of `==`),
+    ///   this lint's suggestion will change your program's behavior.
+    ///
+    /// ### Example
+    ///
+    /// ```rs
+    /// use std::cmp;
+    ///
+    /// let x = 0.0;
+    /// let y = -0.0;
+    /// let is_less = x.partial_cmp(&y).unwrap_or(cmp::Ordering::Greater);
+    ///
+    /// vec.sort_by(|a, b| a.partial_cmp(b).unwrap_or(cmp::Ordering::Equal));
+    /// ```
+    ///
+    /// Use instead:
+    ///
+    /// ```rs
+    /// let x = 0.0;
+    /// let y = -0.0;
+    /// let is_less = x.total_cmp(&y);
+    ///
+    /// vec.sort_by(|a, b| a.total_cmp(b));
+    /// ```
+    #[clippy::version = "1.97.0"]
+    pub NON_CANONICAL_TOTAL_FLOAT_CMP,
+    suspicious,
+    "non-canonical total float comparison which is likely a mistake"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for duplicate open options as well as combinations
@@ -4867,6 +4934,7 @@ impl_lint_pass!(Methods => [
     NEEDLESS_SPLITN,
     NEW_RET_NO_SELF,
     NONSENSICAL_OPEN_OPTIONS,
+    NON_CANONICAL_TOTAL_FLOAT_CMP,
     NO_EFFECT_REPLACE,
     OBFUSCATED_IF_ELSE,
     OK_EXPECT,
@@ -5685,6 +5753,9 @@ impl Methods {
                         Some((sym::map, m_recv, [m_arg], span, _)) => {
                             map_unwrap_or::check(cx, expr, m_recv, m_arg, recv, u_arg, span, self.msrv);
                         },
+                        Some((sym::partial_cmp, m_recv, [m_arg], _, _)) => {
+                            non_canonical_float_cmp::check(cx, expr, m_recv, m_arg, u_arg);
+                        },
                         Some((then_method @ (sym::then | sym::then_some), t_recv, [t_arg], _, _)) => {
                             obfuscated_if_else::check(
                                 cx,

clippy_lints/src/methods/non_canonical_float_cmp.rs

@@ -0,0 +1,62 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::{expr_or_init, paths};
+use rustc_errors::Applicability;
+use rustc_lint::LateContext;
+
+use crate::methods::NON_CANONICAL_TOTAL_FLOAT_CMP;
+
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    full_expr: &rustc_hir::Expr<'_>,
+    partial_cmp_recv: &rustc_hir::Expr<'_>,
+    partial_cmp_arg: &rustc_hir::Expr<'_>,
+    unwrap_or_arg: &rustc_hir::Expr<'_>,
+) {
+    // format: <float>.partial_cmp(<float>).unwrap_or(<Ordering>)
+
+    if !cx
+        .typeck_results()
+        .expr_ty(expr_or_init(cx, partial_cmp_recv))
+        .peel_refs()
+        .is_floating_point()
+        || !cx
+            .typeck_results()
+            .expr_ty(expr_or_init(cx, partial_cmp_arg))
+            .peel_refs()
+            .is_floating_point()
+    {
+        return;
+    }
+
+    let ordering_variants = [
+        &paths::CMP_ORDERING_EQUAL,
+        &paths::CMP_ORDERING_GREATER,
+        &paths::CMP_ORDERING_LESS,
+    ];
+
+    let is_cmp_ordering_variant = ordering_variants
+        .iter()
+        .any(|variant| variant.matches_path(cx, expr_or_init(cx, unwrap_or_arg)));
+
+    if !is_cmp_ordering_variant {
+        return;
+    }
+
+    let mut applicability = Applicability::MaybeIncorrect;
+
+    let partial_cmp_recv =
+        clippy_utils::source::snippet_with_applicability(cx.tcx.sess, partial_cmp_recv.span, "..", &mut applicability);
+
+    let partial_cmp_arg =
+        clippy_utils::source::snippet_with_applicability(cx.tcx.sess, partial_cmp_arg.span, "..", &mut applicability);
+
+    span_lint_and_sugg(
+        cx,
+        NON_CANONICAL_TOTAL_FLOAT_CMP,
+        full_expr.span,
+        "non-canonical total float comparison",
+        "try",
+        format!("{partial_cmp_recv}.total_cmp({partial_cmp_arg})"),
+        applicability,
+    );
+}

clippy_utils/src/paths.rs

@@ -135,6 +135,10 @@ path_macros! {
 
 // Paths in the standard library missing a diagnostic item
 
+pub static CMP_ORDERING_LESS: PathLookup = value_path!(core::cmp::Ordering::Less);
+pub static CMP_ORDERING_EQUAL: PathLookup = value_path!(core::cmp::Ordering::Equal);
+pub static CMP_ORDERING_GREATER: PathLookup = value_path!(core::cmp::Ordering::Greater);
+
 // Paths in external crates
 pub static FUTURES_IO_ASYNCREADEXT: PathLookup = type_path!(futures_util::AsyncReadExt);
 pub static FUTURES_IO_ASYNCWRITEEXT: PathLookup = type_path!(futures_util::AsyncWriteExt);

clippy_utils/src/sym.rs

@@ -66,6 +66,7 @@ generate! {
     FileType,
     FsOpenOptions,
     FsPermissions,
+    Greater,
     HashMapEntry,
     Instant,
     IntoIter,
@@ -84,6 +85,7 @@ generate! {
     LF: "\n",
     LateContext,
     Lazy,
+    Less,
     LinkedList,
     Lint,
     LowerExp,

tests/ui/non_canonical_total_float_cmp.fixed

@@ -0,0 +1,63 @@
+//@aux-build:proc_macros.rs
+#![feature(f16)]
+#![feature(f128)]
+#![warn(clippy::non_canonical_total_float_cmp)]
+use std::cmp;
+use std::cmp::Ordering;
+use std::cmp::Ordering::{Equal, Greater, Less};
+
+extern crate proc_macros;
+
+fn example() {
+    93f32.total_cmp(&83f32);
+    //~^ non_canonical_total_float_cmp
+
+    93.0f16.total_cmp(&83.0);
+    //~^ non_canonical_total_float_cmp
+
+    93.0f128.total_cmp(&83.0);
+    //~^ non_canonical_total_float_cmp
+
+    93.0f64.total_cmp(&83.0);
+    //~^ non_canonical_total_float_cmp
+
+    93.0_f32.total_cmp(&83.0);
+    //~^ non_canonical_total_float_cmp
+
+    93f64.total_cmp(&83f64);
+    //~^ non_canonical_total_float_cmp
+
+    let x = cmp::Ordering::Less;
+    93f32.total_cmp(&83f32);
+    //~^ non_canonical_total_float_cmp
+
+    let x: f32 = 0.0;
+    x.total_cmp(&83f32);
+    //~^ non_canonical_total_float_cmp
+
+    let x: f32 = 0.0;
+    93f32.total_cmp(&x);
+    //~^ non_canonical_total_float_cmp
+
+    //
+    // Tests for tokens that come from expansion.
+    //
+
+    // no lint: can't change to "total_cmp"
+    proc_macros::external!(93f32.partial_cmp(&83f32).unwrap_or(Equal));
+
+    // Ideally, this would lint, but that requires a bigger chance to the
+    // way that method lints are handled.
+    proc_macros::external!(93f32).partial_cmp(&83f32).unwrap_or(Equal);
+
+    // Ideally, this would lint, but that requires a bigger chance to the
+    // way that method lints are handled.
+    93f32.partial_cmp(proc_macros::external!(&83f32)).unwrap_or(Equal);
+
+    // no lint: can't change to "total_cmp"
+    proc_macros::external!(93f32.partial_cmp(&83f32)).unwrap_or(Equal);
+
+    // Ideally, this would lint, but that requires a bigger chance to the
+    // way that method lints are handled.
+    93f32.partial_cmp(&83f32).unwrap_or(proc_macros::external!(Equal));
+}

tests/ui/non_canonical_total_float_cmp.rs

@@ -0,0 +1,63 @@
+//@aux-build:proc_macros.rs
+#![feature(f16)]
+#![feature(f128)]
+#![warn(clippy::non_canonical_total_float_cmp)]
+use std::cmp;
+use std::cmp::Ordering;
+use std::cmp::Ordering::{Equal, Greater, Less};
+
+extern crate proc_macros;
+
+fn example() {
+    93f32.partial_cmp(&83f32).unwrap_or(Ordering::Less);
+    //~^ non_canonical_total_float_cmp
+
+    93.0f16.partial_cmp(&83.0).unwrap_or(Ordering::Greater);
+    //~^ non_canonical_total_float_cmp
+
+    93.0f128.partial_cmp(&83.0).unwrap_or(Ordering::Equal);
+    //~^ non_canonical_total_float_cmp
+
+    93.0f64.partial_cmp(&83.0).unwrap_or(Less);
+    //~^ non_canonical_total_float_cmp
+
+    93.0_f32.partial_cmp(&83.0).unwrap_or(Greater);
+    //~^ non_canonical_total_float_cmp
+
+    93f64.partial_cmp(&83f64).unwrap_or(Equal);
+    //~^ non_canonical_total_float_cmp
+
+    let x = cmp::Ordering::Less;
+    93f32.partial_cmp(&83f32).unwrap_or(x);
+    //~^ non_canonical_total_float_cmp
+
+    let x: f32 = 0.0;
+    x.partial_cmp(&83f32).unwrap_or(core::cmp::Ordering::Greater);
+    //~^ non_canonical_total_float_cmp
+
+    let x: f32 = 0.0;
+    93f32.partial_cmp(&x).unwrap_or(std::cmp::Ordering::Equal);
+    //~^ non_canonical_total_float_cmp
+
+    //
+    // Tests for tokens that come from expansion.
+    //
+
+    // no lint: can't change to "total_cmp"
+    proc_macros::external!(93f32.partial_cmp(&83f32).unwrap_or(Equal));
+
+    // Ideally, this would lint, but that requires a bigger chance to the
+    // way that method lints are handled.
+    proc_macros::external!(93f32).partial_cmp(&83f32).unwrap_or(Equal);
+
+    // Ideally, this would lint, but that requires a bigger chance to the
+    // way that method lints are handled.
+    93f32.partial_cmp(proc_macros::external!(&83f32)).unwrap_or(Equal);
+
+    // no lint: can't change to "total_cmp"
+    proc_macros::external!(93f32.partial_cmp(&83f32)).unwrap_or(Equal);
+
+    // Ideally, this would lint, but that requires a bigger chance to the
+    // way that method lints are handled.
+    93f32.partial_cmp(&83f32).unwrap_or(proc_macros::external!(Equal));
+}

tests/ui/non_canonical_total_float_cmp.stderr

@@ -0,0 +1,59 @@
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:12:5
+   |
+LL |     93f32.partial_cmp(&83f32).unwrap_or(Ordering::Less);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `93f32.total_cmp(&83f32)`
+   |
+   = note: `-D clippy::non-canonical-total-float-cmp` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::non_canonical_total_float_cmp)]`
+
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:15:5
+   |
+LL |     93.0f16.partial_cmp(&83.0).unwrap_or(Ordering::Greater);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `93.0f16.total_cmp(&83.0)`
+
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:18:5
+   |
+LL |     93.0f128.partial_cmp(&83.0).unwrap_or(Ordering::Equal);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `93.0f128.total_cmp(&83.0)`
+
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:21:5
+   |
+LL |     93.0f64.partial_cmp(&83.0).unwrap_or(Less);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `93.0f64.total_cmp(&83.0)`
+
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:24:5
+   |
+LL |     93.0_f32.partial_cmp(&83.0).unwrap_or(Greater);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `93.0_f32.total_cmp(&83.0)`
+
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:27:5
+   |
+LL |     93f64.partial_cmp(&83f64).unwrap_or(Equal);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `93f64.total_cmp(&83f64)`
+
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:31:5
+   |
+LL |     93f32.partial_cmp(&83f32).unwrap_or(x);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `93f32.total_cmp(&83f32)`
+
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:35:5
+   |
+LL |     x.partial_cmp(&83f32).unwrap_or(core::cmp::Ordering::Greater);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.total_cmp(&83f32)`
+
+error: non-canonical total float comparison
+  --> tests/ui/non_canonical_total_float_cmp.rs:39:5
+   |
+LL |     93f32.partial_cmp(&x).unwrap_or(std::cmp::Ordering::Equal);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `93f32.total_cmp(&x)`
+
+error: aborting due to 9 previous errors
+