Skip to content

Commit

Permalink
Remove special-cased stable hashing for HIR module
Browse files Browse the repository at this point in the history
All other 'containers' (e.g. `impl` blocks) hashed their contents
in the normal, order-dependent way. However, `Mod` was hashing
its contents in a (sort-of) order-independent way. However, the
exact order is exposed to consumers through `Mod.item_ids`,
and through query results like `hir_module_items`. Therefore,
stable hashing needs to take the order of items into account,
to avoid fingerprint ICEs.

Unforuntately, I was unable to directly build a reproducer
for the ICE, due to the behavior of `Fingerprint::combine_commutative`.
This operation swaps the upper and lower `u64` when constructing the
result, which makes the function non-associative. Since we start
the hashing of module items by combining `Fingerprint::ZERO` with
the first item, it's difficult to actually build an example where
changing the order of module items leaves the final hash unchanged.

However, this appears to have been hit in practice in #92218
While we're not able to reproduce it, the fact that proc-macros
are involved (which can give an entire module the same span, preventing
any span-related invalidations) makes me confident that the root
cause of that issue is our method of hashing module items.

This PR removes all of the special handling for `Mod`, instead deriving
a `HashStable` implementation. This makes `Mod` consistent with other
'contains' like `Impl`, which hash their contents through the typical
derive of `HashStable`.
  • Loading branch information
Aaron1011 committed Dec 24, 2021
1 parent 489296d commit da3f196
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 33 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_hir/src/hir.rs
Expand Up @@ -2484,7 +2484,7 @@ impl FnRetTy<'_> {
}
}

#[derive(Encodable, Debug)]
#[derive(Encodable, Debug, HashStable_Generic)]
pub struct Mod<'hir> {
/// A span from the first token past `{` to the last token until `}`.
/// For `mod foo;`, the inner span ranges from the first token
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_hir/src/stable_hash_impls.rs
Expand Up @@ -2,7 +2,7 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher, ToStableHas

use crate::hir::{
AttributeMap, BodyId, Crate, Expr, ForeignItem, ForeignItemId, ImplItem, ImplItemId, Item,
ItemId, Mod, OwnerNodes, TraitCandidate, TraitItem, TraitItemId, Ty, VisibilityKind,
ItemId, OwnerNodes, TraitCandidate, TraitItem, TraitItemId, Ty, VisibilityKind,
};
use crate::hir_id::{HirId, ItemLocalId};
use rustc_span::def_id::DefPathHash;
Expand All @@ -16,7 +16,6 @@ pub trait HashStableContext:
fn hash_hir_id(&mut self, _: HirId, hasher: &mut StableHasher);
fn hash_body_id(&mut self, _: BodyId, hasher: &mut StableHasher);
fn hash_reference_to_item(&mut self, _: HirId, hasher: &mut StableHasher);
fn hash_hir_mod(&mut self, _: &Mod<'_>, hasher: &mut StableHasher);
fn hash_hir_expr(&mut self, _: &Expr<'_>, hasher: &mut StableHasher);
fn hash_hir_ty(&mut self, _: &Ty<'_>, hasher: &mut StableHasher);
fn hash_hir_visibility_kind(&mut self, _: &VisibilityKind<'_>, hasher: &mut StableHasher);
Expand Down Expand Up @@ -132,12 +131,6 @@ impl<HirCtx: crate::HashStableContext> HashStable<HirCtx> for TraitItemId {
}
}

impl<HirCtx: crate::HashStableContext> HashStable<HirCtx> for Mod<'_> {
fn hash_stable(&self, hcx: &mut HirCtx, hasher: &mut StableHasher) {
hcx.hash_hir_mod(self, hasher)
}
}

impl<HirCtx: crate::HashStableContext> HashStable<HirCtx> for Expr<'_> {
fn hash_stable(&self, hcx: &mut HirCtx, hasher: &mut StableHasher) {
hcx.hash_hir_expr(self, hasher)
Expand Down
25 changes: 1 addition & 24 deletions compiler/rustc_query_system/src/ich/impls_hir.rs
Expand Up @@ -3,8 +3,7 @@

use crate::ich::hcx::BodyResolver;
use crate::ich::{NodeIdHashingMode, StableHashingContext};
use rustc_data_structures::fingerprint::Fingerprint;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher, ToStableHashKey};
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_hir as hir;
use std::mem;

Expand Down Expand Up @@ -47,28 +46,6 @@ impl<'ctx> rustc_hir::HashStableContext for StableHashingContext<'ctx> {
})
}

#[inline]
fn hash_hir_mod(&mut self, module: &hir::Mod<'_>, hasher: &mut StableHasher) {
let hcx = self;
let hir::Mod { inner: ref inner_span, ref item_ids } = *module;

inner_span.hash_stable(hcx, hasher);

// Combining the `DefPathHash`s directly is faster than feeding them
// into the hasher. Because we use a commutative combine, we also don't
// have to sort the array.
let item_ids_hash = item_ids
.iter()
.map(|id| {
let def_path_hash = id.to_stable_hash_key(hcx);
def_path_hash.0
})
.fold(Fingerprint::ZERO, |a, b| a.combine_commutative(b));

item_ids.len().hash_stable(hcx, hasher);
item_ids_hash.hash_stable(hcx, hasher);
}

fn hash_hir_expr(&mut self, expr: &hir::Expr<'_>, hasher: &mut StableHasher) {
self.while_hashing_hir_bodies(true, |hcx| {
let hir::Expr { hir_id: _, ref span, ref kind } = *expr;
Expand Down
28 changes: 28 additions & 0 deletions src/test/incremental/hash-module-order.rs
@@ -0,0 +1,28 @@
// revisions: rpass1 rpass2
// compile-flags: -Z incremental-ignore-spans -Z query-dep-graph

// Tests that module hashing depends on the order of the items
// (since the order is exposed through `Mod.item_ids`).
// Changing the order of items (while keeping `Span`s the same)
// should still result in `hir_owner` being invalidated.
// Note that it's possible to keep the spans unchanged using
// a proc-macro (e.g. producing the module via `quote!`)
// but we use `-Z incremental-ignore-spans` for simplicity

#![feature(rustc_attrs)]

#[cfg(rpass1)]
#[rustc_clean(cfg="rpass1",except="hir_owner")]
mod foo {
struct First;
struct Second;
}

#[cfg(rpass2)]
#[rustc_clean(cfg="rpass2",except="hir_owner")]
mod foo {
struct Second;
struct First;
}

fn main() {}

0 comments on commit da3f196

Please sign in to comment.