Fix: inconsistent/incorrect implementations of Ord, PartialOrd, `…

…PartialEq` and `Eq` traits leading to panics on sort (#28)

checker/Cargo.toml

@@ -11,7 +11,7 @@ edition = "2021"
 build = "build.rs"
 
 [lib]
-test = false # we have no unit tests
+test = true # we have some unit tests
 doctest = false # and no doc tests
 
 [[bin]]

checker/src/abstract_value.rs

@@ -6,6 +6,7 @@
 #![allow(clippy::declare_interior_mutable_const)]
 
 use std::cell::RefCell;
+use std::cmp::Ordering;
 use std::collections::HashSet;
 use std::fmt::{Debug, Formatter, Result};
 use std::hash::Hash;
@@ -39,7 +40,7 @@ use crate::tag_domain::{Tag, TagDomain};
 /// When we do know everything about a value, it is concrete rather than
 /// abstract, but is convenient to just use this structure for concrete values
 /// as well, since all operations can be uniform.
-#[derive(Serialize, Deserialize, Clone, Eq, Ord, PartialOrd)]
+#[derive(Serialize, Deserialize, Clone)]
 pub struct AbstractValue {
     // This is not a domain element, but a representation of how this value has been constructed.
     // It is used to refine the value with respect to path conditions and actual arguments.
@@ -87,7 +88,24 @@ impl PartialEq for AbstractValue {
     #[logfn_inputs(TRACE)]
     #[allow(clippy::unconditional_recursion)]
     fn eq(&self, other: &Self) -> bool {
-        self.expression.eq(&other.expression)
+        self.expression == other.expression
+    }
+}
+
+impl Eq for AbstractValue {}
+
+// `Ord` and `PartialOrd` implementations should be consistent with `PartialEq` and `Eq`,
+// so they shouldn't be derived due to manual implementation of `PartialEq` above.
+// Ref: <https://doc.rust-lang.org/nightly/std/cmp/trait.Ord.html#how-can-i-implement-ord>
+impl Ord for AbstractValue {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.expression.cmp(&other.expression)
+    }
+}
+
+impl PartialOrd for AbstractValue {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
     }
 }
 
@@ -7124,3 +7142,39 @@ impl AbstractValueTrait for Rc<AbstractValue> {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // Checks consistency of `Ord`, `PartialOrd`, `PartialEq` and `Eq` implementations for `AbstractValue`.
+    #[test]
+    fn eq_and_ord_consistency_check() {
+        let var_non_null = AbstractValue {
+            expression: Expression::Variable {
+                path: Path::new_computed(TOP.into()),
+                var_type: ExpressionType::NonPrimitive,
+            },
+            expression_size: 1,
+            interval: RefCell::new(None),
+            is_non_null: RefCell::new(Some(true)),
+            tags: RefCell::new(None),
+        };
+        let var_nullable = AbstractValue {
+            expression: Expression::Variable {
+                path: Path::new_computed(TOP.into()),
+                var_type: ExpressionType::NonPrimitive,
+            },
+            expression_size: 1,
+            interval: RefCell::new(None),
+            is_non_null: RefCell::new(None),
+            tags: RefCell::new(None),
+        };
+        assert_eq!(var_non_null, var_nullable);
+        assert_eq!(var_non_null.cmp(&var_nullable), Ordering::Equal);
+        assert_eq!(
+            var_non_null.partial_cmp(&var_nullable),
+            Some(Ordering::Equal)
+        );
+    }
+}

checker/src/constant_domain.rs

@@ -4,18 +4,21 @@
 // LICENSE file in the root directory of this source tree.
 #![allow(clippy::float_cmp)]
 
-use log_derive::{logfn, logfn_inputs};
-use mirai_annotations::*;
 use rustc_hir::def_id::DefId;
 use rustc_middle::ty::{GenericArgsRef, Ty, TyCtxt};
-use serde::{Deserialize, Serialize};
+
 // use std::{f128, f16, f64};
 use std::cmp::Ordering;
 use std::collections::HashMap;
 use std::fmt::{Debug, Formatter, Result};
+use std::hash::Hash;
 use std::rc::Rc;
 use std::{f16, f64};
 
