From 3a58309798e39ab48e4cb85c1013ad7e7410ace8 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Fri, 2 Dec 2022 15:14:49 +0100 Subject: [PATCH 1/2] Add StableOrd trait as proposed in MCP 533. The StableOrd trait can be used to mark types as having a stable sort order across compilation sessions. Collections that sort their items in a stable way can safely implement HashStable by hashing items in sort order. --- compiler/rustc_abi/src/lib.rs | 7 ++ .../rustc_data_structures/src/fingerprint.rs | 2 +- .../rustc_data_structures/src/sorted_map.rs | 4 +- .../src/sorted_map/index_map.rs | 9 +- .../src/stable_hasher.rs | 99 +++++++++++++------ compiler/rustc_hir/src/hir_id.rs | 6 +- compiler/rustc_session/src/config.rs | 5 +- 7 files changed, 96 insertions(+), 36 deletions(-) diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index 85693259cd015..e14c9ea9a5d1a 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -9,6 +9,8 @@ use std::ops::{Add, AddAssign, Mul, RangeInclusive, Sub}; use std::str::FromStr; use bitflags::bitflags; +#[cfg(feature = "nightly")] +use rustc_data_structures::stable_hasher::StableOrd; use rustc_index::vec::{Idx, IndexVec}; #[cfg(feature = "nightly")] use rustc_macros::HashStable_Generic; @@ -403,6 +405,11 @@ pub struct Size { raw: u64, } +// Safety: Ord is implement as just comparing numerical values and numerical values +// are not changed by (de-)serialization. +#[cfg(feature = "nightly")] +unsafe impl StableOrd for Size {} + // This is debug-printed a lot in larger structs, don't waste too much space there impl fmt::Debug for Size { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { diff --git a/compiler/rustc_data_structures/src/fingerprint.rs b/compiler/rustc_data_structures/src/fingerprint.rs index a39178016ce20..6f0b9bff90a7f 100644 --- a/compiler/rustc_data_structures/src/fingerprint.rs +++ b/compiler/rustc_data_structures/src/fingerprint.rs @@ -140,7 +140,7 @@ impl stable_hasher::StableHasherResult for Fingerprint { } } -impl_stable_hash_via_hash!(Fingerprint); +impl_stable_ord_and_stable_hash_via_hash!(Fingerprint); impl Encodable for Fingerprint { #[inline] diff --git a/compiler/rustc_data_structures/src/sorted_map.rs b/compiler/rustc_data_structures/src/sorted_map.rs index d607a5c8314e1..d13313dfd0ebf 100644 --- a/compiler/rustc_data_structures/src/sorted_map.rs +++ b/compiler/rustc_data_structures/src/sorted_map.rs @@ -1,4 +1,4 @@ -use crate::stable_hasher::{HashStable, StableHasher}; +use crate::stable_hasher::{HashStable, StableHasher, StableOrd}; use std::borrow::Borrow; use std::cmp::Ordering; use std::iter::FromIterator; @@ -308,7 +308,7 @@ impl FromIterator<(K, V)> for SortedMap { } } -impl, V: HashStable, CTX> HashStable for SortedMap { +impl + StableOrd, V: HashStable, CTX> HashStable for SortedMap { #[inline] fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) { self.data.hash_stable(ctx, hasher); diff --git a/compiler/rustc_data_structures/src/sorted_map/index_map.rs b/compiler/rustc_data_structures/src/sorted_map/index_map.rs index 0ec32dc43070d..c2f0ae328962b 100644 --- a/compiler/rustc_data_structures/src/sorted_map/index_map.rs +++ b/compiler/rustc_data_structures/src/sorted_map/index_map.rs @@ -120,13 +120,20 @@ where self.items.hash(hasher) } } + impl HashStable for SortedIndexMultiMap where K: HashStable, V: HashStable, { fn hash_stable(&self, ctx: &mut C, hasher: &mut StableHasher) { - self.items.hash_stable(ctx, hasher) + let SortedIndexMultiMap { + items, + // We can ignore this field because it is not observable from the outside. + idx_sorted_by_item_key: _, + } = self; + + items.hash_stable(ctx, hasher) } } diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs index e2c33e7e06268..d43b74c2740ce 100644 --- a/compiler/rustc_data_structures/src/stable_hasher.rs +++ b/compiler/rustc_data_structures/src/stable_hasher.rs @@ -219,7 +219,35 @@ pub trait ToStableHashKey { fn to_stable_hash_key(&self, hcx: &HCX) -> Self::KeyType; } -/// Implement HashStable by just calling `Hash::hash()`. +/// Trait for marking a type as having a sort order that is +/// stable across compilation session boundaries. More formally: +/// +/// ```txt +/// Ord::cmp(a1, b1) == Ord:cmp(a2, b2) +/// where a2 = decode(encode(a1, context1), context2) +/// b2 = decode(encode(b1, context1), context2) +/// ``` +/// +/// i.e. the result of `Ord::cmp` is not influenced by encoding +/// the values in one session and then decoding them in another +/// session. +/// +/// This is trivially true for types where encoding and decoding +/// don't change the bytes of the values that are used during +/// comparison and comparison only depends on these bytes (as +/// opposed to some non-local state). Examples are u32, String, +/// Path, etc. +/// +/// But it is not true for: +/// - `*const T` and `*mut T` because the values of these pointers +/// will change between sessions. +/// - `DefIndex`, `CrateNum`, `LocalDefId`, because their concrete +/// values depend on state that might be different between +/// compilation sessions. +pub unsafe trait StableOrd: Ord {} + +/// Implement HashStable by just calling `Hash::hash()`. Also implement `StableOrd` for the type since +/// that has the same requirements. /// /// **WARNING** This is only valid for types that *really* don't need any context for fingerprinting. /// But it is easy to misuse this macro (see [#96013](https://github.com/rust-lang/rust/issues/96013) @@ -227,7 +255,7 @@ pub trait ToStableHashKey { /// here in this module. /// /// Use `#[derive(HashStable_Generic)]` instead. -macro_rules! impl_stable_hash_via_hash { +macro_rules! impl_stable_ord_and_stable_hash_via_hash { ($t:ty) => { impl $crate::stable_hasher::HashStable for $t { #[inline] @@ -235,26 +263,28 @@ macro_rules! impl_stable_hash_via_hash { ::std::hash::Hash::hash(self, hasher); } } + + unsafe impl $crate::stable_hasher::StableOrd for $t {} }; } -impl_stable_hash_via_hash!(i8); -impl_stable_hash_via_hash!(i16); -impl_stable_hash_via_hash!(i32); -impl_stable_hash_via_hash!(i64); -impl_stable_hash_via_hash!(isize); +impl_stable_ord_and_stable_hash_via_hash!(i8); +impl_stable_ord_and_stable_hash_via_hash!(i16); +impl_stable_ord_and_stable_hash_via_hash!(i32); +impl_stable_ord_and_stable_hash_via_hash!(i64); +impl_stable_ord_and_stable_hash_via_hash!(isize); -impl_stable_hash_via_hash!(u8); -impl_stable_hash_via_hash!(u16); -impl_stable_hash_via_hash!(u32); -impl_stable_hash_via_hash!(u64); -impl_stable_hash_via_hash!(usize); +impl_stable_ord_and_stable_hash_via_hash!(u8); +impl_stable_ord_and_stable_hash_via_hash!(u16); +impl_stable_ord_and_stable_hash_via_hash!(u32); +impl_stable_ord_and_stable_hash_via_hash!(u64); +impl_stable_ord_and_stable_hash_via_hash!(usize); -impl_stable_hash_via_hash!(u128); -impl_stable_hash_via_hash!(i128); +impl_stable_ord_and_stable_hash_via_hash!(u128); +impl_stable_ord_and_stable_hash_via_hash!(i128); -impl_stable_hash_via_hash!(char); -impl_stable_hash_via_hash!(()); +impl_stable_ord_and_stable_hash_via_hash!(char); +impl_stable_ord_and_stable_hash_via_hash!(()); impl HashStable for ! { fn hash_stable(&self, _ctx: &mut CTX, _hasher: &mut StableHasher) { @@ -444,6 +474,10 @@ impl HashStable for String { } } +// Safety: String comparison only depends on their contents and the +// contents are not changed by (de-)serialization. +unsafe impl StableOrd for String {} + impl ToStableHashKey for String { type KeyType = String; #[inline] @@ -459,6 +493,9 @@ impl HashStable for bool { } } +// Safety: sort order of bools is not changed by (de-)serialization. +unsafe impl StableOrd for bool {} + impl HashStable for Option where T: HashStable, @@ -474,6 +511,9 @@ where } } +// Safety: the Option wrapper does not add instability to comparison. +unsafe impl StableOrd for Option {} + impl HashStable for Result where T1: HashStable, @@ -550,8 +590,8 @@ where } } -impl_stable_hash_via_hash!(::std::path::Path); -impl_stable_hash_via_hash!(::std::path::PathBuf); +impl_stable_ord_and_stable_hash_via_hash!(::std::path::Path); +impl_stable_ord_and_stable_hash_via_hash!(::std::path::PathBuf); impl HashStable for ::std::collections::HashMap where @@ -584,27 +624,26 @@ where impl HashStable for ::std::collections::BTreeMap where - K: ToStableHashKey, + K: HashStable + StableOrd, V: HashStable, { fn hash_stable(&self, hcx: &mut HCX, hasher: &mut StableHasher) { - stable_hash_reduce(hcx, hasher, self.iter(), self.len(), |hasher, hcx, (key, value)| { - let key = key.to_stable_hash_key(hcx); - key.hash_stable(hcx, hasher); - value.hash_stable(hcx, hasher); - }); + self.len().hash_stable(hcx, hasher); + for entry in self.iter() { + entry.hash_stable(hcx, hasher); + } } } -impl HashStable for ::std::collections::BTreeSet +impl HashStable for ::std::collections::BTreeSet where - K: ToStableHashKey, + K: HashStable + StableOrd, { fn hash_stable(&self, hcx: &mut HCX, hasher: &mut StableHasher) { - stable_hash_reduce(hcx, hasher, self.iter(), self.len(), |hasher, hcx, key| { - let key = key.to_stable_hash_key(hcx); - key.hash_stable(hcx, hasher); - }); + self.len().hash_stable(hcx, hasher); + for entry in self.iter() { + entry.hash_stable(hcx, hasher); + } } } diff --git a/compiler/rustc_hir/src/hir_id.rs b/compiler/rustc_hir/src/hir_id.rs index 33f02a115ef38..9f7e79596e943 100644 --- a/compiler/rustc_hir/src/hir_id.rs +++ b/compiler/rustc_hir/src/hir_id.rs @@ -1,5 +1,5 @@ use crate::def_id::{DefId, DefIndex, LocalDefId, CRATE_DEF_ID}; -use rustc_data_structures::stable_hasher::{HashStable, StableHasher, ToStableHashKey}; +use rustc_data_structures::stable_hasher::{HashStable, StableHasher, StableOrd, ToStableHashKey}; use rustc_span::{def_id::DefPathHash, HashStableContext}; use std::fmt; @@ -146,6 +146,10 @@ impl ItemLocalId { pub const INVALID: ItemLocalId = ItemLocalId::MAX; } +// Safety: Ord is implement as just comparing the LocalItemId's numerical +// values and these are not changed by (de-)serialization. +unsafe impl StableOrd for ItemLocalId {} + /// The `HirId` corresponding to `CRATE_NODE_ID` and `CRATE_DEF_ID`. pub const CRATE_HIR_ID: HirId = HirId { owner: OwnerId { def_id: CRATE_DEF_ID }, local_id: ItemLocalId::from_u32(0) }; diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 927810351e958..5dcd53e8e5166 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -10,7 +10,7 @@ use crate::{lint, HashStableContext}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; -use rustc_data_structures::stable_hasher::ToStableHashKey; +use rustc_data_structures::stable_hasher::{StableOrd, ToStableHashKey}; use rustc_target::abi::Align; use rustc_target::spec::{PanicStrategy, SanitizerSet, SplitDebuginfo}; use rustc_target::spec::{Target, TargetTriple, TargetWarnings, TARGETS}; @@ -288,6 +288,9 @@ pub enum OutputType { DepInfo, } +// Safety: Trivial C-Style enums have a stable sort order across compilation sessions. +unsafe impl StableOrd for OutputType {} + impl ToStableHashKey for OutputType { type KeyType = Self; From 56aacb245ce484994ac2e0ecab72b477ad6dc362 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 5 Dec 2022 10:45:31 +0100 Subject: [PATCH 2/2] StableOrd: Address review comments. --- .../rustc_data_structures/src/fingerprint.rs | 2 +- .../src/stable_hasher.rs | 36 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_data_structures/src/fingerprint.rs b/compiler/rustc_data_structures/src/fingerprint.rs index 6f0b9bff90a7f..d98f4e43fe884 100644 --- a/compiler/rustc_data_structures/src/fingerprint.rs +++ b/compiler/rustc_data_structures/src/fingerprint.rs @@ -140,7 +140,7 @@ impl stable_hasher::StableHasherResult for Fingerprint { } } -impl_stable_ord_and_stable_hash_via_hash!(Fingerprint); +impl_stable_traits_for_trivial_type!(Fingerprint); impl Encodable for Fingerprint { #[inline] diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs index d43b74c2740ce..be70d30eeaf08 100644 --- a/compiler/rustc_data_structures/src/stable_hasher.rs +++ b/compiler/rustc_data_structures/src/stable_hasher.rs @@ -255,7 +255,7 @@ pub unsafe trait StableOrd: Ord {} /// here in this module. /// /// Use `#[derive(HashStable_Generic)]` instead. -macro_rules! impl_stable_ord_and_stable_hash_via_hash { +macro_rules! impl_stable_traits_for_trivial_type { ($t:ty) => { impl $crate::stable_hasher::HashStable for $t { #[inline] @@ -268,23 +268,23 @@ macro_rules! impl_stable_ord_and_stable_hash_via_hash { }; } -impl_stable_ord_and_stable_hash_via_hash!(i8); -impl_stable_ord_and_stable_hash_via_hash!(i16); -impl_stable_ord_and_stable_hash_via_hash!(i32); -impl_stable_ord_and_stable_hash_via_hash!(i64); -impl_stable_ord_and_stable_hash_via_hash!(isize); +impl_stable_traits_for_trivial_type!(i8); +impl_stable_traits_for_trivial_type!(i16); +impl_stable_traits_for_trivial_type!(i32); +impl_stable_traits_for_trivial_type!(i64); +impl_stable_traits_for_trivial_type!(isize); -impl_stable_ord_and_stable_hash_via_hash!(u8); -impl_stable_ord_and_stable_hash_via_hash!(u16); -impl_stable_ord_and_stable_hash_via_hash!(u32); -impl_stable_ord_and_stable_hash_via_hash!(u64); -impl_stable_ord_and_stable_hash_via_hash!(usize); +impl_stable_traits_for_trivial_type!(u8); +impl_stable_traits_for_trivial_type!(u16); +impl_stable_traits_for_trivial_type!(u32); +impl_stable_traits_for_trivial_type!(u64); +impl_stable_traits_for_trivial_type!(usize); -impl_stable_ord_and_stable_hash_via_hash!(u128); -impl_stable_ord_and_stable_hash_via_hash!(i128); +impl_stable_traits_for_trivial_type!(u128); +impl_stable_traits_for_trivial_type!(i128); -impl_stable_ord_and_stable_hash_via_hash!(char); -impl_stable_ord_and_stable_hash_via_hash!(()); +impl_stable_traits_for_trivial_type!(char); +impl_stable_traits_for_trivial_type!(()); impl HashStable for ! { fn hash_stable(&self, _ctx: &mut CTX, _hasher: &mut StableHasher) { @@ -590,8 +590,8 @@ where } } -impl_stable_ord_and_stable_hash_via_hash!(::std::path::Path); -impl_stable_ord_and_stable_hash_via_hash!(::std::path::PathBuf); +impl_stable_traits_for_trivial_type!(::std::path::Path); +impl_stable_traits_for_trivial_type!(::std::path::PathBuf); impl HashStable for ::std::collections::HashMap where @@ -635,7 +635,7 @@ where } } -impl HashStable for ::std::collections::BTreeSet +impl HashStable for ::std::collections::BTreeSet where K: HashStable + StableOrd, {