Skip to content

Commit 39219ce

Browse files
borsbors
committed
Auto merge of #152318 - nnethercote:streamline-HashStableContext, r=cjgillot
Streamline `rustc_span::HashStableContext`. Currently this trait has five methods. But it only really needs three. For example, currently stable hashing of spans is implemented in `rustc_span`, except a couple of sub-operations are delegated to `rustc_query_system`: `def_span` and `span_data_to_lines_and_cols`. These two delegated sub-operations can be reduced to a single delegated operation that does the full hash computation. Likewise, `assert_default_hashing_controls` depends on two delegated sub-operations, `hashing_controls` and `unstable_opts_incremental_ignore_spans`, and can be simplified. I find the resulting code simpler and clearer -- when necessary, we do a whole operation in `rustc_query_system` instead of doing it partly in `rustc_span` and partly in `rustc_query_system`. r? @cjgillot
2 parents 1c316d3 + 0f31083 commit 39219ce

3 files changed

Lines changed: 121 additions & 136 deletions

File tree

‎compiler/rustc_query_system/src/ich/hcx.rs‎

Lines changed: 108 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
1-
use rustc_data_structures::stable_hasher::HashingControls;
1+
use std::hash::Hash;
2+
3+
use rustc_data_structures::stable_hasher::{HashStable, HashingControls, StableHasher};
24
use rustc_hir::def_id::{DefId, LocalDefId};
35
use rustc_hir::definitions::DefPathHash;
46
use rustc_session::Session;
57
use rustc_session::cstore::Untracked;
68
use rustc_span::source_map::SourceMap;
7-
use rustc_span::{BytePos, CachingSourceMapView, DUMMY_SP, SourceFile, Span, SpanData};
9+
use rustc_span::{CachingSourceMapView, DUMMY_SP, Pos, Span};
810

911
// Very often, we are hashing something that does not need the `CachingSourceMapView`, so we
1012
// initialize it lazily.
@@ -60,16 +62,100 @@ impl<'a> StableHashingContext<'a> {
6062
}
6163
}
6264

65+
#[inline]
66+
fn def_span(&self, def_id: LocalDefId) -> Span {
67+
self.untracked.source_span.get(def_id).unwrap_or(DUMMY_SP)
68+
}
69+
6370
#[inline]
6471
pub fn hashing_controls(&self) -> HashingControls {
6572
self.hashing_controls
6673
}
6774
}
6875

