-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fixes #26681: don't recompute an already-current selection type when inlining #26682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -963,7 +963,18 @@ class Inliner(val call: tpd.Tree)(using Context): | |
| // For instance in tests/pos/i22070 when we type `Featureful[?]#toFeatures`, | ||
| // `selectionType` will skolemize the prefix, find the denotation, | ||
| // and then set that denotation for the `TermRef(Featureful[?], symbol toFeatures)`. | ||
| val reselectedType = selectionType(tree, qual1) | ||
| // 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. | ||
| val reselectedType = tree.typeOpt match | ||
| case tpe: NamedType | ||
| if tpe.denotationIsCurrent | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this condition is required? |
||
| && skolemizesPrefix(tpe.prefix, tree.name) | ||
| && tpe.prefix == qual1.tpe.widenIfUnstable => tpe | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| case _ => selectionType(tree, qual1) | ||
|
|
||
| def isConcreteImplementationOf(reselected: Symbol, overridden: Symbol)(using Context): Boolean = | ||
| reselected.isTerm | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // https://github.com/scala/scala3/issues/26681 | ||
| // | ||
| // Selecting on an unstable prefix skolemizes that prefix, and each selection must | ||
| // get its *own* skolem: two evaluations of `unstable` return two different `Owner`s, | ||
| // so their `w` are unrelated and `Box[w.type]` must not unify across occurrences. | ||
| // | ||
| // Memoizing the `QualSkolemType` per prefix type would make `maybeSkolemizePrefix` | ||
| // idempotent -- which is tempting, since a fresh skolem per call is what made the | ||
| // same selection carry two different skolems in i26681 -- but it also makes every | ||
| // case below compile. This test pins the property so that stays visible. | ||
| class Box[T](val x: T) | ||
|
|
||
| class Owner: | ||
| val w: Int = 8 | ||
| val b: Box[w.type] = new Box(w) | ||
| def get: Box[w.type] = new Box(w) | ||
| def set(bb: Box[w.type]): Unit = () | ||
|
|
||
| def unstable: Owner = new Owner | ||
|
|
||
| def same[T](a: Box[T], b: Box[T]): Unit = () | ||
|
|
||
| def t1 = same(unstable.b, unstable.b) // error | ||
| def t2 = same(unstable.get, unstable.get) // error | ||
| def t3 = unstable.set(unstable.get) // error |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ok | ||
| ok |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // https://github.com/scala/scala3/issues/26681 | ||
| class Box[T] | ||
|
|
||
| class Owner: | ||
| val w: Int = 8 | ||
| val b: Box[w.type] = new Box[w.type] | ||
|
|
||
| def use[T](b: Box[T]): String = "ok" | ||
|
|
||
| def unstable: Owner = new Owner | ||
|
|
||
| transparent inline def f(inline rhs: Any): Any = | ||
| inline rhs match | ||
| case r: Box[t] => use[t](r) | ||
|
|
||
| @main def Test = | ||
| // `unstable.b` has an unstable prefix, so its type is `Owner#b`, whose info is | ||
| // computed by skolemizing the prefix. Retyping the selection while inlining used | ||
| // to create a second skolem and overwrite the denotation of `Owner#b`, so the type | ||
| // `t` inferred when reducing the inline match no longer matched the scrutinee. | ||
| println(f(unstable.b)) | ||
| val o = unstable | ||
| println(f(o.b)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks pretty ad-hoc for this specific case, if the problem is
selectionTypecreates different skolem, can't we make those methods idempotent? WhyInliner.typedSelectspecial? I guess all otherselectionTypecall-sites also have the same foot-gun.