Skip to content

Commit

Permalink
Avoid the TypeVar.inst trap
Browse files Browse the repository at this point in the history
`tvar.inst` gives the _permanent_ instance of a type variable `tvar`. Even if `tvar.isInstantiated` is true
its `inst` can still be NoType. This is a trap that caused a regression in the code of glb. This commit fixes
the regression and introduces different names that will hopefully avoid the trap in the future.

Fixes #20154

[Cherry-picked 2863a29][modified]
  • Loading branch information
WojciechMazur committed Jul 9, 2024
1 parent 1e31b17 commit e2471f7
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 21 deletions.
12 changes: 6 additions & 6 deletions compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds,

override def tyconTypeParams(tp: AppliedType)(using Context): List[ParamInfo] =
def tparams(tycon: Type): List[ParamInfo] = tycon match
case tycon: TypeVar if !tycon.inst.exists => tparams(tycon.origin)
case tycon: TypeVar if !tycon.isPermanentlyInstantiated => tparams(tycon.origin)
case tycon: TypeParamRef if !hasBounds(tycon) =>
val entryParams = entry(tycon).typeParams
if entryParams.nonEmpty then entryParams
Expand Down Expand Up @@ -713,7 +713,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
var newDepEntry = newEntry
replacedTypeVar match
case tvar: TypeVar =>
if tvar.inst.exists // `isInstantiated` would use ctx.typerState.constraint rather than the current constraint
if tvar.isPermanentlyInstantiated // `isInstantiated` would use ctx.typerState.constraint rather than the current constraint
then
// If the type variable has been instantiated, we need to forget about
// the instantiation for old dependencies.
Expand Down Expand Up @@ -770,7 +770,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
@tailrec def allRemovable(last: Int): Boolean =
if (last < 0) true
else typeVar(entries, last) match {
case tv: TypeVar => tv.inst.exists && allRemovable(last - 1)
case tv: TypeVar => tv.isPermanentlyInstantiated && allRemovable(last - 1)
case _ => false
}
allRemovable(paramCount(entries) - 1)
Expand Down Expand Up @@ -876,7 +876,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
val limit = paramCount(entries)
while i < limit do
typeVar(entries, i) match
case tv: TypeVar if !tv.inst.exists => op(tv)
case tv: TypeVar if !tv.isPermanentlyInstantiated => op(tv)
case _ =>
i += 1
}
Expand All @@ -885,12 +885,12 @@ class OrderingConstraint(private val boundsMap: ParamBounds,

/** The uninstantiated typevars of this constraint */
def uninstVars: collection.Seq[TypeVar] = {
if (myUninstVars == null || myUninstVars.uncheckedNN.exists(_.inst.exists)) {
if (myUninstVars == null || myUninstVars.uncheckedNN.exists(_.isPermanentlyInstantiated)) {
myUninstVars = new mutable.ArrayBuffer[TypeVar]
boundsMap.foreachBinding { (poly, entries) =>
for (i <- 0 until paramCount(entries))
typeVar(entries, i) match {
case tv: TypeVar if !tv.inst.exists && isBounds(entries(i)) => myUninstVars.uncheckedNN += tv
case tv: TypeVar if !tv.isPermanentlyInstantiated && isBounds(entries(i)) => myUninstVars.uncheckedNN += tv
case _ =>
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/SymDenotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1596,7 +1596,7 @@ object SymDenotations {
case tp: RefinedType => hasSkolems(tp.parent) || hasSkolems(tp.refinedInfo)
case tp: RecType => hasSkolems(tp.parent)
case tp: TypeBounds => hasSkolems(tp.lo) || hasSkolems(tp.hi)
case tp: TypeVar => hasSkolems(tp.inst)
case tp: TypeVar => hasSkolems(tp.permanentInst)
case tp: ExprType => hasSkolems(tp.resType)
case tp: AppliedType => hasSkolems(tp.tycon) || tp.args.exists(hasSkolems)
case tp: LambdaType => tp.paramInfos.exists(hasSkolems) || hasSkolems(tp.resType)
Expand Down
6 changes: 3 additions & 3 deletions compiler/src/dotty/tools/dotc/core/TyperState.scala
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ class TyperState() {
val tvars = tl.paramRefs.map(other.typeVarOfParam(_)).collect { case tv: TypeVar => tv }
if this.isCommittable then
tvars.foreach(tvar =>
if !tvar.inst.exists && !isOwnedAnywhere(this, tvar) then includeVar(tvar))
if !tvar.isPermanentlyInstantiated && !isOwnedAnywhere(this, tvar) then includeVar(tvar))
typeComparer.addToConstraint(tl, tvars)
}) &&
// Integrate the additional constraints on type variables from `other`
Expand Down Expand Up @@ -286,10 +286,10 @@ class TyperState() {
for tvar <- ownedVars do
val tvarState = tvar.owningState.nn.get
assert(tvarState eqn this, s"Inconsistent state in $this: it owns $tvar whose owningState is ${tvarState}")
assert(!tvar.inst.exists, s"Inconsistent state in $this: it owns $tvar which is already instantiated")
assert(!tvar.isPermanentlyInstantiated, s"Inconsistent state in $this: it owns $tvar which is already instantiated")
val inst = constraint.instType(tvar)
if inst.exists then
tvar.setInst(inst)
tvar.setPermanentInst(inst)
val tl = tvar.origin.binder
if constraint.isRemovable(tl) then toCollect += tl
for tl <- toCollect do
Expand Down
31 changes: 20 additions & 11 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ object Types extends TypeUtils {
case t: AppliedType =>
t.fold(false, (x, tp) => x || test(tp, theAcc))
case t: TypeVar =>
!t.inst.exists || test(t.inst, theAcc)
!t.isPermanentlyInstantiated || test(t.permanentInst, theAcc)
case t: LazyRef =>
!t.completed || test(t.ref, theAcc)
case _ =>
Expand Down Expand Up @@ -4792,20 +4792,24 @@ object Types extends TypeUtils {
def setOrigin(p: TypeParamRef) = currentOrigin = p

/** The permanent instance type of the variable, or NoType is none is given yet */
private var myInst: Type = NoType
private var inst: Type = NoType

private[core] def inst: Type = myInst
private[core] def setInst(tp: Type): Unit =
myInst = tp
/** The permanent instance type that's stored in the type variable, so it cannot be retracted
* anymore, or NoType if the variable can still be further constrained or a provisional
* instance type in the constraint can be retracted.
*/
private[core] def permanentInst = inst
private[core] def setPermanentInst(tp: Type): Unit =
inst = tp
if tp.exists && owningState != null then
val owningState1 = owningState.uncheckedNN.get
if owningState1 != null then
owningState1.ownedVars -= this
owningState = null // no longer needed; null out to avoid a memory leak

private[core] def resetInst(ts: TyperState): Unit =
assert(myInst.exists)
myInst = NoType
assert(inst.exists)
inst = NoType
owningState = new WeakReference(ts)

/** The state owning the variable. This is at first `creatorState`, but it can
Expand Down Expand Up @@ -4843,18 +4847,23 @@ object Types extends TypeUtils {
/** Is the variable already instantiated? */
def isInstantiated(using Context): Boolean = instanceOpt.exists

/** Is the variable already instantiated so that the instance cannot be
* retracted anymore?
*/
def isPermanentlyInstantiated: Boolean = inst.exists

/** Instantiate variable with given type */
def instantiateWith(tp: Type)(using Context): Type = {
assert(tp ne this, i"self instantiation of $origin, constraint = ${ctx.typerState.constraint}")
assert(!myInst.exists, i"$origin is already instantiated to $myInst but we attempted to instantiate it to $tp")
assert(!inst.exists, i"$origin is already instantiated to $inst but we attempted to instantiate it to $tp")
typr.println(i"instantiating $this with $tp")

if Config.checkConstraintsSatisfiable then
assert(currentEntry.bounds.contains(tp),
i"$origin is constrained to be $currentEntry but attempted to instantiate it to $tp")

if ((ctx.typerState eq owningState.nn.get.uncheckedNN) && !TypeComparer.subtypeCheckInProgress)
setInst(tp)
setPermanentInst(tp)
ctx.typerState.constraint = ctx.typerState.constraint.replace(origin, tp)
tp
}
Expand All @@ -4868,8 +4877,8 @@ object Types extends TypeUtils {
*/
def instantiate(fromBelow: Boolean)(using Context): Type =
val tp = TypeComparer.instanceType(origin, fromBelow, widenUnions, nestingLevel)
if myInst.exists then // The line above might have triggered instantiation of the current type variable
myInst
if inst.exists then // The line above might have triggered instantiation of the current type variable
inst
else
instantiateWith(tp)

Expand Down
15 changes: 15 additions & 0 deletions tests/pos/i20154.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
sealed abstract class Kyo[+T, -S]
opaque type <[+T, -S] >: T = T | Kyo[T, S]

abstract class Effect[+E]:
type Command[_]

case class Recurse[Command[_], Result[_], E <: Effect[E], T, S, S2](
h: ResultHandler[Command, Result, E, S],
v: T < (E & S & S2)
)

abstract class ResultHandler[Command[_], Result[_], E <: Effect[E], S]:
opaque type Handle[T, S2] >: (Result[T] < (S & S2)) = Result[T] < (S & S2) | Recurse[Command, Result, E, T, S, S2]

def handle[T, S2](h: ResultHandler[Command, Result, E, S], v: T < (E & S & S2)): Handle[T, S2] = Recurse(h, v)

0 comments on commit e2471f7

Please sign in to comment.