Auto merge of #152086 - nnethercote:stable-hashing-cleanups, r=<try>

bors · bors · commit 1b52388c28bd · 2026-02-04T07:57:38.000Z
Stable hashing cleanups
diff --git a/Cargo.lock b/Cargo.lock
@@ -4594,7 +4594,6 @@ dependencies = [
  "rustc_macros",
  "rustc_metadata",
  "rustc_middle",
- "rustc_query_system",
  "rustc_session",
  "rustc_span",
  "smallvec",
diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs
@@ -607,7 +607,7 @@ where
 /// result (for example, using a `Fingerprint` produced while
 /// hashing `Span`s when a `Fingerprint` without `Span`s is
 /// being requested)
-#[derive(Clone, Hash, Eq, PartialEq, Debug)]
+#[derive(Clone, Copy, Hash, Eq, PartialEq, Debug)]
 pub struct HashingControls {
     pub hash_spans: bool,
 }
diff --git a/compiler/rustc_hir/src/stable_hash_impls.rs b/compiler/rustc_hir/src/stable_hash_impls.rs
@@ -11,9 +11,7 @@ use crate::lints::DelayedLints;
 /// Requirements for a `StableHashingContext` to be used in this crate.
 /// This is a hack to allow using the `HashStable_Generic` derive macro
 /// instead of implementing everything in `rustc_middle`.
-pub trait HashStableContext: rustc_ast::HashStableContext + rustc_abi::HashStableContext {
-    fn hash_attr_id(&mut self, id: &HashIgnoredAttrId, hasher: &mut StableHasher);
-}
+pub trait HashStableContext: rustc_ast::HashStableContext + rustc_abi::HashStableContext {}
 
 impl<HirCtx: crate::HashStableContext> ToStableHashKey<HirCtx> for BodyId {
     type KeyType = (DefPathHash, ItemLocalId);
@@ -104,7 +102,7 @@ impl<HirCtx: crate::HashStableContext> HashStable<HirCtx> for Crate<'_> {
 }
 
 impl<HirCtx: crate::HashStableContext> HashStable<HirCtx> for HashIgnoredAttrId {
-    fn hash_stable(&self, hcx: &mut HirCtx, hasher: &mut StableHasher) {
-        hcx.hash_attr_id(self, hasher)
+    fn hash_stable(&self, _hcx: &mut HirCtx, _hasher: &mut StableHasher) {
+        /* we don't hash HashIgnoredAttrId, we ignore them */
     }
 }
diff --git a/compiler/rustc_middle/src/ty/impls_ty.rs b/compiler/rustc_middle/src/ty/impls_ty.rs
@@ -20,6 +20,9 @@ where
     T: HashStable<StableHashingContext<'a>>,
 {
     fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
+        // Note: this cache makes an *enormous* performance difference on certain benchmarks. E.g.
+        // without it, compiling `diesel-2.2.10` can be 74% slower, and compiling
+        // `deeply-nested-multi` can be ~4,000x slower(!)
         thread_local! {
             static CACHE: RefCell<FxHashMap<(*const (), HashingControls), Fingerprint>> =
                 RefCell::new(Default::default());
diff --git a/compiler/rustc_query_system/src/ich/hcx.rs b/compiler/rustc_query_system/src/ich/hcx.rs
@@ -1,13 +1,20 @@
-use rustc_ast as ast;
+use std::hash::Hash;
+
 use rustc_data_structures::stable_hasher::{HashStable, HashingControls, StableHasher};
 use rustc_hir::def_id::{DefId, LocalDefId};
 use rustc_hir::definitions::DefPathHash;
 use rustc_session::Session;
 use rustc_session::cstore::Untracked;
 use rustc_span::source_map::SourceMap;
-use rustc_span::{BytePos, CachingSourceMapView, DUMMY_SP, SourceFile, Span, SpanData, Symbol};
+use rustc_span::{CachingSourceMapView, DUMMY_SP, Pos, Span};
 
-use crate::ich;
+// Very often, we are hashing something that does not need the `CachingSourceMapView`, so we
+// initialize it lazily.
+#[derive(Clone)]
+enum CachingSourceMap<'a> {
+    Unused(&'a SourceMap),
+    InUse(CachingSourceMapView<'a>),
+}
 
 /// This is the context state available during incr. comp. hashing. It contains
 /// enough information to transform `DefId`s and `HirId`s into stable `DefPath`s (i.e.,
@@ -19,10 +26,7 @@ pub struct StableHashingContext<'a> {
     // The value of `-Z incremental-ignore-spans`.
     // This field should only be used by `unstable_opts_incremental_ignore_span`
     incremental_ignore_spans: bool,
-    // Very often, we are hashing something that does not need the
-    // `CachingSourceMapView`, so we initialize it lazily.
-    raw_source_map: &'a SourceMap,
-    caching_source_map: Option<CachingSourceMapView<'a>>,
+    caching_source_map: CachingSourceMap<'a>,
     hashing_controls: HashingControls,
 }
 
@@ -34,8 +38,7 @@ impl<'a> StableHashingContext<'a> {
         StableHashingContext {
             untracked,
             incremental_ignore_spans: sess.opts.unstable_opts.incremental_ignore_spans,
-            caching_source_map: None,
-            raw_source_map: sess.source_map(),
+            caching_source_map: CachingSourceMap::Unused(sess.source_map()),
             hashing_controls: HashingControls { hash_spans: hash_spans_initial },
         }
     }
@@ -49,81 +52,143 @@ impl<'a> StableHashingContext<'a> {
     }
 
     #[inline]
-    pub fn def_path_hash(&self, def_id: DefId) -> DefPathHash {
-        if let Some(def_id) = def_id.as_local() {
-            self.local_def_path_hash(def_id)
-        } else {
-            self.untracked.cstore.read().def_path_hash(def_id)
-        }
-    }
-
-    #[inline]
-    pub fn local_def_path_hash(&self, def_id: LocalDefId) -> DefPathHash {
-        self.untracked.definitions.read().def_path_hash(def_id)
-    }
-
-    #[inline]
-    pub fn source_map(&mut self) -> &mut CachingSourceMapView<'a> {
+    fn source_map(&mut self) -> &mut CachingSourceMapView<'a> {
         match self.caching_source_map {
-            Some(ref mut sm) => sm,
-            ref mut none => {
-                *none = Some(CachingSourceMapView::new(self.raw_source_map));
-                none.as_mut().unwrap()
+            CachingSourceMap::InUse(ref mut sm) => sm,
+            CachingSourceMap::Unused(sm) => {
+                self.caching_source_map = CachingSourceMap::InUse(CachingSourceMapView::new(sm));
+                self.source_map() // this recursive call will hit the `InUse` case
             }
         }
     }
 
     #[inline]
-    pub fn is_ignored_attr(&self, name: Symbol) -> bool {
-        ich::IGNORED_ATTRIBUTES.contains(&name)
+    fn def_span(&self, def_id: LocalDefId) -> Span {
+        self.untracked.source_span.get(def_id).unwrap_or(DUMMY_SP)
     }
 
     #[inline]
     pub fn hashing_controls(&self) -> HashingControls {
-        self.hashing_controls.clone()
-    }
-}
-
-impl<'a> HashStable<StableHashingContext<'a>> for ast::NodeId {
-    #[inline]
-    fn hash_stable(&self, _: &mut StableHashingContext<'a>, _: &mut StableHasher) {
-        panic!("Node IDs should not appear in incremental state");
+        self.hashing_controls
     }
 }
 
 impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> {
-    #[inline]
-    fn hash_spans(&self) -> bool {
-        self.hashing_controls.hash_spans
-    }
+    /// Hashes a span in a stable way. We can't directly hash the span's `BytePos` fields (that
+    /// would be similar to hashing pointers, since those are just offsets into the `SourceMap`).
+    /// Instead, we hash the (file name, line, column) triple, which stays the same even if the
+    /// containing `SourceFile` has moved within the `SourceMap`.
+    ///
+    /// Also note that we are hashing byte offsets for the column, not unicode codepoint offsets.
+    /// For the purpose of the hash that's sufficient. Also, hashing filenames is expensive so we
+    /// avoid doing it twice when the span starts and ends in the same file, which is almost always
+    /// the case.
+    ///
+    /// IMPORTANT: changes to this method should be reflected in implementations of `SpanEncoder`.
+    fn span_hash_stable(&mut self, span: Span, hasher: &mut StableHasher) {
+        const TAG_VALID_SPAN: u8 = 0;
+        const TAG_INVALID_SPAN: u8 = 1;
+        const TAG_RELATIVE_SPAN: u8 = 2;
+
+        if !self.hashing_controls().hash_spans {
+            return;
+        }
 
-    #[inline]
-    fn unstable_opts_incremental_ignore_spans(&self) -> bool {
-        self.incremental_ignore_spans
-    }
+        let span = span.data_untracked();
+        span.ctxt.hash_stable(self, hasher);
+        span.parent.hash_stable(self, hasher);
 
-    #[inline]
-    fn def_path_hash(&self, def_id: DefId) -> DefPathHash {
-        self.def_path_hash(def_id)
-    }
+        if span.is_dummy() {
+            Hash::hash(&TAG_INVALID_SPAN, hasher);
+            return;
+        }
 
-    #[inline]
-    fn def_span(&self, def_id: LocalDefId) -> Span {
-        self.untracked.source_span.get(def_id).unwrap_or(DUMMY_SP)
+        let parent = span.parent.map(|parent| self.def_span(parent).data_untracked());
+        if let Some(parent) = parent
+            && parent.contains(span)
+        {
+            // This span is enclosed in a definition: only hash the relative position. This catches
+            // a subset of the cases from the `file.contains(parent.lo)`. But we can do this check
+            // cheaply without the expensive `span_data_to_lines_and_cols` query.
+            Hash::hash(&TAG_RELATIVE_SPAN, hasher);
+            (span.lo - parent.lo).to_u32().hash_stable(self, hasher);
+            (span.hi - parent.lo).to_u32().hash_stable(self, hasher);
+            return;
+        }
+
+        // If this is not an empty or invalid span, we want to hash the last position that belongs
+        // to it, as opposed to hashing the first position past it.
+        let Some((file, line_lo, col_lo, line_hi, col_hi)) =
+            self.source_map().span_data_to_lines_and_cols(&span)
+        else {
+            Hash::hash(&TAG_INVALID_SPAN, hasher);
+            return;
+        };
+
+        if let Some(parent) = parent
+            && file.contains(parent.lo)
+        {
+            // This span is relative to another span in the same file,
+            // only hash the relative position.
+            Hash::hash(&TAG_RELATIVE_SPAN, hasher);
+            Hash::hash(&(span.lo.0.wrapping_sub(parent.lo.0)), hasher);
+            Hash::hash(&(span.hi.0.wrapping_sub(parent.lo.0)), hasher);
+            return;
+        }
+
+        Hash::hash(&TAG_VALID_SPAN, hasher);
+        Hash::hash(&file.stable_id, hasher);
+
+        // Hash both the length and the end location (line/column) of a span. If we hash only the
+        // length, for example, then two otherwise equal spans with different end locations will
+        // have the same hash. This can cause a problem during incremental compilation wherein a
+        // previous result for a query that depends on the end location of a span will be
+        // incorrectly reused when the end location of the span it depends on has changed (see
+        // issue #74890). A similar analysis applies if some query depends specifically on the
+        // length of the span, but we only hash the end location. So hash both.
+
+        let col_lo_trunc = (col_lo.0 as u64) & 0xFF;
+        let line_lo_trunc = ((line_lo as u64) & 0xFF_FF_FF) << 8;
+        let col_hi_trunc = (col_hi.0 as u64) & 0xFF << 32;
+        let line_hi_trunc = ((line_hi as u64) & 0xFF_FF_FF) << 40;
+        let col_line = col_lo_trunc | line_lo_trunc | col_hi_trunc | line_hi_trunc;
+        let len = (span.hi - span.lo).0;
+        Hash::hash(&col_line, hasher);
+        Hash::hash(&len, hasher);
     }
 
     #[inline]
-    fn span_data_to_lines_and_cols(
-        &mut self,
-        span: &SpanData,
-    ) -> Option<(&SourceFile, usize, BytePos, usize, BytePos)> {
-        self.source_map().span_data_to_lines_and_cols(span)
+    fn def_path_hash(&self, def_id: DefId) -> DefPathHash {
+        if let Some(def_id) = def_id.as_local() {
+            self.untracked.definitions.read().def_path_hash(def_id)
+        } else {
+            self.untracked.cstore.read().def_path_hash(def_id)
+        }
     }
 
-    #[inline]
-    fn hashing_controls(&self) -> HashingControls {
-        self.hashing_controls.clone()
+    /// Assert that the provided `HashStableContext` is configured with the default
+    /// `HashingControls`. We should always have bailed out before getting to here with a
+    /// non-default mode. With this check in place, we can avoid the need to maintain separate
+    /// versions of `ExpnData` hashes for each permutation of `HashingControls` settings.
+    fn assert_default_hashing_controls(&self, msg: &str) {
+        let hashing_controls = self.hashing_controls;
+        let HashingControls { hash_spans } = hashing_controls;
+
+        // Note that we require that `hash_spans` be the inverse of the global `-Z
+        // incremental-ignore-spans` option. Normally, this option is disabled, in which case
+        // `hash_spans` must be true.
+        //
+        // Span hashing can also be disabled without `-Z incremental-ignore-spans`. This is the
+        // case for instance when building a hash for name mangling. Such configuration must not be
+        // used for metadata.
+        assert_eq!(
+            hash_spans, !self.incremental_ignore_spans,
+            "Attempted hashing of {msg} with non-default HashingControls: {hashing_controls:?}"
+        );
     }
 }
 
+impl<'a> rustc_abi::HashStableContext for StableHashingContext<'a> {}
+impl<'a> rustc_ast::HashStableContext for StableHashingContext<'a> {}
+impl<'a> rustc_hir::HashStableContext for StableHashingContext<'a> {}
 impl<'a> rustc_session::HashStableContext for StableHashingContext<'a> {}
diff --git a/compiler/rustc_query_system/src/ich/impls_syntax.rs b/compiler/rustc_query_system/src/ich/impls_syntax.rs
@@ -2,14 +2,18 @@
 //! from various crates in no particular order.
 
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
-use rustc_hir::{self as hir, HashIgnoredAttrId};
-use rustc_span::SourceFile;
+use rustc_span::{SourceFile, Symbol, sym};
 use smallvec::SmallVec;
+use {rustc_ast as ast, rustc_hir as hir};
 
 use crate::ich::StableHashingContext;
 
-impl<'ctx> rustc_abi::HashStableContext for StableHashingContext<'ctx> {}
-impl<'ctx> rustc_ast::HashStableContext for StableHashingContext<'ctx> {}
+impl<'a> HashStable<StableHashingContext<'a>> for ast::NodeId {
+    #[inline]
+    fn hash_stable(&self, _: &mut StableHashingContext<'a>, _: &mut StableHasher) {
+        panic!("Node IDs should not appear in incremental state");
+    }
+}
 
 impl<'a> HashStable<StableHashingContext<'a>> for [hir::Attribute] {
     fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
@@ -24,7 +28,7 @@ impl<'a> HashStable<StableHashingContext<'a>> for [hir::Attribute] {
             .filter(|attr| {
                 attr.is_doc_comment().is_none()
                     // FIXME(jdonszelmann) have a better way to handle ignored attrs
-                    && !attr.name().is_some_and(|ident| hcx.is_ignored_attr(ident))
+                    && !attr.name().is_some_and(|ident| is_ignored_attr(ident))
             })
             .collect();
 
@@ -35,10 +39,19 @@ impl<'a> HashStable<StableHashingContext<'a>> for [hir::Attribute] {
     }
 }
 
-impl<'ctx> rustc_hir::HashStableContext for StableHashingContext<'ctx> {
-    fn hash_attr_id(&mut self, _id: &HashIgnoredAttrId, _hasher: &mut StableHasher) {
-        /* we don't hash HashIgnoredAttrId, we ignore them */
-    }
+#[inline]
+fn is_ignored_attr(name: Symbol) -> bool {
+    const IGNORED_ATTRIBUTES: &[Symbol] = &[
+        sym::cfg_trace, // FIXME should this really be ignored?
+        sym::rustc_if_this_changed,
+        sym::rustc_then_this_would_need,
+        sym::rustc_dirty,
+        sym::rustc_clean,
+        sym::rustc_partition_reused,
+        sym::rustc_partition_codegened,
+        sym::rustc_expected_cgu_reuse,
+    ];
+    IGNORED_ATTRIBUTES.contains(&name)
 }
 
 impl<'a> HashStable<StableHashingContext<'a>> for SourceFile {
diff --git a/compiler/rustc_query_system/src/ich/mod.rs b/compiler/rustc_query_system/src/ich/mod.rs
@@ -1,19 +1,6 @@
 //! ICH - Incremental Compilation Hash
 
-use rustc_span::{Symbol, sym};
-
 pub use self::hcx::StableHashingContext;
 
 mod hcx;
 mod impls_syntax;
-
-pub const IGNORED_ATTRIBUTES: &[Symbol] = &[
-    sym::cfg_trace, // FIXME should this really be ignored?
-    sym::rustc_if_this_changed,
-    sym::rustc_then_this_would_need,
-    sym::rustc_dirty,
-    sym::rustc_clean,
-    sym::rustc_partition_reused,
-    sym::rustc_partition_codegened,
-    sym::rustc_expected_cgu_reuse,
-];
diff --git a/compiler/rustc_resolve/Cargo.toml b/compiler/rustc_resolve/Cargo.toml
@@ -22,7 +22,6 @@ rustc_index = { path = "../rustc_index" }
 rustc_macros = { path = "../rustc_macros" }
 rustc_metadata = { path = "../rustc_metadata" }
 rustc_middle = { path = "../rustc_middle" }
-rustc_query_system = { path = "../rustc_query_system" }
 rustc_session = { path = "../rustc_session" }
 rustc_span = { path = "../rustc_span" }
 smallvec = { version = "1.8.1", features = ["union", "may_dangle"] }
diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs
@@ -72,7 +72,6 @@ use rustc_middle::ty::{
     self, DelegationFnSig, DelegationInfo, Feed, MainDefinition, RegisteredTools,
     ResolverAstLowering, ResolverGlobalCtxt, TyCtxt, TyCtxtFeed, Visibility,
 };
-use rustc_query_system::ich::StableHashingContext;
 use rustc_session::config::CrateType;
 use rustc_session::lint::builtin::PRIVATE_MACRO_USE;
 use rustc_span::hygiene::{ExpnId, LocalExpnId, MacroKind, SyntaxContext, Transparency};
@@ -1841,10 +1840,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         ResolverOutputs { global_ctxt, ast_lowering }
     }
 
-    fn create_stable_hashing_context(&self) -> StableHashingContext<'_> {
-        StableHashingContext::new(self.tcx.sess, self.tcx.untracked())
-    }
-
     fn cstore(&self) -> FreezeReadGuard<'_, CStore> {
         CStore::from_tcx(self.tcx)
     }
diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs
diff --git a/compiler/rustc_span/src/hygiene.rs b/compiler/rustc_span/src/hygiene.rs
diff --git a/compiler/rustc_span/src/lib.rs b/compiler/rustc_span/src/lib.rs

Original file line number	Diff line number	Diff line change
`@@ -607,7 +607,7 @@ where`
`607`	`607`	/// result (for example, using a `Fingerprint` produced while
`608`	`608`	/// hashing `Span`s when a `Fingerprint` without `Span`s is
`609`	`609`	`/// being requested)`
`610`		`-#[derive(Clone, Hash, Eq, PartialEq, Debug)]`
	`610`	`+#[derive(Clone, Copy, Hash, Eq, PartialEq, Debug)]`
`611`	`611`	`pub struct HashingControls {`
`612`	`612`	`pub hash_spans: bool,`
`613`	`613`	`}`
Original file line number	Diff line number	Diff line change
`@@ -11,9 +11,7 @@ use crate::lints::DelayedLints;`
`11`	`11`	/// Requirements for a `StableHashingContext` to be used in this crate.
`12`	`12`	/// This is a hack to allow using the `HashStable_Generic` derive macro
`13`	`13`	/// instead of implementing everything in `rustc_middle`.
`14`		`-pub trait HashStableContext: rustc_ast::HashStableContext + rustc_abi::HashStableContext {`
`15`		`- fn hash_attr_id(&mut self, id: &HashIgnoredAttrId, hasher: &mut StableHasher);`
`16`		`-}`
	`14`	`+pub trait HashStableContext: rustc_ast::HashStableContext + rustc_abi::HashStableContext {}`
`17`	`15`
`18`	`16`	`impl<HirCtx: crate::HashStableContext> ToStableHashKey<HirCtx> for BodyId {`
`19`	`17`	`type KeyType = (DefPathHash, ItemLocalId);`
`@@ -104,7 +102,7 @@ impl<HirCtx: crate::HashStableContext> HashStable<HirCtx> for Crate<'_> {`
`104`	`102`	`}`
`105`	`103`
`106`	`104`	`impl<HirCtx: crate::HashStableContext> HashStable<HirCtx> for HashIgnoredAttrId {`
`107`		`- fn hash_stable(&self, hcx: &mut HirCtx, hasher: &mut StableHasher) {`
`108`		`- hcx.hash_attr_id(self, hasher)`
	`105`	`+ fn hash_stable(&self, _hcx: &mut HirCtx, _hasher: &mut StableHasher) {`
	`106`	`+ /* we don't hash HashIgnoredAttrId, we ignore them */`
`109`	`107`	`}`
`110`	`108`	`}`