Merge pull request #12519 from dotty-staging/reown-tvars

Properly re-own type variables when merging constraints

Author: smarter

compiler/src/dotty/tools/dotc/core/Constraint.scala

@@ -162,6 +162,11 @@ abstract class Constraint extends Showable {
    */
   def & (other: Constraint, otherHasErrors: Boolean)(using Context): Constraint
 
+  /** Whether `tl` is present in both `this` and `that` but is associated with
+   *  different TypeVars there, meaning that the constraints cannot be merged.
+   */
+  def hasConflictingTypeVarsFor(tl: TypeLambda, that: Constraint): Boolean
+
   /** Check that no constrained parameter contains itself as a bound */
   def checkNonCyclic()(using Context): this.type
 

compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala

@@ -493,6 +493,12 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
         merge(this.upperMap, that.upperMap, mergeParams))
   }.showing(i"constraint merge $this with $other = $result", constr)
 
+  def hasConflictingTypeVarsFor(tl: TypeLambda, that: Constraint): Boolean =
+    contains(tl) && that.contains(tl) &&
+    // Since TypeVars are allocated in bulk for each type lambda, we only have
+    // to check the first one to find out if some of them are different.
+    (this.typeVarOfParam(tl.paramRefs(0)) ne that.typeVarOfParam(tl.paramRefs(0)))
+
   def subst(from: TypeLambda, to: TypeLambda)(using Context): OrderingConstraint =
     def swapKey[T](m: ArrayValuedMap[T]) = m.remove(from).updated(to, m(from))
     var current = newConstraint(swapKey(boundsMap), swapKey(lowerMap), swapKey(upperMap))

compiler/src/dotty/tools/dotc/core/TyperState.scala

