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.

Author: smarter

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
 

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

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 =
 

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
+}