Revert #126024 "Do not use global caches if opaque types can be defined"

[lqd]

This PR prepares a revert of #126024 as discussed on zulip, because of its regressions to compilation times vs unclear soundness fix. Opening as draft for discussion between @compiler-errors and @lcnr.

---

#126024 didn't fix the ICE in #119272 (proof). Enabling the new solver in coherence did fix both the codegen ICE and incorrectly accepting the code in the first place.

It seems the theoretical unsoundness fixed in #126024 didn't yet have an MCVE showcasing the issue, and didn't have tests. The correct fix is in principle to use the new solver.

However, that is not happening soon and until then #126024 has a big impact on compile-times in some cases:

• as seen in this zulip thread: fuchsia is seeing a 10x regression on the starnix_core crate with Do not use global caches if opaque types can be defined #126024.
• similarly, as seen in regression: crate compiles much slower with 1.82 #132064: it looks like one crate is making the entire project build 6-7x slower in 1.82.0. It seemed likely that people would encounter the same behavior when Do not use global caches if opaque types can be defined #126024 landed on stable
• even the post-merge perf results showed significant regressions on a popular crate (hyper). Some of the PR's effects should be seen by a large number of people but thankfully they're not always as dramatic as the previous 2 cases.

---

I didn't add the crash test for #119272 back, and the auto-traits/opaque_type_candidate_selection.rs test was deleted in #130654. I assume the latter was because there already was coverage for such a case, but if not I can add that test back, updated for -Znext-solver=coherence. _{I don't have an MCVE for #132064 but if needed, I could try to minimize it.}

Fixes #132064.

[rustbot]

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[lqd]

The first failure is just a test needing expectations updated, but the second is that type-alias-impl-trait/reveal_local.rs now ICEs in #122192 code, which is understandable since #126024 was extracted from it.

Just to check, I also tried to revert #122192 and all UI tests passed.

[bors]

☔ The latest upstream changes (presumably #132349) made this pull request unmergeable. Please resolve the merge conflicts.

[compiler-errors]

Hi @lqd, can you rebase this PR and getting this into a landable state? I'm in favor of landing this, especially because the unsoundness it fixes was never really deeply investigated AFAICT, and it's affecting people on stable. cc @lcnr

[compiler-errors]

@rustbot author

[lqd]

Hi @lqd, can you rebase this PR and getting this into a landable state?

Hi @compiler-errors,

I can get this into a reviewable state (and just pushed), but I'm not sure I can get it into a landable state per se: this is getting too subtle for my puny brain.

As mentioned in my last comment, some of #122192 depends on #126024, and I've also reverted part of it so that tests pass. This seems somewhat ok as it was just an unreachable!() protecting the previous status quo, but I'm not sure of the subtle effects and if it was only exercised via TAITs.

I've tried to add comments, but we'd likely need actual an issue to track the possible unsoundness / not using the global cache when defining opaque types, even if it would end up being fixed only when the new solver stabilizes.

So that's why it's still in draft. At this point, I'm not sure if I'm just wasting your time and it'd have been faster if you/lcnr had done the revert instead.

@rustbot ready

[compiler-errors]

Oof, sorry I have not been following closely enough to understand the problem with the revert. I'll take a closer look -- thanks for pushing what you have.

[bors]

☔ The latest upstream changes (presumably #132625) made this pull request unmergeable. Please resolve the merge conflicts.

[lqd]

@lcnr @compiler-errors do you want me to close this PR?

[lcnr]

yeah, I'd think so 👍 thanks for tracking this work and making sure we actually do something here