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.

Sign up for GitHub

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

Jump to bottom

Fix HashStable implementation on InferTy #91892

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Fix HashStable implementation on InferTy #91892

merged 1 commit into from
Dec 14, 2021

Conversation

compiler-errors
Copy link
Member

HashStable impl forgot to hash the discriminant.

Fixes #91807

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 14, 2021
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 14, 2021
@nbdd0121
Copy link
Contributor

I thought about a potential hash collision but scratched my head really hard looking at #88552 (the regressing PR) and couldn't find anything suspicious. Didn't expect the cause to be somewhere that I completely untouched :)

Given that this is a hash collision issue, I wonder if the added test is sufficient to prevent regression.

@compiler-errors
Copy link
Member Author

@nbdd0121, yeah, the only reason the regression was on your PR was because you introduced a new query that caused us to infer a IntVar and TyVar that collided in HashStable (modulo discriminant).

Not sure what the best way to more thoroughly prevent regressions via a test. On the other hand, I might audit the remainder of the (non-derive) HashStable impls in rustc to check that they're consistent with their Eq implementations.

@dtolnay
Copy link
Member

dtolnay commented Dec 14, 2021

Is it a known flaw that a hash collision can manifest as an ICE in the first place? I'm wondering whether this PR should close #91807 or keep an issue open to track this. In general it's not correct that unequal items have an unequal hash, even with hash impls that hit all the fields/discriminants. Are there enough bits in this hash that we're happy pretending that's the case?

@dtolnay
Copy link
Member

dtolnay commented Dec 14, 2021

Either way, this PR looks obviously correct to me. :) Thanks!

Maybe a good follow-up would be to see if there's a good place to insert a tip in the ICE, so that instead of/in addition to "forcing query with already existing DepNode", the panic message would point out you've most likely got a colliding hash.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 14, 2021

📌 Commit 75729af has been approved by dtolnay

@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 Dec 14, 2021
@nbdd0121
Copy link
Contributor

Is it a known flaw that a hash collision can manifest as an ICE in the first place? I'm wondering whether this PR should close #91807 or keep an issue open to track this. In general it's not correct that unequal items have an unequal hash, even with hash impls that hit all the fields/discriminants. Are there enough bits in this hash that we're happy pretending that's the case?

It's intended that hash only is used: https://rustc-dev-guide.rust-lang.org/queries/incremental-compilation-in-detail.html

This approach works rather well but it's not without flaws:

  • There is a small possibility of hash collisions. That is, two different results could have the same fingerprint and the system would erroneously assume that the result hasn't changed, leading to a missed update.

    We mitigate this risk by using a high-quality hash function and a 128 bit wide hash value. Due to these measures the practical risk of a hash collision is negligible.

It works well in practice (modulo incorrect implementation)

@dtolnay
Copy link
Member

dtolnay commented Dec 14, 2021

Okay, I buy that. Thanks for the link! I agree 128 bits is enough that this won't happen with correct impls.

🙈

@compiler-errors
Copy link
Member Author

Maybe a good follow-up would be to see if there's a good place to insert a tip in the ICE, so that instead of/in addition to "forcing query with already existing DepNode", the panic message would point out you've most likely got a colliding hash.

Good suggestion! I'll probably turn that assert into a bug! and leave a more detailed error message that the Eq and HashStable implementations probably have diverged for the key type, in case ICEs like this show up in the future.

@matthiaskrgr matthiaskrgr mentioned this pull request Dec 14, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2021
@bors
Auto merge of rust-lang#91902 - matthiaskrgr:rollup-hjjyhow, r=matthi…
404c847 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91529 (add BinaryHeap::try_reserve and BinaryHeap::try_reserve_exact)
 - rust-lang#91820 (Suggest to specify a target triple when lang item is missing)
 - rust-lang#91851 (Make `MaybeUninit::zeroed` `const`)
 - rust-lang#91875 (Use try_normalize_erasing_regions in RevealAllVisitor)
 - rust-lang#91887 (Remove `in_band_lifetimes` from `rustc_const_eval`)
 - rust-lang#91892 (Fix HashStable implementation on InferTy)
 - rust-lang#91893 (Remove `in_band_lifetimes` from `rustc_hir`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d0e6bb7 into rust-lang:master Dec 14, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 14, 2021
@teohhanhui
Copy link

Considering this was a regression, can it be backported to stable?

https://forge.rust-lang.org/release/backporting.html#stable-backporting-in-rust-langrust

@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 15, 2021
@pnkfelix
Copy link
Member

discussed in T-compiler meeting. Beta-accepted.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 16, 2021
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.59.0, 1.58.0 Jan 5, 2022
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 5, 2022
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jan 5, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2022
@bors
Auto merge of rust-lang#92592 - Mark-Simulacrum:beta-next, r=Mark-Sim…
4aa9d23 
…ulacrum

[beta] backports

Backports these PRs:

* Fix HashStable implementation on InferTy rust-lang#91892
* Revert setting a default for the MACOSX_DEPLOYMENT_TARGET env var for linking rust-lang#91870
* Make rustdoc headings black, and markdown blue rust-lang#91534
* Disable LLVM newPM by default rust-lang#91190
* Deduplicate projection sub-obligations rust-lang#90423
*  Sync portable-simd to remove autosplats rust-lang#91484 by dropping portable_simd entirely (keeping the subtree, just from std/core)
*  Quote bat script command line rust-lang#92208
* Fix failing tests rust-lang#92201 (CI fix)

r? `@Mark-Simulacrum`
@compiler-errors compiler-errors deleted the fix-inferty-hashtable branch April 7, 2022 04:36
@dtolnay dtolnay assigned dtolnay and unassigned estebank Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dtolnay dtolnay

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.

Regression in Deref<Target = dyn Fn(T)>: forcing query with already existing DepNode
10 participants
@compiler-errors @rust-highfive @nbdd0121 @dtolnay @bors @teohhanhui @pnkfelix @estebank @Mark-Simulacrum @rustbot