From c08be1dffdf1d6c3e3435f78493fef1081a38625 Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Fri, 3 Jan 2025 00:58:48 +0300 Subject: [PATCH] Fix: inconsistent/incorrect implementations of `Ord`, `PartialOrd`, `PartialEq` and `Eq` traits leading to panics on sort --- checker/Cargo.toml | 2 +- checker/src/abstract_value.rs | 58 ++++++++++++++++++++++- checker/src/constant_domain.rs | 30 ++++++++---- checker/src/path.rs | 86 ++++++++++++++++++++++++++++------ 4 files changed, 150 insertions(+), 26 deletions(-) diff --git a/checker/Cargo.toml b/checker/Cargo.toml index 30a2c409..0fba458b 100644 --- a/checker/Cargo.toml +++ b/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]] diff --git a/checker/src/abstract_value.rs b/checker/src/abstract_value.rs index e021f214..9ee0d149 100644 --- a/checker/src/abstract_value.rs +++ b/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: +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 { + Some(self.cmp(other)) } } @@ -7124,3 +7142,39 @@ impl AbstractValueTrait for Rc { } } } + +#[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) + ); + } +} diff --git a/checker/src/constant_domain.rs b/checker/src/constant_domain.rs index c9c3d123..5942348d 100644 --- a/checker/src/constant_domain.rs +++ b/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, } -impl PartialOrd for FunctionReference { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) +// `DefId` doesn't/can't implement `Ord` and `PartialOrd`, so they can't be derived. +// Ref: +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: + 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 { + Some(self.cmp(other)) } } diff --git a/checker/src/path.rs b/checker/src/path.rs index e586016e..3e5d5132 100644 --- a/checker/src/path.rs +++ b/checker/src/path.rs @@ -3,7 +3,10 @@ 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}; @@ -11,11 +14,8 @@ 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: 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: +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: + return Ordering::Equal; + } + self.value.cmp(&other.value) + } +} + +impl PartialOrd for Path { + fn partial_cmp(&self, other: &Self) -> Option { + 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 { + fn partial_cmp(&self, other: &Self) -> Option { + if self == other { + // `Ord` (and `PartialOrd`) implementation should be consistent with `PartialEq` and `Eq`. + // Ref: + 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: +/// Ref: +/// +/// **NOTE:** Manually sorting by discriminant would require an unsafe transmute, +/// because [`std::mem::Discriminant`] doesn't implement `Ord`. +/// Ref: +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, } }