Draft
Conversation
rustbot
added
A-attributes
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Why on earth is |
Contributor
Author
|
I just tried PR on an aarch64-unknown-linux-gnu box that I have and it compiled fine. Weird. |
This comment has been minimized.
This comment has been minimized.
nnethercote
force-pushed
the
simplify-HashStable
branch
from
3bc77fb to
9e9a113
Compare
petrochenkov
reviewed
Apr 9, 2026
| /// visible. | ||
| pub trait HashStableContext { | ||
| /// The main event: stable hashing of a span. | ||
| fn span_hash_stable(&mut self, span: RawSpan, hasher: &mut StableHasher); |
Contributor
There was a problem hiding this comment.
Suggested change
| fn span_hash_stable(&mut self, span: RawSpan, hasher: &mut StableHasher); | |
| fn span_hash_stable(&mut self, span: Self::Span, hasher: &mut StableHasher); |
Perhaps Span, DefId and RawDefPathHash could be associated types in HashStableContext?
Then impl HashStableContext for StableHashingContext could fill them with concrete types.
If that works then "type erased" versions of the aforementioned types could be avoided.
Contributor
Author
There was a problem hiding this comment.
I tried that but I couldn't get it to work.
Contributor
Author
There was a problem hiding this comment.
Specifically, if I add an associate type MySpan to the trait, then in Span::hash_stable I get this error:
error[E0308]: mismatched types
--> compiler/rustc_span/src/lib.rs:2793:30
|
2793 | hcx.span_hash_stable(*self, hasher)
| ---------------- ^^^^^ expected associated type, found `Span`
| |
| arguments to this method are incorrect
|
= note: expected associated type `<Hcx as HashStableContext>::MySpan`
found struct `span_encoding::Span`
help: consider constraining the associated type `<Hcx as HashStableContext>::MySpan` to `span_encoding::Span`
|
2791 | fn hash_stable<Hcx: HashStableContext<MySpan = span_encoding::Span>>(&self, hcx: &mut Hcx, hasher: &mut StableHasher) {
| ++++++++++++++++++++++++++++++
Then if I try the help suggestion I get this error:
error[E0271]: type mismatch resolving `<Hcx as HashStableContext>::MySpan == Span`
--> compiler/rustc_span/src/lib.rs:2791:43
|
2791 | ...ashStableContext<MySpan = Span>>(&self, hcx: &mut Hcx, hasher: &mut StableHasher) {
| ^^^^^^^^^^^^^ expected `Span`, found associated type
|
= note: expected struct `span_encoding::Span`
found associated type `<Hcx as HashStableContext>::MySpan`
note: the requirement `<Hcx as HashStableContext>::MySpan == span_encoding::Span` appears on the `impl`'s method `hash_stable` but not on the corresponding trait's method
--> compiler/rustc_data_structures/src/stable_hasher.rs:82:8
In other words, the impl method no longer matches the trait method.
Maybe there's a way around this? I couldn't find one.
Contributor
There was a problem hiding this comment.
You can probably make it work with appropriate bounds on the associated types for the impl, but that's going to be crate dependent so still doesn't play nicely with the proc macro. I think you'd need to keep the generic on the trait too.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
This is blocked by #154924. |
nnethercote
added
S-blocked
Contributor
Author
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
`def_id.to_stable_hash_key(hcx)` calls `hcx.def_path_hash(def_id)`. This commit replaces various other occurrences of the latter form with the former form. It's all inlined so there should be no perf effects. This will make things easier for the next commit, which will change `DefId::to_stable_hash_key`.
This puts it in the same crate as the `HashStable` and `ToStableHasher` traits. This requires introducing three types `RawSpan`, `RawDefId` and `RawDefPathHash` to work around the fact that `rustc_data_structures` is upstream of `rustc_span` and so doesn't have access to `Span`, `DefId`, and `DefPathHash`. This is a bit ugly but is worth it because moving `HashStableContext` enables big cleanups across many crates in subsequent commits.
`std::hash::Hash` looks like this:
```
pub trait Hash {
fn hash<H>(&self, state: &mut H)
where H: Hasher;
...
}
```
The method is generic.
In contrast, `HashStable` looks like this:
```
pub trait HashStable<Hcx> {
fn hash_stable(&self, hcx: &mut Hcx, hasher: &mut StableHasher);
}
```
and impls look like this (in crates upstream of `rustc_middle`):
```
impl<Hcx: HashStableContext> HashStable<Hcx> for Path {
fn hash_stable(&self, hcx: &mut Hcx, hasher: &mut StableHasher) {
...
}
}
```
or this (in `rustc_middle` and crates downstream of `rustc_middle`):
```
impl<'tcx> HashStable<StableHashingContext<'tcx>> for rustc_feature::Features {
fn hash_stable(&self, hcx: &mut StableHashingContext<'tcx>, hasher: &mut StableHasher) {
...
}
}
```
Differences to `std::hash::Hash`:
- The trait is generic, rather than the method.
- The way impls are written depends their position in the crate graph.
- This explains why we have both `derive(HashStable)` and
`derive(HashStable_Generic)`. The former is for the
downstream-of-`rustc_middle` case, the latter is for the upstream of
`rustc_middle` case.
Why the differences? It all boils down to `HashStable` and
`HashStableContext` being in different crates. But the previous commit
fixed that, which means `HashStable` can be simplified to this, with a
generic method:
```
pub trait HashStable {
fn hash_stable<Hcx: HashStableContext>(&self, hcx: &mut Hcx, hasher: &mut StableHasher);
}
```
and all impls look like this:
```
impl HashStable for Path {
fn hash_stable<Hcx: HashStableContext>(&self, hcx: &mut Hcx, hasher: &mut StableHasher) {
...
}
}
```
Other consequences:
- `derive(HashStable_Generic)` is no longer needed; `derive(HashStable)`
can be used instead.
- In this commit, `derive(HashStable_Generic` is made a synonym of
`derive(HashStable)`. The next commit will remove this synonym,
because it's a change that touches many lines.
- `#[stable_hash_generic]` is no longer needed (for `newtype_index`);
`#[stable_hash]` can be used instead.
- `#[stable_hash_no_context]` was already a synonym of
`#[stable_hash_generic]`, so it's also removed in favour of just
`#[stable_hash]`.
- The difference between `derive(HashStable)` and
`derive(HashStable_NoContext)` now comes down to the difference
between `synstructure::AddBounds::Generics` and
`synstructure::AddBounds::Fields`, which is basically "vanilla derive"
vs "(near) perfect derive".
- I have improved the comments on `HashStableMode` to better
explaining this subtle difference.
- `rustc_middle/src/ich/impls_syntax.rs` is no longer needed; the
relevant impls can be defined in the crate that defines the relevant
type.
- Occurrences of `for<'a> HashStable<StableHashingContext<'a>>` are
replaced with with `HashStable`, hooray.
- The commit adds a `HashStableContext::hashing_controls` method, which
is no big deal.
Overall this is a big simplification, removing a lots of confusing
complexity in stable hashing traits.
It's now just a synonym for `derive(HashStable)`.
Thanks to the `HashStable` trait being simplified, `HashStable_NoContext` is only needed when a (near) perfect derive is required. In practice, this means it's only needed for types with a generic `<I: Interner>` parameter. This commit replaces `derive(HashStable_NoContext)` with `derive(HashStable)` for all types that don't have `<I: Interner>`.
`rustc_middle::ich` is now just a very thin wrapper around `rustc_middle::ich::hcx`, so the contents of the latter can be moved into the former.
XXX: It's causing problems on aarch64-gnu-llvm-21-1 on CI.
nnethercote
force-pushed
the
simplify-HashStable
branch
from
9e9a113 to
ce139ba
Compare
Contributor
Author
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (959be73): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 6.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 488.849s -> 492.643s (0.78%) |
rustbot
added
perf-regression
Contributor
|
☔ The latest upstream changes (presumably #155099) made this pull request unmergeable. Please resolve the merge conflicts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR:
HashStabletrait, by moving its generic parameter from the trait to its single method.derive(HashStable)/derive(HashStable_Generic)distinction.derive(HashStable)/derive(HashStable_NoContext)distinction.