Skip to content
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

Investigate usages of #[stable_hasher(project(name))] #91921

Closed
Aaron1011 opened this issue Dec 14, 2021 · 5 comments
Closed

Investigate usages of #[stable_hasher(project(name))] #91921

Aaron1011 opened this issue Dec 14, 2021 · 5 comments
Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Dec 14, 2021

In several places, we use #[stable_hasher(project(name))] to skip hashing the Span of an Ident. For example:

#[derive(Debug, HashStable_Generic)]
pub struct PathSegment<'hir> {
/// The identifier portion of this path segment.
#[stable_hasher(project(name))]
pub ident: Ident,

It's not obvious why this is correct. Assuming that this isn't causing any issues with incr comp invalidation, then we should add comments explaining why this is the right thing to do.

@Aaron1011 Aaron1011 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-incr-comp Area: Incremental compilation labels Dec 14, 2021
@pierwill
Copy link
Member

@rustbot claim

@petrochenkov
Copy link
Contributor

IIRC, it was done purely as a bug-compatibility measure when Idents were introduced to HIR instead of Symbols to support hygiene.
Hashing spans caused either incremental errors or perf regressions.
This was long ago, so the issues preventing span hashing may be fixed now, we need to try to remove this annotations and check what happens.

@Aaron1011
Copy link
Member Author

This is now causing a bug - since we now serialize the full AdtDef to disk, we may try to deserialize a stale ExpnId from a foreign crate, because we will incorrect end up thinking that an AdtDef is unchanged when one of its Span fields changes. See #92192

@Aaron1011
Copy link
Member Author

This was fixed in #92534, #93095, and #92533

@pierwill
Copy link
Member

pierwill commented Feb 6, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants