WIP: experiment fix #26681 by making maybeSkolemizePrefix idempotent - #26710
Draft
tanishiking wants to merge 3 commits into
Draft
WIP: experiment fix #26681 by making maybeSkolemizePrefix idempotent#26710tanishiking wants to merge 3 commits into
maybeSkolemizePrefix idempotent#26710tanishiking wants to merge 3 commits into
Conversation
`TypeAssigner.selectionType` skolemizes an unstable qualifier with a *fresh* `QualSkolemType` on every call, and stores the resulting info in the denotation of the (shared, hash-consed) `NamedType` it returns. `Inliner.InlineTyper.typedSelect` re-ran `selectionType` on trees that were already typed, so the same selection could end up with a different skolem each time, and the types derived from the previous denotation silently changed meaning. For `f(unstable.b)` in the issue this produced three skolems for `Owner#b`: one at the call site, one when the inline match selector was retyped, and one when the reduced `val $scrutinee1 = unstable.b` was retyped. The reduction instantiated the pattern-bound type `t` from the second while the scrutinee's type read back through the third, so the reduced body failed to typecheck with `Found: Box[(?1.w : Int)] / Required: Box[(?2.w : Int)]`. Skip the recomputation when it provably cannot select anything different from what the tree already has: the denotation is current, the prefix is unchanged, and selecting on that prefix skolemizes it, so the only thing a recomputation could change is the skolem. The refresh that i22070 and i23134 rely on, where the prefix does change while inlining, still happens. `skolemizesPrefix` names that condition rather than spelling it out separately from `maybeSkolemizePrefix`. Note that a fresh skolem per call is not itself the bug: distinct occurrences of a selection on an unstable prefix must not share one, or two evaluations of `unstable` get conflated. Making `maybeSkolemizePrefix` idempotent fixes this issue but is unsound, so the fix is at the point that recomputes needlessly. `NamedType.memberDenot` also recomputed a member as seen from the raw prefix, while the typer computes it as seen from a skolem of the prefix. A recomputed denotation was therefore more approximate than the pickled one -- `Box[(Owner#w : Int)]` came back as `Box[? <: (Owner#w : Int)]` -- and trees did not survive a pickling round-trip. Skolemize there too, so that a selection on an unstable prefix means the same thing compiled from source and from TASTy.
Each selection on an unstable prefix must skolemize that prefix afresh: two evaluations of `unstable` return two different `Owner`s, so their `w` are unrelated and `Box[w.type]` must not unify across occurrences. Nothing in the test suite covered this. Memoizing the `QualSkolemType` per prefix type -- which makes `maybeSkolemizePrefix` idempotent, and is the natural response to the same selection carrying two different skolems in scala#26681 -- makes all three cases here compile, with the whole suite still green. This test makes that regression visible.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #26681
Have you relied on LLM-based tools in this contribution?
Yes, for discuss about idea, and not for codegen
How was the solution tested?
New automated tests (including the issue's reproducer, if applicable)
Covered by existing tests (this is a refactoring)