Fixes #26681: don't recompute an already-current selection type when inlining - #26682
Fixes #26681: don't recompute an already-current selection type when inlining#26682soronpo wants to merge 2 commits into
Conversation
|
|
|
Already signed the CLA. There is a bug in the system. |
|
There is a related issue that required adding this to class Box[T]
class Owner:
val w: Int = 8
val b: Box[w.type] = new Box[w.type]
def unstable: Owner = new Owner
transparent inline def f(inline x: Any): Any =
inline x match
case _ => 1
def t = f(unstable.b)
Recompiling the pickled output with |
@soronpo There is no bug. The commits on your branch are not signed by your account, and rather - by Claude. You are expected to own the code you contribute, including generated code, as per our policy. |
58f9dbc to
5113693
Compare
tanishiking
left a comment
tanishiking
left a comment
There was a problem hiding this comment.
I left several comments, but the main thing is that, is there a reason not to work on making selectionType idempotent, rather than special handling on call-site?
| // But skip it when it cannot select anything else than what we already have: for an | ||
| // unstable prefix `selectionType` creates a *fresh* skolem on every call (see | ||
| // `maybeSkolemizePrefix`), and since the `NamedType` it returns is shared, computing | ||
| // it again silently changes the meaning of the types that were derived from the | ||
| // previous denotation. That is how the same selection ended up carrying two different | ||
| // skolems in i26681. |
There was a problem hiding this comment.
It looks pretty ad-hoc for this specific case, if the problem is selectionType creates different skolem, can't we make those methods idempotent? Why Inliner.typedSelect special? I guess all other selectionType call-sites also have the same foot-gun.
| // skolems in i26681. | ||
| val reselectedType = tree.typeOpt match | ||
| case tpe: NamedType | ||
| if tpe.denotationIsCurrent |
There was a problem hiding this comment.
Why this condition is required?
| case tpe: NamedType | ||
| if tpe.denotationIsCurrent | ||
| && !TypeOps.isLegalPrefix(tpe.prefix) | ||
| && tpe.prefix == qual1.tpe.widenIfUnstable => tpe |
There was a problem hiding this comment.
I believe PR description says that when we encounter the same selection path, we should keep the existing denotation that already contains the skolem.
However, this condition seems weaker than that, it only checks whether the prefix type is the same after widening. When make different unstable prefixes end up as the same type, this condition can't distinguish between them, and I'm worried it might end up wrongly reusing a denotation (contains skolems) meant for a different qualifier. Why is this condition sufficient?
`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.
5113693 to
638cf54
Compare
Yes, it's unsound. Added a neg test that would have compiled successfully otherwise. |
|
@soronpo Thanks, it's unsound because it seems you memoizing What I meant is, reuse the skolem when retyping the same tree (because the problem is same tree is allocated different skolem for typing in different phases), and still allocate a fresh one for distinct tree occurrences. The key is the tree, not the prefix type. I tried that in #26710 by storing the |
3 participants
TypeAssigner.selectionTypeskolemizes an unstable qualifier with a freshQualSkolemTypeon every call, and stores the resulting info in the denotation of the (shared, hash-consed)NamedTypeit returns.Inliner.InlineTyper.typedSelectre-ranselectionTypeon 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 forOwner#b: one at the call site, one when the inline match selector was retyped, and one when the reducedval $scrutinee1 = unstable.bwas retyped. The inline match reduction instantiated the pattern-bound typetfrom the second, while the scrutinee's type read back through the third, so the reduced body failed to typecheck withFound: Box[(?1.w : Int)] / Required: Box[(?2.w : Int)].Only refresh the denotation when the selection does not already have one computed in this run, which keeps the fix for i22070 while leaving an existing denotation untouched.
Fixes #26681
Have you relied on LLM-based tools in this contribution?
Yes, and I checked the output by using a local snapshot on my library that surfaced this bug.
How was the solution tested?
New automated tests (including the issue's reproducer, if applicable)