Prevent caching normalization results with a cycle #80246
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This avoid the hang/oom from rust-lang#79714
When normalizing a projection which results in a cycle, we would cache the result of `project_type` without the nested obligations (because they're not needed for inference). This would result in the nested obligations only being handled once in fulfill, which would avoid the cycle error. Fixes rust-lang#79714, a regresion from rust-lang#79305 caused by the removal of `get_paranoid_cache_value_obligation`.
matthewjasper
added
beta-nominated
rust-highfive
added
the
S-waiting-on-review
bjorn3
reviewed
Dec 24, 2020
| @@ -90,6 +90,7 @@ impl ProjectionCacheKey<'tcx> { | |||
| pub enum ProjectionCacheEntry<'tcx> { | |||
| InProgress, | |||
| Ambiguous, | |||
| Recur, | |||
There was a problem hiding this comment.
Suggested change
| Recur, | |
| Recursive, |
I had to read the doc comment on the recur method to understand what this meant.
bjorn3
reviewed
Dec 24, 2020
| /// Indicates that while trying to normalize `key`, `key` was required to | ||
| /// be normalized again. Selection or evaluation should eventually report | ||
| /// an error here. | ||
| pub fn recur(&mut self, key: ProjectionCacheKey<'tcx>) { |
There was a problem hiding this comment.
Suggested change
| pub fn recur(&mut self, key: ProjectionCacheKey<'tcx>) { | |
| pub fn recursive(&mut self, key: ProjectionCacheKey<'tcx>) { |
Mark-Simulacrum
added
the
beta-accepted
|
@bors r+ p=5 rollup=never I would like this to get some testing on master before we promote it to beta (and in short order, beta to stable). The patch has received some amount of eyes on it. |
|
📌 Commit 2e92b13 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Merged
|
Backported to 1.49 in #80417, but leaving accepted for beta backport to 1.50 (this landed after that branched). |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 28, 2020
…ulacrum [beta] backports This backports the following to 1.49: * Revert change to trait evaluation order rust-lang#80132 * Don't allow `const` to begin a nonterminal rust-lang#80135 * Prevent caching normalization results with a cycle rust-lang#80246 r? `@Mark-Simulacrum`
Mark-Simulacrum
removed
the
beta-nominated
Merged
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 31, 2020
…ulacrum [beta] backports This backports accepted PRs and switches to bootstrapping from the released compiler: * de-stabilize unsized raw ptr methods for Weak rust-lang#80422 * Use package name for top-level directory in bare tarballs rust-lang#80397 * Prevent caching normalization results with a cycle rust-lang#80246 r? `@Mark-Simulacrum`
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Jan 28, 2021
…=nikomatsakis Make hitting the recursion limit in projection non-fatal This change was originally made in rust-lang#80246 to avoid future (effectively) infinite loop bugs in projections, but wundergraph relies on rustc recovering here. cc rust-lang#80953 r? `@nikomatsakis`
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 28, 2021
…ikomatsakis Make hitting the recursion limit in projection non-fatal This change was originally made in rust-lang#80246 to avoid future (effectively) infinite loop bugs in projections, but wundergraph relies on rustc recovering here. cc rust-lang#80953 r? `@nikomatsakis`
ehuss
pushed a commit
to ehuss/rust
that referenced
this pull request
Feb 5, 2021
…ikomatsakis Make hitting the recursion limit in projection non-fatal This change was originally made in rust-lang#80246 to avoid future (effectively) infinite loop bugs in projections, but wundergraph relies on rustc recovering here. cc rust-lang#80953 r? `@nikomatsakis`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When normalizing a projection which results in a cycle, we would cache the result of
project_typewithout the nested obligations (because they're not needed for inference). This would result in the nested obligations only being handled once in fulfill, which would avoid the cycle error.get_paranoid_cache_value_obligationused to add an obligation that resulted in a cycle in this case previously, but was removed by #73905.This PR makes the projection cache not cache the value of a projection if it was ever normalized in a cycle (except in a snapshot that's rolled back).
Fixes #79714.
r? @nikomatsakis