+use log_derive::{logfn, logfn_inputs};
+use mirai_annotations::*;
+use serde::{Deserialize, Serialize};
+
 use crate::expression::{Expression, ExpressionType};
 use crate::known_names::{KnownNames, KnownNamesCache};
 use crate::summaries::SummaryCache;
@@ -113,15 +116,24 @@ pub struct FunctionReference {
     pub argument_type_key: Rc<str>,
 }
 
-impl PartialOrd for FunctionReference {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.cmp(other))
+// `DefId` doesn't/can't implement `Ord` and `PartialOrd`, so they can't be derived.
+// Ref: <https://github.com/rust-lang/rust/blob/eeb90cda1969383f56a2637cbd3037bdf598841c/compiler/rustc_span/src/def_id.rs#L239-L243>
+impl Ord for FunctionReference {
+    fn cmp(&self, other: &Self) -> Ordering {
+        if self == other {
+            // `Ord` (and `PartialOrd`) implementation should be consistent with `PartialEq` and `Eq`.
+            // Ref: <https://doc.rust-lang.org/nightly/std/cmp/trait.Ord.html#how-can-i-implement-ord>
+            return Ordering::Equal;
+        }
+        self.summary_cache_key
+            .cmp(&other.summary_cache_key)
+            .then(self.argument_type_key.cmp(&other.argument_type_key))
     }
 }
 
-impl Ord for FunctionReference {
-    fn cmp(&self, other: &Self) -> Ordering {
-        self.partial_cmp(other).unwrap()
+impl PartialOrd for FunctionReference {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
     }
 }
 

checker/src/path.rs

@@ -3,19 +3,19 @@ use std::cmp::Ordering;
 //
 // This source code is licensed under the MIT license found in the
 // LICENSE file in the root directory of this source tree.
-//
+
+use rustc_hir::def_id::DefId;
+use rustc_middle::ty::{Ty, TyCtxt};
+
 use std::collections::hash_map::DefaultHasher;
 use std::collections::HashSet;
 use std::fmt::{Debug, Formatter, Result};
 use std::hash::{Hash, Hasher};
 use std::rc::Rc;
 
 use log_derive::*;
-use serde::{Deserialize, Serialize};
-
 use mirai_annotations::*;
-use rustc_hir::def_id::DefId;
-use rustc_middle::ty::{Ty, TyCtxt};
+use serde::{Deserialize, Serialize};
 
 use crate::abstract_value::{self, AbstractValue, AbstractValueTrait};
 use crate::constant_domain::ConstantDomain;
@@ -28,7 +28,7 @@ use crate::{k_limits, utils};
 /// to get rehashed. This turns out to be expensive, so for this case we cache the hash to avoid
 /// recomputing it. The caching has a cost, so only use this in cases where it is highly likely
 /// that the path will be hashed more than once.
-#[derive(Serialize, Deserialize, Clone, Eq, Ord, PartialOrd)]
+#[derive(Serialize, Deserialize, Clone)]
 pub struct Path {
     pub value: PathEnum,
     hash: u64,
@@ -52,9 +52,33 @@ impl Hash for Path {
     }
 }
 
+// `Eq` (and `PartialEq`) and `Hash` implementations should be consistent.
+// Ref: <https://doc.rust-lang.org/nightly/std/hash/trait.Hash.html#hash-and-eq>
 impl PartialEq for Path {
     fn eq(&self, other: &Path) -> bool {
-        self.hash == other.hash && self.value == other.value
+        self.hash == other.hash
+    }
+}
+
+impl Eq for Path {}
+
+// `Ord` and `PartialOrd` implementations should be consistent with `PartialEq` and `Eq`,
+// so they shouldn't be derived due to manual implementation of `PartialEq` above.
+// Ref: <https://doc.rust-lang.org/nightly/std/cmp/trait.Ord.html#how-can-i-implement-ord>
+impl Ord for Path {
+    fn cmp(&self, other: &Self) -> Ordering {
+        if self == other {
+            // `Ord` (and `PartialOrd`) implementation should be consistent with `PartialEq` and `Eq`.
+            // Ref: <https://doc.rust-lang.org/nightly/std/cmp/trait.Ord.html#how-can-i-implement-ord>
+            return Ordering::Equal;
+        }
+        self.value.cmp(&other.value)
+    }
+}
+
+impl PartialOrd for Path {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
     }
 }
 
