Fix HashStable implementation on InferTy#91892
Conversation
|
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
|
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. |
|
@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. |
|
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? |
|
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+ |
|
📌 Commit 75729af has been approved by |
It's intended that hash only is used: https://rustc-dev-guide.rust-lang.org/queries/incremental-compilation-in-detail.html
It works well in practice (modulo incorrect implementation) |
|
Okay, I buy that. Thanks for the link! I agree 128 bits is enough that this won't happen with correct impls. 🙈 |
Good suggestion! I'll probably turn that assert into a |
…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
|
Considering this was a regression, can it be backported to stable? https://forge.rust-lang.org/release/backporting.html#stable-backporting-in-rust-langrust |
|
discussed in T-compiler meeting. Beta-accepted. |
…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`
HashStable impl forgot to hash the discriminant.
Fixes #91807