@@ -79,10 +79,11 @@ class TyperState() {
 
   private var isCommitted: Boolean = _
 
-  /** The set of uninstantiated type variables which have this state as their owning state
-   *  NOTE: It could be that a variable in `ownedVars` is already instantiated. This is because
-   *  the link between ownedVars and variable instantiation in TypeVar#setInst is made up
-   *  from a weak reference and weak references can have spurious nulls.
+  /** The set of uninstantiated type variables which have this state as their owning state.
+   *
+   *  Invariant:
+   *   if `tstate.isCommittable` then
+   *     `tstate.ownedVars.contains(tvar)` iff `tvar.owningState.get eq tstate`
    */
   private var myOwnedVars: TypeVars = _
   def ownedVars: TypeVars = myOwnedVars
@@ -138,6 +139,7 @@ class TyperState() {
   def commit()(using Context): Unit = {
     Stats.record("typerState.commit")
     assert(isCommittable)
+    setCommittable(false)
     val targetState = ctx.typerState
     if constraint ne targetState.constraint then
       Stats.record("typerState.commit.new constraint")
@@ -157,12 +159,7 @@ class TyperState() {
    *  in this constraint and its predecessors where necessary.
    */
   def ensureNotConflicting(other: Constraint)(using Context): Unit =
-    def hasConflictingTypeVarsFor(tl: TypeLambda) =
-      constraint.typeVarOfParam(tl.paramRefs(0)) ne other.typeVarOfParam(tl.paramRefs(0))
-        // Note: Since TypeVars are allocated in bulk for each type lambda, we only
-        // have to check the first one to find out if some of them are different.
-    val conflicting = constraint.domainLambdas.find(tl =>
-      other.contains(tl) && hasConflictingTypeVarsFor(tl))
+    val conflicting = constraint.domainLambdas.filter(constraint.hasConflictingTypeVarsFor(_, other))
     for tl <- conflicting do
       val tl1 = constraint.ensureFresh(tl)
       for case (tvar: TypeVar, pref1) <- tl.paramRefs.map(constraint.typeVarOfParam).lazyZip(tl1.paramRefs) do
@@ -172,15 +169,29 @@ class TyperState() {
         ts.constraint = ts.constraint.subst(tl, tl1)
         ts = ts.previous
 
+  /** Integrate the constraints from `that` into this TyperState.
+   *
+   *  @pre If `that` is committable, it must not contain any type variable which
+   *       does not exist in `this` (in other words, all its type variables must
+   *       be owned by a common parent of `this` and `that`).
+   */
   def mergeConstraintWith(that: TyperState)(using Context): Unit =
     that.ensureNotConflicting(constraint)
     constraint = constraint & (that.constraint, otherHasErrors = that.reporter.errorsReported)
     for tvar <- constraint.uninstVars do
-      if !isOwnedAnywhere(this, tvar) then ownedVars += tvar
+      if !isOwnedAnywhere(this, tvar) then includeVar(tvar)
     for tl <- constraint.domainLambdas do
       if constraint.isRemovable(tl) then constraint = constraint.remove(tl)
 
+  /** Take ownership of `tvar`.
+   *
+   *  @pre `tvar` is not owned by a committable TyperState. This ensures
+   *       each tvar can only be instantiated by one TyperState.
+   */
   private def includeVar(tvar: TypeVar)(using Context): Unit =
+    val oldState = tvar.owningState.get
+    assert(oldState == null || !oldState.isCommittable,
+      i"$this attempted to take ownership of $tvar which is already owned by committable $oldState")
     tvar.owningState = new WeakReference(this)
     ownedVars += tvar
 
@@ -196,12 +207,13 @@ class TyperState() {
       Stats.record("typerState.gc")
       val toCollect = new mutable.ListBuffer[TypeLambda]
       for tvar <- ownedVars do
-        if !tvar.inst.exists then // See comment of `ownedVars` for why this test is necessary
-          val inst = constraint.instType(tvar)
-          if inst.exists then
-            tvar.setInst(inst)
-            val tl = tvar.origin.binder
-            if constraint.isRemovable(tl) then toCollect += tl
+        assert(tvar.owningState.get eq this, s"Inconsistent state in $this: it owns $tvar whose owningState is ${tvar.owningState.get}")
+        assert(!tvar.inst.exists, s"Inconsistent state in $this: it owns $tvar which is already instantiated")
+        val inst = constraint.instType(tvar)
+        if inst.exists then
+          tvar.setInst(inst)
+          val tl = tvar.origin.binder
+          if constraint.isRemovable(tl) then toCollect += tl
       for tl <- toCollect do
         constraint = constraint.remove(tl)
 

compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -979,8 +979,10 @@ trait Implicits:
       val result =
         result0 match {
           case result: SearchSuccess =>
-            result.tstate.commit()
-            ctx.gadt.restore(result.gstate)
+            if result.tstate ne ctx.typerState then
+              result.tstate.commit()
+            if result.gstate ne ctx.gadt then
+              ctx.gadt.restore(result.gstate)
             if hasSkolem(false, result.tree) then
               report.error(SkolemInInferred(result.tree, pt, argument), ctx.source.atSpan(span))
             implicits.println(i"success: $result")

compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala

@@ -385,21 +385,46 @@ object ProtoTypes {
      *                before it is typed. The second Int parameter is the parameter index.
      */
     def typedArgs(norm: (untpd.Tree, Int) => untpd.Tree = sameTree)(using Context): List[Tree] =
-      if (state.typedArgs.size == args.length) state.typedArgs
-      else {
-        val prevConstraint = protoCtx.typerState.constraint
-
-        try
-          inContext(protoCtx) {
-            val args1 = args.mapWithIndexConserve((arg, idx) =>
-              cacheTypedArg(arg, arg => typer.typed(norm(arg, idx)), force = false))
-            if !args1.exists(arg => isUndefined(arg.tpe)) then state.typedArgs = args1
-            args1
-          }
-        finally
-          if (protoCtx.typerState.constraint ne prevConstraint)
-            ctx.typerState.mergeConstraintWith(protoCtx.typerState)
-      }
+      if state.typedArgs.size == args.length then state.typedArgs
+      else
+        val passedTyperState = ctx.typerState
+        inContext(protoCtx) {
+          val protoTyperState = protoCtx.typerState
+          val oldConstraint = protoTyperState.constraint
+          val args1 = args.mapWithIndexConserve((arg, idx) =>
+            cacheTypedArg(arg, arg => typer.typed(norm(arg, idx)), force = false))
+          val newConstraint = protoTyperState.constraint
+
+          if !args1.exists(arg => isUndefined(arg.tpe)) then state.typedArgs = args1
+
+          // We only need to propagate constraints if we typed the arguments in a different
+          // TyperState and if that created additional constraints.
+          if (passedTyperState ne protoTyperState) && (oldConstraint ne newConstraint) then
+            // To respect the pre-condition of `mergeConstraintWith` and keep
+            // `protoTyperState` committable we must ensure that it does not
+            // contain any type variable which don't already exist in the passed
+            // TyperState. This is achieved by instantiating any such type
+            // variable.
+            if protoTyperState.isCommittable then
+              val passedConstraint = passedTyperState.constraint
+              val newLambdas = newConstraint.domainLambdas.filter(tl =>
+                !passedConstraint.contains(tl) || passedConstraint.hasConflictingTypeVarsFor(tl, newConstraint))
+              val newTvars = newLambdas.flatMap(_.paramRefs).map(newConstraint.typeVarOfParam)
+
+              args1.foreach(arg => Inferencing.instantiateSelected(arg.tpe, newTvars))
+
+              // `instantiateSelected` can leave some type variables uninstantiated,
+              // so we maximize them in a second pass.
+              newTvars.foreach {
+                case tvar: TypeVar if !tvar.isInstantiated =>
+                  tvar.instantiate(fromBelow = false)
+                case _ =>
+              }
+
+            passedTyperState.mergeConstraintWith(protoTyperState)
+          end if
+          args1
+        }
 
     /** Type single argument and remember the unadapted result in `myTypedArg`.
      *  used to avoid repeated typings of trees when backtracking.