@@ -192,7 +216,12 @@ pub enum PathEnum {
 
 impl PartialOrd for PathEnum {
     #[allow(clippy::non_canonical_partial_ord_impl)]
-    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        if self == other {
+            // `Ord` (and `PartialOrd`) implementation should be consistent with `PartialEq` and `Eq`.
+            // Ref: <https://doc.rust-lang.org/nightly/std/cmp/trait.Ord.html#how-can-i-implement-ord>
+            return Some(Ordering::Equal);
+        }
         use PathEnum::*;
         match (self, other) {
             (Computed { value: lhs }, Computed { value: rhs }) => lhs.partial_cmp(rhs),
@@ -207,12 +236,12 @@ impl PartialOrd for PathEnum {
                     type_index: ri,
                 },
             ) => match l.partial_cmp(r) {
-                Some(std::cmp::Ordering::Equal) => li.partial_cmp(ri),
+                Some(Ordering::Equal) => li.partial_cmp(ri),
                 other => other,
             },
             (Offset { value: lhs }, Offset { value: rhs }) => lhs.partial_cmp(rhs),
             (Parameter { ordinal: l }, Parameter { ordinal: r }) => l.partial_cmp(r),
-            (Result, Result) => Some(std::cmp::Ordering::Equal),
+            (Result, Result) => Some(Ordering::Equal),
             (
                 StaticVariable {
                     summary_cache_key: lk,
@@ -225,10 +254,10 @@ impl PartialOrd for PathEnum {
                     ..
                 },
             ) => match lk.partial_cmp(rk) {
-                Some(std::cmp::Ordering::Equal) => lt.partial_cmp(rt),
+                Some(Ordering::Equal) => lt.partial_cmp(rt),
                 other => other,
             },
-            (PhantomData, PhantomData) => Some(std::cmp::Ordering::Equal),
+            (PhantomData, PhantomData) => Some(Ordering::Equal),
             (PromotedConstant { ordinal: l }, PromotedConstant { ordinal: r }) => l.partial_cmp(r),
             (
                 QualifiedPath {
@@ -242,7 +271,7 @@ impl PartialOrd for PathEnum {
                     ..
                 },
             ) => match lq.partial_cmp(rq) {
-                Some(std::cmp::Ordering::Equal) => ls.partial_cmp(rs),
+                Some(Ordering::Equal) => ls.partial_cmp(rs),
                 other => other,
             },
             (_, _) => None,
@@ -252,7 +281,36 @@ impl PartialOrd for PathEnum {
 
 impl Ord for PathEnum {
     fn cmp(&self, other: &Self) -> Ordering {
-        self.partial_cmp(other).unwrap()
+        self.partial_cmp(other).unwrap_or_else(|| {
+            // See [`path_order_rank`] for details.
+            Ord::cmp(&path_order_rank(self), &path_order_rank(other))
+        })
+    }
+}
+
+/// Assigns [`PathEnum`] variants a (somewhat) arbitrarily rank, based on perceived complexity
+/// (e.g. of refinement).
+///
+/// This function is currently only used to properly implement a strict weak order for [`PathEnum`]
+/// variants as is required by sort implementations in Rust >= 1.81
+/// Ref: <https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html#new-sort-implementations>
+/// Ref: <https://doc.rust-lang.org/nightly/std/cmp/trait.Ord.html>
+///
+/// **NOTE:** Manually sorting by discriminant would require an unsafe transmute,
+/// because [`std::mem::Discriminant`] doesn't implement `Ord`.
+/// Ref: <https://doc.rust-lang.org/std/mem/fn.discriminant.html#accessing-the-numeric-value-of-the-discriminant>
+fn path_order_rank(path: &PathEnum) -> u8 {
+    match path {
+        PathEnum::PhantomData => 0,
+        PathEnum::Result => 1,
+        PathEnum::PromotedConstant { .. } => 2,
+        PathEnum::Parameter { .. } => 3,
+        PathEnum::LocalVariable { .. } => 4,
+        PathEnum::StaticVariable { .. } => 5,
+        PathEnum::QualifiedPath { .. } => 6,
+        PathEnum::Computed { .. } => 7,
+        PathEnum::Offset { .. } => 8,
+        PathEnum::HeapBlock { .. } => 9,
     }
 }