Skip to content

Commit

Permalink
Change logic to find members of recursive types (#17386)
Browse files Browse the repository at this point in the history
The previous logic used two boolean variables to selectively create
defensive copies. It was quite complicated. The new logic is simpler and
copies less.

- It copies only if the same recursive type is accessed with two
different prefixes. As long as the prefix stays the same,no confusion in
the `substRecThis` is possible.
- It avoids the openedTwice logic that causes defensive copies at all
points in the future. It seems that trick is no longer necessary.

Fixes #17380 by avoiding infinite recursion due to defensive copies.
Fixes #17381 as well.
  • Loading branch information
smarter authored May 5, 2023
2 parents 1854cd0 + 90f4253 commit f7ba81c
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 23 deletions.
36 changes: 13 additions & 23 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -735,33 +735,24 @@ object Types {
// TODO: change tp.parent to nullable or other values
if ((tp.parent: Type | Null) == null) NoDenotation
else if (tp eq pre) go(tp.parent)
else {
else
//println(s"find member $pre . $name in $tp")

// We have to be careful because we might open the same (wrt eq) recursive type
// twice during findMember which risks picking the wrong prefix in the `substRecThis(rt, pre)`
// call below. To avoid this problem we do a defensive copy of the recursive
// type first. But if we do this always we risk being inefficient and we ran into
// stackoverflows when compiling pos/hk.scala under the refinement encoding
// of hk-types. So we only do a copy if the type
// is visited again in a recursive call to `findMember`, as tracked by `tp.opened`.
// Furthermore, if this happens we mark the original recursive type with `openedTwice`
// which means that we always defensively copy the type in the future. This second
// measure is necessary because findMember calls might be cached, so do not
// necessarily appear in nested order.
// Without the `openedTwice` trick, Typer.scala fails to Ycheck
// at phase resolveSuper.
// twice during findMember with two different prefixes, which risks picking the wrong prefix
// in the `substRecThis(rt, pre)` call below. To avoid this problem we do a defensive copy
// of the recursive type if the new prefix `pre` is neq the prefix with which the
// type was previously opened.

val openedPre = tp.openedWithPrefix
val rt =
if (tp.opened) { // defensive copy
tp.openedTwice = true
if openedPre.exists && (openedPre ne pre) then // defensive copy
RecType(rt => tp.parent.substRecThis(tp, rt.recThis))
}
else tp
rt.opened = true
rt.openedWithPrefix = pre
try go(rt.parent).mapInfo(_.substRecThis(rt, pre))
finally
if (!rt.openedTwice) rt.opened = false
}
finally rt.openedWithPrefix = NoType
end goRec

def goRefined(tp: RefinedType) = {
val pdenot = go(tp.parent)
Expand Down Expand Up @@ -3191,9 +3182,8 @@ object Types {
*/
class RecType(parentExp: RecType => Type) extends RefinedOrRecType with BindingType {

// See discussion in findMember#goRec why these vars are needed
private[Types] var opened: Boolean = false
private[Types] var openedTwice: Boolean = false
// See discussion in findMember#goRec why this field is needed
private[Types] var openedWithPrefix: Type = NoType

val parent: Type = parentExp(this: @unchecked)

Expand Down
3 changes: 3 additions & 0 deletions tests/pos/i17380.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class C { type T; type U }

type X = C { type U = T; def u: U } { type T = String }
9 changes: 9 additions & 0 deletions tests/pos/i17381.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import reflect.Selectable.reflectiveSelectable

type Y = { type T = String; def u(): T }

trait Test {

val y1: Y
val y2 = y1.u()
}

0 comments on commit f7ba81c

Please sign in to comment.