Skip to content

Remove special-cased stable hashing for HIR module #92259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

combine_commutativecan be removed, since it isn't used anywhere else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that it might be used again in the future, so I kept it. cc @michaelwoerister

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's keep it please. I might have a concrete use for it soon.

Copy link
Contributor

@tmiasko tmiasko Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For similar use case that ignores order of combined fingerprints? In that case we should fix the implementation instead of leaving it broken: when used as here it doesn't completely ignore the order when combining more than two fingerprints.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened to #92528 to remove unintentional swap of lower and upper part of the result.


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
Original file line number Diff line number Diff line change
@@ -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() {}