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

Use DefId instead of Span in BrAnon to avoid stable hashing collisions #104274

Closed

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Nov 11, 2022

Recent reports of ICEs due to the fact that BrAnon regions now capture spans, which apparently sometimes can StableHash into identical values but Hash into different values, causing collisions in dep nodes that are not covered by identical cache entries.

We can avoid this by just using a DefId instead which doesn't affect diagnostics edit: this is not true.

I have no idea how to turn #104271, #104255, or #104238 into a MCVE, though, but local testing shows that the projects build without ICE after this change.

This brings up a bigger question about whether we should be rejecting Spans from query keys (or at least being much more careful about them)...

Fixes #104271
Fixes #104255
Fixes #104238

cc #103171 @cjgillot @jackh726

@rustbot
Copy link
Collaborator

rustbot commented Nov 11, 2022

r? @fee1-dead

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 11, 2022
@compiler-errors
Copy link
Member Author

Hm, actually, that diagnostic does kinda suck now for the '_ case, since that span is totally different from what we used to be showing.

However, I'm inclined to land this as-is given the ICEs (and to avoid having to revert the original PR altogther), but I'd like @jackh726's opinion on it.

@cjgillot cjgillot self-assigned this Nov 11, 2022
@jackh726
Copy link
Member

Yeah, I had tried this first before I realized that the diagnostics were pointing to the wrong place.

I guess we can land this. I suppose the error is still better than what it was before.

Ugh.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 11, 2022

📌 Commit 7c9c62d has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 11, 2022
@jackh726
Copy link
Member

And let's try to get an MCVE in at some point.

@jackh726
Copy link
Member

@bors r-

Let's see about #104282

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 11, 2022
@compiler-errors
Copy link
Member Author

Superseded, not needed anymore!

@compiler-errors compiler-errors deleted the br-anon-stable-hashing branch August 11, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants