[DRAFT] Do not rewrite supertypes in AbstractTypeRefining #7720
Conversation
This file contains hidden or 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
The interpreter was returning the wrong value when evaluating a branching descriptor operand to a descriptor cast. Adding tests for this fix also exposed a bug where the validator did not allow for unreachable descriptor operands without also having unreachable ref operands.
Update the cast optimizations in OptimizeInstructions to correctly optimize ref.cast_desc. The optimizations differ from the optimizations of normal casts because they have to be careful not to skip any side effects in the descriptor operand and because type information alone is not sufficient to prove that a cast will succeed.
We previously hit an assertion failure when accessing the GCData of the ref literal because i31 literals do not have GCData.
Keeping descriptor chains valid while rewriting subtyping is complicated. Make it simpler to optimize descriptor and described types by not rewriting supertypes at all in AbstractTypeRefining. Landing this is blocked on making Unsubtyping more efficient so we can use it by default as a follow-up to AbstractTypeRefining to avoid regressions.
kripken
reviewed
Jul 14, 2025
| // supertype, which is not allowed. We could fix this by walking up the | ||
| // supertype chain to find a supertype that is not being rewritten, but | ||
| // changing subtype relationships and keep descriptor chains valid is | ||
| // nontrivial. Instead, avoid changing subtype relationships entirely. |
There was a problem hiding this comment.
Suggested change
| // nontrivial. Instead, avoid changing subtype relationships entirely. | |
| // nontrivial. Instead, avoid changing subtype relationships entirely: | |
| // leave that for Unsubtyping. |
| // C. But if we rewrote C's supertype, C would declare itself to be its own | ||
| // supertype, which is not allowed. We could fix this by walking up the | ||
| // supertype chain to find a supertype that is not being rewritten, but | ||
| // changing subtype relationships and keep descriptor chains valid is |
There was a problem hiding this comment.
Suggested change
| // changing subtype relationships and keep descriptor chains valid is | |
| // changing subtype relationships and keeping descriptor chains valid is |
| } | ||
| return super; | ||
| // We do not want to update subtype relationships. | ||
| return oldType.getDeclaredSuperType(); |
There was a problem hiding this comment.
This is identical to the function being overridden, so it can be removed. We do not need to declare AbstractTypeRefiningTypeMapper at all, I think.
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.
Keeping descriptor chains valid while rewriting subtyping is
complicated. Make it simpler to optimize descriptor and described types
by not rewriting supertypes at all in AbstractTypeRefining.
Landing this is blocked on making Unsubtyping more efficient so we can
use it by default as a follow-up to AbstractTypeRefining to avoid
regressions.