From 0e73e7095ae7aa7ead69c4ba7ba002560ef118fa Mon Sep 17 00:00:00 2001 From: Alan Egerton Date: Sat, 22 Jun 2024 07:11:42 +0100 Subject: [PATCH] Ensure careful consideration is given by impls Added an associated `const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED` to the `StableOrd` trait to ensure that implementors carefully consider whether the trait's contract is upheld, as incorrect implementations can cause miscompilations. --- compiler/rustc_abi/src/lib.rs | 6 ++- .../src/stable_hasher.rs | 50 ++++++++++++++++--- compiler/rustc_hir/src/hir_id.rs | 6 ++- .../src/dep_graph/dep_node.rs | 3 ++ compiler/rustc_session/src/config.rs | 4 +- compiler/rustc_span/src/def_id.rs | 4 +- 6 files changed, 60 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index 5d8c882803d..6aa5ad59966 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -425,11 +425,13 @@ pub struct Size { raw: u64, } -// Ord is implement as just comparing numerical values and numerical values -// are not changed by (de-)serialization. #[cfg(feature = "nightly")] impl StableOrd for Size { const CAN_USE_UNSTABLE_SORT: bool = true; + + // `Ord` is implemented as just comparing numerical values and numerical values + // are not changed by (de-)serialization. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } // This is debug-printed a lot in larger structs, don't waste too much space there diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs index 206146cc60d..a57f5067dd8 100644 --- a/compiler/rustc_data_structures/src/stable_hasher.rs +++ b/compiler/rustc_data_structures/src/stable_hasher.rs @@ -238,15 +238,21 @@ pub trait ToStableHashKey { /// The associated constant `CAN_USE_UNSTABLE_SORT` denotes whether /// unstable sorting can be used for this type. Set to true if and /// only if `a == b` implies `a` and `b` are fully indistinguishable. -/// -/// **Be careful when implementing this trait, as an incorrect -/// implementation can cause miscompilation!** pub trait StableOrd: Ord { const CAN_USE_UNSTABLE_SORT: bool; + + /// Marker to ensure that implementors have carefully considered + /// whether their `Ord` implementation obeys this trait's contract. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: (); } impl StableOrd for &T { const CAN_USE_UNSTABLE_SORT: bool = T::CAN_USE_UNSTABLE_SORT; + + // Ordering of a reference is exactly that of the referent, and since + // the ordering of the referet is stable so must be the ordering of the + // reference. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } /// This is a companion trait to `StableOrd`. Some types like `Symbol` can be @@ -295,6 +301,10 @@ macro_rules! impl_stable_traits_for_trivial_type { impl $crate::stable_hasher::StableOrd for $t { const CAN_USE_UNSTABLE_SORT: bool = true; + + // Encoding and decoding doesn't change the bytes of trivial types + // and `Ord::cmp` depends only on those bytes. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } }; } @@ -332,6 +342,10 @@ impl HashStable for Hash128 { impl StableOrd for Hash128 { const CAN_USE_UNSTABLE_SORT: bool = true; + + // Encoding and decoding doesn't change the bytes of `Hash128` + // and `Ord::cmp` depends only on those bytes. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl HashStable for ! { @@ -397,6 +411,10 @@ impl, T2: HashStable, CTX> HashStable for (T1, T2) impl StableOrd for (T1, T2) { const CAN_USE_UNSTABLE_SORT: bool = T1::CAN_USE_UNSTABLE_SORT && T2::CAN_USE_UNSTABLE_SORT; + + // Ordering of tuples is a pure function of their elements' ordering, and since + // the ordering of each element is stable so must be the ordering of the tuple. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl HashStable for (T1, T2, T3) @@ -416,6 +434,10 @@ where impl StableOrd for (T1, T2, T3) { const CAN_USE_UNSTABLE_SORT: bool = T1::CAN_USE_UNSTABLE_SORT && T2::CAN_USE_UNSTABLE_SORT && T3::CAN_USE_UNSTABLE_SORT; + + // Ordering of tuples is a pure function of their elements' ordering, and since + // the ordering of each element is stable so must be the ordering of the tuple. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl HashStable for (T1, T2, T3, T4) @@ -439,6 +461,10 @@ impl StableOrd for ( && T2::CAN_USE_UNSTABLE_SORT && T3::CAN_USE_UNSTABLE_SORT && T4::CAN_USE_UNSTABLE_SORT; + + // Ordering of tuples is a pure function of their elements' ordering, and since + // the ordering of each element is stable so must be the ordering of the tuple. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl, CTX> HashStable for [T] { @@ -533,6 +559,10 @@ impl HashStable for str { impl StableOrd for &str { const CAN_USE_UNSTABLE_SORT: bool = true; + + // Encoding and decoding doesn't change the bytes of string slices + // and `Ord::cmp` depends only on those bytes. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl HashStable for String { @@ -542,10 +572,12 @@ impl HashStable for String { } } -// String comparison only depends on their contents and the -// contents are not changed by (de-)serialization. impl StableOrd for String { const CAN_USE_UNSTABLE_SORT: bool = true; + + // String comparison only depends on their contents and the + // contents are not changed by (de-)serialization. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl ToStableHashKey for String { @@ -571,9 +603,11 @@ impl HashStable for bool { } } -// sort order of bools is not changed by (de-)serialization. impl StableOrd for bool { const CAN_USE_UNSTABLE_SORT: bool = true; + + // sort order of bools is not changed by (de-)serialization. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl HashStable for Option @@ -591,9 +625,11 @@ where } } -// the Option wrapper does not add instability to comparison. impl StableOrd for Option { const CAN_USE_UNSTABLE_SORT: bool = T::CAN_USE_UNSTABLE_SORT; + + // the Option wrapper does not add instability to comparison. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl HashStable for Result diff --git a/compiler/rustc_hir/src/hir_id.rs b/compiler/rustc_hir/src/hir_id.rs index 1ed84fb0968..c0ca1a8017e 100644 --- a/compiler/rustc_hir/src/hir_id.rs +++ b/compiler/rustc_hir/src/hir_id.rs @@ -165,10 +165,12 @@ impl ItemLocalId { pub const INVALID: ItemLocalId = ItemLocalId::MAX; } -// Ord is implement as just comparing the ItemLocalId's numerical -// values and these are not changed by (de-)serialization. impl StableOrd for ItemLocalId { const CAN_USE_UNSTABLE_SORT: bool = true; + + // `Ord` is implemented as just comparing the ItemLocalId's numerical + // values and these are not changed by (de-)serialization. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } /// The `HirId` corresponding to `CRATE_NODE_ID` and `CRATE_DEF_ID`. diff --git a/compiler/rustc_query_system/src/dep_graph/dep_node.rs b/compiler/rustc_query_system/src/dep_graph/dep_node.rs index 3fb59ad26f6..f2a68e35671 100644 --- a/compiler/rustc_query_system/src/dep_graph/dep_node.rs +++ b/compiler/rustc_query_system/src/dep_graph/dep_node.rs @@ -304,6 +304,9 @@ impl ToStableHashKey for WorkProductId { impl StableOrd for WorkProductId { // Fingerprint can use unstable (just a tuple of `u64`s), so WorkProductId can as well const CAN_USE_UNSTABLE_SORT: bool = true; + + // `WorkProductId` sort order is not affected by (de)serialization. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } // Some types are used a lot. Make sure they don't unintentionally get bigger. diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index d5428df0329..f4fd9dee02e 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -491,9 +491,11 @@ pub enum OutputType { DepInfo, } -// Trivial C-Style enums have a stable sort order across compilation sessions. impl StableOrd for OutputType { const CAN_USE_UNSTABLE_SORT: bool = true; + + // Trivial C-Style enums have a stable sort order across compilation sessions. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } impl ToStableHashKey for OutputType { diff --git a/compiler/rustc_span/src/def_id.rs b/compiler/rustc_span/src/def_id.rs index 4d534ad8007..5456303b36f 100644 --- a/compiler/rustc_span/src/def_id.rs +++ b/compiler/rustc_span/src/def_id.rs @@ -120,9 +120,11 @@ impl Default for DefPathHash { } } -// `DefPathHash` sort order is not affected (de)serialization. impl StableOrd for DefPathHash { const CAN_USE_UNSTABLE_SORT: bool = true; + + // `DefPathHash` sort order is not affected by (de)serialization. + const THIS_IMPLEMENTATION_HAS_BEEN_TRIPLE_CHECKED: () = (); } /// A [`StableCrateId`] is a 64-bit hash of a crate name, together with all