6976
impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> {
77+
/// Hashes a span in a stable way. We can't directly hash the span's `BytePos` fields (that
78+
/// would be similar to hashing pointers, since those are just offsets into the `SourceMap`).
79+
/// Instead, we hash the (file name, line, column) triple, which stays the same even if the
80+
/// containing `SourceFile` has moved within the `SourceMap`.
81+
///
82+
/// Also note that we are hashing byte offsets for the column, not unicode codepoint offsets.
83+
/// For the purpose of the hash that's sufficient. Also, hashing filenames is expensive so we
84+
/// avoid doing it twice when the span starts and ends in the same file, which is almost always
85+
/// the case.
86+
///
87+
/// IMPORTANT: changes to this method should be reflected in implementations of `SpanEncoder`.
7088
#[inline]
71-
fn unstable_opts_incremental_ignore_spans(&self) -> bool {
72-
self.incremental_ignore_spans
89+
fn span_hash_stable(&mut self, span: Span, hasher: &mut StableHasher) {
90+
const TAG_VALID_SPAN: u8 = 0;
91+
const TAG_INVALID_SPAN: u8 = 1;
92+
const TAG_RELATIVE_SPAN: u8 = 2;
93+
94+
if !self.hashing_controls().hash_spans {
95+
return;
96+
}
97+
98+
let span = span.data_untracked();
99+
span.ctxt.hash_stable(self, hasher);
100+
span.parent.hash_stable(self, hasher);
101+
102+
if span.is_dummy() {
103+
Hash::hash(&TAG_INVALID_SPAN, hasher);
104+
return;
105+
}
106+
107+
let parent = span.parent.map(|parent| self.def_span(parent).data_untracked());
108+
if let Some(parent) = parent
109+
&& parent.contains(span)
110+
{
111+
// This span is enclosed in a definition: only hash the relative position. This catches
112+
// a subset of the cases from the `file.contains(parent.lo)`. But we can do this check
113+
// cheaply without the expensive `span_data_to_lines_and_cols` query.
114+
Hash::hash(&TAG_RELATIVE_SPAN, hasher);
115+
(span.lo - parent.lo).to_u32().hash_stable(self, hasher);
116+
(span.hi - parent.lo).to_u32().hash_stable(self, hasher);
117+
return;
118+
}
119+
120+
// If this is not an empty or invalid span, we want to hash the last position that belongs
121+
// to it, as opposed to hashing the first position past it.
122+
let Some((file, line_lo, col_lo, line_hi, col_hi)) =
123+
self.source_map().span_data_to_lines_and_cols(&span)
124+
else {
125+
Hash::hash(&TAG_INVALID_SPAN, hasher);
126+
return;
127+
};
128+
129+
if let Some(parent) = parent
130+
&& file.contains(parent.lo)
131+
{
132+
// This span is relative to another span in the same file,
133+
// only hash the relative position.
134+
Hash::hash(&TAG_RELATIVE_SPAN, hasher);
135+
Hash::hash(&(span.lo.0.wrapping_sub(parent.lo.0)), hasher);
136+
Hash::hash(&(span.hi.0.wrapping_sub(parent.lo.0)), hasher);
137+
return;
138+
}
139+
140+
Hash::hash(&TAG_VALID_SPAN, hasher);
141+
Hash::hash(&file.stable_id, hasher);
142+
143+
// Hash both the length and the end location (line/column) of a span. If we hash only the
144+
// length, for example, then two otherwise equal spans with different end locations will
145+
// have the same hash. This can cause a problem during incremental compilation wherein a
146+
// previous result for a query that depends on the end location of a span will be
147+
// incorrectly reused when the end location of the span it depends on has changed (see
148+
// issue #74890). A similar analysis applies if some query depends specifically on the
149+
// length of the span, but we only hash the end location. So hash both.
150+
151+
let col_lo_trunc = (col_lo.0 as u64) & 0xFF;
152+
let line_lo_trunc = ((line_lo as u64) & 0xFF_FF_FF) << 8;
153+
let col_hi_trunc = (col_hi.0 as u64) & 0xFF << 32;
154+
let line_hi_trunc = ((line_hi as u64) & 0xFF_FF_FF) << 40;
155+
let col_line = col_lo_trunc | line_lo_trunc | col_hi_trunc | line_hi_trunc;
156+
let len = (span.hi - span.lo).0;
157+
Hash::hash(&col_line, hasher);
158+
Hash::hash(&len, hasher);
73159
}
74160

75161
#[inline]
@@ -81,22 +167,26 @@ impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> {
81167
}
82168
}
83169

170+
/// Assert that the provided `HashStableContext` is configured with the default
171+
/// `HashingControls`. We should always have bailed out before getting to here with a
172+
/// non-default mode. With this check in place, we can avoid the need to maintain separate
173+
/// versions of `ExpnData` hashes for each permutation of `HashingControls` settings.
84174
#[inline]
85-
fn def_span(&self, def_id: LocalDefId) -> Span {
86-
self.untracked.source_span.get(def_id).unwrap_or(DUMMY_SP)
87-
}
88-
89-
#[inline]
90-
fn span_data_to_lines_and_cols(
91-
&mut self,
92-
span: &SpanData,
93-
) -> Option<(&SourceFile, usize, BytePos, usize, BytePos)> {
94-
self.source_map().span_data_to_lines_and_cols(span)
95-
}
175+
fn assert_default_hashing_controls(&self, msg: &str) {
176+
let hashing_controls = self.hashing_controls;
177+
let HashingControls { hash_spans } = hashing_controls;
96178

97-
#[inline]
98-
fn hashing_controls(&self) -> HashingControls {
99-
self.hashing_controls
179+
// Note that we require that `hash_spans` be the inverse of the global `-Z
180+
// incremental-ignore-spans` option. Normally, this option is disabled, in which case
181+
// `hash_spans` must be true.
182+
//
183+
// Span hashing can also be disabled without `-Z incremental-ignore-spans`. This is the
184+
// case for instance when building a hash for name mangling. Such configuration must not be
185+
// used for metadata.
186+
assert_eq!(
187+
hash_spans, !self.incremental_ignore_spans,
188+
"Attempted hashing of {msg} with non-default HashingControls: {hashing_controls:?}"
189+
);
100190
}
101191
}
102192

‎compiler/rustc_span/src/hygiene.rs‎

Lines changed: 3 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ use std::{fmt, iter, mem};
3030

3131
use rustc_data_structures::fingerprint::Fingerprint;
3232
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
33-
use rustc_data_structures::stable_hasher::{HashStable, HashingControls, StableHasher};
33+
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
3434
use rustc_data_structures::sync::Lock;
3535
use rustc_data_structures::unhash::UnhashMap;
3636
use rustc_hashes::Hash64;
@@ -126,29 +126,6 @@ rustc_index::newtype_index! {
126126
impl !Ord for LocalExpnId {}
127127
impl !PartialOrd for LocalExpnId {}
128128

129-
/// Assert that the provided `HashStableContext` is configured with the 'default'
130-
/// `HashingControls`. We should always have bailed out before getting to here
131-
/// with a non-default mode. With this check in place, we can avoid the need
132-
/// to maintain separate versions of `ExpnData` hashes for each permutation
133-
/// of `HashingControls` settings.
134-
fn assert_default_hashing_controls(ctx: &impl HashStableContext, msg: &str) {
135-
let hashing_controls = ctx.hashing_controls();
136-
let HashingControls { hash_spans } = hashing_controls;
137-
138-
// Note that we require that `hash_spans` be the inverse of the global
139-
// `-Z incremental-ignore-spans` option. Normally, this option is disabled,
140-
// in which case `hash_spans` must be true.
141-
//
142-
// Span hashing can also be disabled without `-Z incremental-ignore-spans`.
143-
// This is the case for instance when building a hash for name mangling.
144-
// Such configuration must not be used for metadata.
145-
assert_eq!(
146-
hash_spans,
147-
!ctx.unstable_opts_incremental_ignore_spans(),
148-
"Attempted hashing of {msg} with non-default HashingControls: {hashing_controls:?}"
149-
);
150-
}
151-
152129
/// A unique hash value associated to an expansion.
153130
#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, Encodable, Decodable, HashStable_Generic)]
154131
pub struct ExpnHash(Fingerprint);
@@ -1508,7 +1485,7 @@ pub fn raw_encode_syntax_context(
15081485
fn update_disambiguator(expn_data: &mut ExpnData, mut ctx: impl HashStableContext) -> ExpnHash {
15091486
// This disambiguator should not have been set yet.
15101487
assert_eq!(expn_data.disambiguator, 0, "Already set disambiguator for ExpnData: {expn_data:?}");
1511-
assert_default_hashing_controls(&ctx, "ExpnData (disambiguator)");
1488+
ctx.assert_default_hashing_controls("ExpnData (disambiguator)");
15121489
let mut expn_hash = expn_data.hash_expn(&mut ctx);
15131490

15141491
let disambiguator = HygieneData::with(|data| {
@@ -1558,7 +1535,7 @@ impl<CTX: HashStableContext> HashStable<CTX> for SyntaxContext {
15581535

15591536
impl<CTX: HashStableContext> HashStable<CTX> for ExpnId {
15601537
fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) {
1561-
assert_default_hashing_controls(ctx, "ExpnId");
1538+
ctx.assert_default_hashing_controls("ExpnId");
15621539
let hash = if *self == ExpnId::root() {
15631540
// Avoid fetching TLS storage for a trivial often-used value.
15641541
Fingerprint::ZERO

‎compiler/rustc_span/src/lib.rs‎

Lines changed: 10 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ use hygiene::Transparency;
5454
pub use hygiene::{
5555
DesugaringKind, ExpnData, ExpnHash, ExpnId, ExpnKind, LocalExpnId, MacroKind, SyntaxContext,
5656
};
57-
use rustc_data_structures::stable_hasher::HashingControls;
5857
pub mod def_id;
5958
use def_id::{CrateNum, DefId, DefIndex, DefPathHash, LOCAL_CRATE, LocalDefId, StableCrateId};
6059
pub mod edit_distance;
@@ -2796,105 +2795,24 @@ impl InnerSpan {
27962795
/// This is a hack to allow using the [`HashStable_Generic`] derive macro
27972796
/// instead of implementing everything in rustc_middle.
27982797
pub trait HashStableContext {
2798+
/// The main event: stable hashing of a span.
2799+
fn span_hash_stable(&mut self, span: Span, hasher: &mut StableHasher);
2800+
2801+
/// Compute a `DefPathHash`.
27992802
fn def_path_hash(&self, def_id: DefId) -> DefPathHash;
2800-
/// Accesses `sess.opts.unstable_opts.incremental_ignore_spans` since
2801-
/// we don't have easy access to a `Session`
2802-
fn unstable_opts_incremental_ignore_spans(&self) -> bool;
2803-
fn def_span(&self, def_id: LocalDefId) -> Span;
2804-
fn span_data_to_lines_and_cols(
2805-
&mut self,
2806-
span: &SpanData,
2807-
) -> Option<(&SourceFile, usize, BytePos, usize, BytePos)>;
2808-
fn hashing_controls(&self) -> HashingControls;
2803+
2804+
/// Assert that the provided `HashStableContext` is configured with the default
2805+
/// `HashingControls`. We should always have bailed out before getting to here with a
2806+
fn assert_default_hashing_controls(&self, msg: &str);
28092807
}
28102808

28112809
impl<CTX> HashStable<CTX> for Span
28122810
where
28132811
CTX: HashStableContext,
28142812
{
2815-
/// Hashes a span in a stable way. We can't directly hash the span's `BytePos`
2816-
/// fields (that would be similar to hashing pointers, since those are just
2817-
/// offsets into the `SourceMap`). Instead, we hash the (file name, line, column)
2818-
/// triple, which stays the same even if the containing `SourceFile` has moved
2819-
/// within the `SourceMap`.
2820-
///
2821-
/// Also note that we are hashing byte offsets for the column, not unicode
2822-
/// codepoint offsets. For the purpose of the hash that's sufficient.
2823-
/// Also, hashing filenames is expensive so we avoid doing it twice when the
2824-
/// span starts and ends in the same file, which is almost always the case.
2825-
///
2826-
/// IMPORTANT: changes to this method should be reflected in implementations of `SpanEncoder`.
28272813
fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) {
2828-
const TAG_VALID_SPAN: u8 = 0;
2829-
const TAG_INVALID_SPAN: u8 = 1;
2830-
const TAG_RELATIVE_SPAN: u8 = 2;
2831-
2832-
if !ctx.hashing_controls().hash_spans {
2833-
return;
2834-
}
2835-
2836-
let span = self.data_untracked();
2837-
span.ctxt.hash_stable(ctx, hasher);
2838-
span.parent.hash_stable(ctx, hasher);
2839-
2840-
if span.is_dummy() {
2841-
Hash::hash(&TAG_INVALID_SPAN, hasher);
2842-
return;
2843-
}
2844-
2845-
let parent = span.parent.map(|parent| ctx.def_span(parent).data_untracked());
2846-
if let Some(parent) = parent
2847-
&& parent.contains(span)
2848-
{
2849-
// This span is enclosed in a definition: only hash the relative position.
2850-
// This catches a subset of the cases from the `file.contains(parent.lo)`,
2851-
// But we can do this check cheaply without the expensive `span_data_to_lines_and_cols` query.
2852-
Hash::hash(&TAG_RELATIVE_SPAN, hasher);
2853-
(span.lo - parent.lo).to_u32().hash_stable(ctx, hasher);
2854-
(span.hi - parent.lo).to_u32().hash_stable(ctx, hasher);
2855-
return;
2856-
}
2857-
2858-
// If this is not an empty or invalid span, we want to hash the last
2859-
// position that belongs to it, as opposed to hashing the first
2860-
// position past it.
2861-
let Some((file, line_lo, col_lo, line_hi, col_hi)) = ctx.span_data_to_lines_and_cols(&span)
2862-
else {
2863-
Hash::hash(&TAG_INVALID_SPAN, hasher);
2864-
return;
2865-
};
2866-
2867-
if let Some(parent) = parent
2868-
&& file.contains(parent.lo)
2869-
{
2870-
// This span is relative to another span in the same file,
2871-
// only hash the relative position.
2872-
Hash::hash(&TAG_RELATIVE_SPAN, hasher);
2873-
Hash::hash(&(span.lo.0.wrapping_sub(parent.lo.0)), hasher);
2874-
Hash::hash(&(span.hi.0.wrapping_sub(parent.lo.0)), hasher);
2875-
return;
2876-
}
2877-
2878-
Hash::hash(&TAG_VALID_SPAN, hasher);
2879-
Hash::hash(&file.stable_id, hasher);
2880-
2881-
// Hash both the length and the end location (line/column) of a span. If we
2882-
// hash only the length, for example, then two otherwise equal spans with
2883-
// different end locations will have the same hash. This can cause a problem
2884-
// during incremental compilation wherein a previous result for a query that
2885-
// depends on the end location of a span will be incorrectly reused when the
2886-
// end location of the span it depends on has changed (see issue #74890). A
2887-
// similar analysis applies if some query depends specifically on the length
2888-
// of the span, but we only hash the end location. So hash both.
2889-
2890-
let col_lo_trunc = (col_lo.0 as u64) & 0xFF;
2891-
let line_lo_trunc = ((line_lo as u64) & 0xFF_FF_FF) << 8;
2892-
let col_hi_trunc = (col_hi.0 as u64) & 0xFF << 32;
2893-
let line_hi_trunc = ((line_hi as u64) & 0xFF_FF_FF) << 40;
2894-
let col_line = col_lo_trunc | line_lo_trunc | col_hi_trunc | line_hi_trunc;
2895-
let len = (span.hi - span.lo).0;
2896-
Hash::hash(&col_line, hasher);
2897-
Hash::hash(&len, hasher);
2814+
// `span_hash_stable` does all the work.
2815+
ctx.span_hash_stable(*self, hasher)
28982816
}
28992817
}
29002818

0 commit comments

Comments
 (0)