Clean up diagnostic hashing - #163148
Merged
Merged
Clean up diagnostic hashing#163148
Conversation
`DiagInner` impls `PartialEq` and `Hash`, as you'd expect for storing it in a hash table. But there's a couple of strange things. - We only store the hash value of the `DiagInner` to do deduplication, not the `DiagInner` itself, which means the `PartialEq` impl is unused. - The `Hash` impl only considers some of the fields. Some of the ignored fields are clearly deliberate (there are comments) but for some it is unclear if it is deliberate. This commit: - Removes the unused `PartialEq` impl. - Inlines and removes `keys` now that it's not needed for `PartialEq`. - Uses struct deconstruction to ensure no fields can be accidentally ignored. I have preserved existing behaviour by assuming that all the ignored fields are supposed to be ignored. - Renames `hash` as an inherent method `dedup_hash` to indicate that it's not a typical hash function, and simplifies it to just return `Hash128` instead of being generic. - Replaces the unnecessary `collect` on `args` with `as_slice`. - Improves the comment on `emitted_diagnostics`.
rustbot
added
S-waiting-on-review
Member
|
Have you seen #162901 by the way? |
Contributor
Author
|
I haven't. It's orthogonal, because this is just a refactoring. Looks like the |
oli-obk
approved these changes
Sep 22, 2026
Contributor
rust-bors
Bot
added
S-waiting-on-bors
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Sep 22, 2026
…r=oli-obk Clean up diagnostic hashing `DiagInner` impls `PartialEq` and `Hash`, as you'd expect for storing it in a hash table. But there's a couple of strange things. - We only store the hash value of the `DiagInner` to do deduplication, not the `DiagInner` itself, which means the `PartialEq` impl is unused. - The `Hash` impl only considers some of the fields. Some of the ignored fields are clearly deliberate (there are comments) but for some it is unclear if it is deliberate. This commit: - Removes the unused `PartialEq` impl. - Inlines and removes `keys` now that it's not needed for `PartialEq`. - Uses struct deconstruction to ensure no fields can be accidentally ignored. I have preserved existing behaviour by assuming that all the ignored fields are supposed to be ignored. - Renames `hash` as an inherent method `dedup_hash` to indicate that it's not a typical hash function, and simplifies it to just return `Hash128` instead of being generic. - Replaces the unnecessary `collect` on `args` with `as_slice`. - Improves the comment on `emitted_diagnostics`. r? @oli-obk
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Sep 22, 2026
…r=oli-obk Clean up diagnostic hashing `DiagInner` impls `PartialEq` and `Hash`, as you'd expect for storing it in a hash table. But there's a couple of strange things. - We only store the hash value of the `DiagInner` to do deduplication, not the `DiagInner` itself, which means the `PartialEq` impl is unused. - The `Hash` impl only considers some of the fields. Some of the ignored fields are clearly deliberate (there are comments) but for some it is unclear if it is deliberate. This commit: - Removes the unused `PartialEq` impl. - Inlines and removes `keys` now that it's not needed for `PartialEq`. - Uses struct deconstruction to ensure no fields can be accidentally ignored. I have preserved existing behaviour by assuming that all the ignored fields are supposed to be ignored. - Renames `hash` as an inherent method `dedup_hash` to indicate that it's not a typical hash function, and simplifies it to just return `Hash128` instead of being generic. - Replaces the unnecessary `collect` on `args` with `as_slice`. - Improves the comment on `emitted_diagnostics`. r? @oli-obk
This was referenced Sep 22, 2026
rust-bors Bot
pushed a commit
that referenced
this pull request
Sep 22, 2026
…uwer Rollup of 7 pull requests Successful merges: - #147876 (Check tainted_by_error in LateLint) - #162998 (Avoid generating overlapping assignments in DSE) - #163136 (library: prune allowed lints) - #163102 (remove unnecessary restriction with next-solver) - #163106 (emit the constant pattern note for raw identifier bindings) - #163118 (add `feature(field_projections)` fixme) - #163148 (Clean up diagnostic hashing)
rust-bors Bot
pushed a commit
that referenced
this pull request
Sep 22, 2026
Rollup merge of #163148 - nnethercote:improve-Diag-hashing, r=oli-obk Clean up diagnostic hashing `DiagInner` impls `PartialEq` and `Hash`, as you'd expect for storing it in a hash table. But there's a couple of strange things. - We only store the hash value of the `DiagInner` to do deduplication, not the `DiagInner` itself, which means the `PartialEq` impl is unused. - The `Hash` impl only considers some of the fields. Some of the ignored fields are clearly deliberate (there are comments) but for some it is unclear if it is deliberate. This commit: - Removes the unused `PartialEq` impl. - Inlines and removes `keys` now that it's not needed for `PartialEq`. - Uses struct deconstruction to ensure no fields can be accidentally ignored. I have preserved existing behaviour by assuming that all the ignored fields are supposed to be ignored. - Renames `hash` as an inherent method `dedup_hash` to indicate that it's not a typical hash function, and simplifies it to just return `Hash128` instead of being generic. - Replaces the unnecessary `collect` on `args` with `as_slice`. - Improves the comment on `emitted_diagnostics`. r? @oli-obk
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
4 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.
DiagInnerimplsPartialEqandHash, as you'd expect for storing it in a hash table. But there's a couple of strange things.We only store the hash value of the
DiagInnerto do deduplication, not theDiagInneritself, which means thePartialEqimpl is unused.The
Hashimpl only considers some of the fields. Some of the ignored fields are clearly deliberate (there are comments) but for some it is unclear if it is deliberate.This commit:
Removes the unused
PartialEqimpl.Inlines and removes
keysnow that it's not needed forPartialEq.Uses struct deconstruction to ensure no fields can be accidentally ignored. I have preserved existing behaviour by assuming that all the ignored fields are supposed to be ignored.
Renames
hashas an inherent methoddedup_hashto indicate that it's not a typical hash function, and simplifies it to just returnHash128instead of being generic.Replaces the unnecessary
collectonargswithas_slice.Improves the comment on
emitted_diagnostics.r? @oli-obk