remove keys w/ skolemized regions from proj cache when popping skolemized regions #37294
Conversation
|
☔ The latest upstream changes (presumably #37289) made this pull request unmergeable. Please resolve the merge conflicts. |
brson
added
the
beta-nominated
|
@bors r+ |
|
@nikomatsakis oh there's conflicts; well, r=me. :) |
|
📌 Commit 80aeb7b has been approved by |
nikomatsakis
force-pushed
the
issue-37154
branch
from
80aeb7b to
483bc86
Compare
|
@bors r+ |
|
📌 Commit 483bc86 has been approved by |
|
⌛ Testing commit 483bc86 with merge f5cfbf6... |
|
@bors: retry force clean |
…sakis remove keys w/ skolemized regions from proj cache when popping skolemized regions This addresses rust-lang#37154 (a regression). The projection cache was incorrectly caching the results for skolemized regions -- when we pop skolemized regions, we are supposed to drop cache keys for them (just as we remove those skolemized regions from the region inference graph). This is because those skolemized region numbers will be reused later with different meaning (and we have determined that the old ones don't leak out in any meaningful way). I did a *somewhat* aggressive fix here of only removing keys that mention the skolemized regions. One could imagine just removing all keys added since we started the skolemization (as indeed I did in my initial commit). This more aggressive fix required fixing a latent bug in `TypeFlags`, as an aside. I believe the more aggressive fix is correct; clearly there can be entries that are unrelated to the skoelemized region, and it's a shame to remove them. My one concern was that it *is* possible I believe to have some region variables that are created and related to skolemized regions, and maybe some of them could end up in the cache. However, that seems harmless enough to me-- those relations will be removed, and couldn't have impacted how trait resolution proceeded anyway (iow, the cache entry is not wrong, though it is kind of useless). r? @pnkfelix cc @arielb1
remove keys w/ skolemized regions from proj cache when popping skolemized regions This addresses #37154 (a regression). The projection cache was incorrectly caching the results for skolemized regions -- when we pop skolemized regions, we are supposed to drop cache keys for them (just as we remove those skolemized regions from the region inference graph). This is because those skolemized region numbers will be reused later with different meaning (and we have determined that the old ones don't leak out in any meaningful way). I did a *somewhat* aggressive fix here of only removing keys that mention the skolemized regions. One could imagine just removing all keys added since we started the skolemization (as indeed I did in my initial commit). This more aggressive fix required fixing a latent bug in `TypeFlags`, as an aside. I believe the more aggressive fix is correct; clearly there can be entries that are unrelated to the skoelemized region, and it's a shame to remove them. My one concern was that it *is* possible I believe to have some region variables that are created and related to skolemized regions, and maybe some of them could end up in the cache. However, that seems harmless enough to me-- those relations will be removed, and couldn't have impacted how trait resolution proceeded anyway (iow, the cache entry is not wrong, though it is kind of useless). r? @pnkfelix cc @arielb1
alexcrichton
added
the
T-compiler
|
I feel mildly uncomfortable accepting this for beta-backport since I am the author. However, my assessment is that it is a relatively low risk -- it basically just removes things from the cache; I don't think this can cause a problem. The only impact might be on cycle detection, but we only remove things from the cache "on the way up", so cycles would still be detected. (And anyway I don't think we rely on this at present.) Somebody want to agree to the backport? cc @rust-lang/compiler |
|
SGTM. |
|
agree to backport. |
|
agree to backport |
|
discussed in compiler team mtg. its a kinda non-trivial backport, and at this point the regression has reached the point of being stable->stable. so the question is: Is it all that bad to leave the ICE in for one more cycle? Some members of the team (myself included) expressed indifference towards backport vs letting trains handle it. |
No strong opinion. @brson, your thoughts? |
|
I'll see if it cherry-picks and passes the test suite, and if not just leave it for next cycle. |
|
By 'non-trivial' you must have meant it won't cherry-pick, because it doesn't. Let's just leave it. |
brson
removed
the
beta-nominated
This addresses #37154 (a regression). The projection cache was incorrectly caching the results for skolemized regions -- when we pop skolemized regions, we are supposed to drop cache keys for them (just as we remove those skolemized regions from the region inference graph). This is because those skolemized region numbers will be reused later with different meaning (and we have determined that the old ones don't leak out in any meaningful way).
I did a somewhat aggressive fix here of only removing keys that mention the skolemized regions. One could imagine just removing all keys added since we started the skolemization (as indeed I did in my initial commit). This more aggressive fix required fixing a latent bug in
TypeFlags, as an aside.I believe the more aggressive fix is correct; clearly there can be entries that are unrelated to the skoelemized region, and it's a shame to remove them. My one concern was that it is possible I believe to have some region variables that are created and related to skolemized regions, and maybe some of them could end up in the cache. However, that seems harmless enough to me-- those relations will be removed, and couldn't have impacted how trait resolution proceeded anyway (iow, the cache entry is not wrong, though it is kind of useless).
r? @pnkfelix
cc @arielb1