From 847530ed30bcd69382d453870923eaa81ca7ceaa Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Sat, 23 May 2020 16:43:48 +0200 Subject: [PATCH] Avoid infinite loop in type variable instantiation Rename `checkNonCyclic` to `occursAtToplevel` and refactor it to return a boolean, use it in `ConstraintHandling#instanceType` to make sure we do not introduce a cycle when instantiating a type variable. Some alternatives I considered: - Run `widenInferred` inside frozen constraints: this prevents `Set[A] | Set[Int]` to be widened to `Set[Int]` after instantiating `A := Int` - Run `widenInferred` with the upper bound of `param` instead of `param` itself as a bound: I think this is still not safe because the upper bound of `param` might recursively refer to `param`, it also breaks type inference of one expression in ZIO. --- .../dotty/tools/dotc/core/Constraint.scala | 6 +++ .../tools/dotc/core/ConstraintHandling.scala | 17 +++++++- .../tools/dotc/core/OrderingConstraint.scala | 41 ++++++++++--------- tests/neg/widenInst-cycle.scala | 12 ++++++ 4 files changed, 55 insertions(+), 21 deletions(-) create mode 100644 tests/neg/widenInst-cycle.scala diff --git a/compiler/src/dotty/tools/dotc/core/Constraint.scala b/compiler/src/dotty/tools/dotc/core/Constraint.scala index d10841530e9c..e3c8d5eeba26 100644 --- a/compiler/src/dotty/tools/dotc/core/Constraint.scala +++ b/compiler/src/dotty/tools/dotc/core/Constraint.scala @@ -153,6 +153,12 @@ abstract class Constraint extends Showable { /** Check that no constrained parameter contains itself as a bound */ def checkNonCyclic()(implicit ctx: Context): this.type + /** Does `param` occur at the toplevel in `tp` ? + * Toplevel means: the type itself or a factor in some + * combination of `&` or `|` types. + */ + def occursAtToplevel(param: TypeParamRef, tp: Type)(using Context): Boolean + /** Check that constraint only refers to TypeParamRefs bound by itself */ def checkClosed()(implicit ctx: Context): Unit diff --git a/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala b/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala index 9d5bd79da935..d0a52808df6a 100644 --- a/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala +++ b/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala @@ -361,8 +361,21 @@ trait ConstraintHandling[AbstractContext] { * is also a singleton type. */ def instanceType(param: TypeParamRef, fromBelow: Boolean)(implicit actx: AbstractContext): Type = { - val inst = approximation(param, fromBelow).simplified - if (fromBelow) widenInferred(inst, param) else inst + val approx = approximation(param, fromBelow).simplified + if (fromBelow) + val widened = widenInferred(approx, param) + // Widening can add extra constraints, in particular the widened type might + // be a type variable which is now instantiated to `param`, and therefore + // cannot be used as an instantiation of `param` without creating a loop. + // If that happens, we run `instanceType` again to find a new instantation. + // (we do not check for non-toplevel occurences: those should never occur + // since `addOneBound` disallows recursive lower bounds). + if constraint.occursAtToplevel(param, widened) then + instanceType(param, fromBelow) + else + widened + else + approx } /** Constraint `c1` subsumes constraint `c2`, if under `c2` as constraint we have diff --git a/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala b/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala index 2b286f05338a..7ebdb04da641 100644 --- a/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala +++ b/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala @@ -300,8 +300,8 @@ class OrderingConstraint(private val boundsMap: ParamBounds, // ---------- Updates ------------------------------------------------------------ /** If `inst` is a TypeBounds, make sure it does not contain toplevel - * references to `param`. Toplevel means: the term itself or a factor in some - * combination of `&` or `|` types. + * references to `param` (see `Constraint#occursAtToplevel` for a definition + * of "toplevel"). * Any such references are replace by `Nothing` in the lower bound and `Any` * in the upper bound. * References can be direct or indirect through instantiations of other @@ -594,33 +594,36 @@ class OrderingConstraint(private val boundsMap: ParamBounds, // ---------- Checking ----------------------------------------------- def checkNonCyclic()(implicit ctx: Context): this.type = - if Config.checkConstraintsNonCyclic then domainParams.foreach(checkNonCyclic) + if Config.checkConstraintsNonCyclic then + domainParams.foreach { param => + val inst = entry(param) + assert(!isLess(param, param), + s"cyclic ordering involving $param in ${this.show}, upper = $inst") + assert(!occursAtToplevel(param, inst), + s"cyclic bound for $param: ${inst.show} in ${this.show}") + } this - private def checkNonCyclic(param: TypeParamRef)(implicit ctx: Context): Unit = - assert(!isLess(param, param), i"cyclic ordering involving $param in $this, upper = ${upper(param)}") + def occursAtToplevel(param: TypeParamRef, inst: Type)(implicit ctx: Context): Boolean = - def recur(tp: Type)(using Context): Unit = tp match + def occurs(tp: Type)(using Context): Boolean = tp match case tp: AndOrType => - recur(tp.tp1) - recur(tp.tp2) + occurs(tp.tp1) || occurs(tp.tp2) case tp: TypeParamRef => - assert(tp ne param, i"cyclic bound for $param: ${entry(param)} in $this") - entry(tp) match - case NoType => - case TypeBounds(lo, hi) => if lo eq hi then recur(lo) - case inst => recur(inst) + (tp eq param) || entry(tp).match + case NoType => false + case TypeBounds(lo, hi) => (lo eq hi) && occurs(lo) + case inst => occurs(inst) case tp: TypeVar => - recur(tp.underlying) + occurs(tp.underlying) case TypeBounds(lo, hi) => - recur(lo) - recur(hi) + occurs(lo) || occurs(hi) case _ => val tp1 = tp.dealias - if tp1 ne tp then recur(tp1) + (tp1 ne tp) && occurs(tp1) - recur(entry(param)) - end checkNonCyclic + occurs(inst) + end occursAtToplevel override def checkClosed()(using Context): Unit = diff --git a/tests/neg/widenInst-cycle.scala b/tests/neg/widenInst-cycle.scala new file mode 100644 index 000000000000..9aa079f257f5 --- /dev/null +++ b/tests/neg/widenInst-cycle.scala @@ -0,0 +1,12 @@ +import scala.reflect.ClassTag + +class Test { + def foo[N >: C | D <: C, C, D](implicit ct: ClassTag[N]): Unit = {} + // This used to lead to an infinite loop, because: + // widenInferred(?C | ?D, ?N) + // returns ?C, with the following extra constraints: + // ?C := ?N + // ?D := ?N + // So we ended up trying to instantiate ?N with ?N. + foo // error +}