From d387e8f67a603898dc5be89932f3ac1e916f77ed Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 18 May 2021 14:44:16 +0200 Subject: [PATCH 1/3] Properly re-own type variables when merging constraints The documentation for myOwnedVars used to say that it could contain already instantiated variables because "weak references can have spurious nulls" but I think that's incorrect: the documentation of WeakReference makes no mention of spurious nulls, the object it references should only be replaced by null if it cannot be reached through a non-weak reference. Instead I believe the issue was that `mergeConstraintWith` did not set the `owningState` of type variables that it now owns. --- .../dotty/tools/dotc/core/TyperState.scala | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TyperState.scala b/compiler/src/dotty/tools/dotc/core/TyperState.scala index 170d0dc92d99..0904e5ab7b27 100644 --- a/compiler/src/dotty/tools/dotc/core/TyperState.scala +++ b/compiler/src/dotty/tools/dotc/core/TyperState.scala @@ -79,10 +79,10 @@ 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: + * `tstate.ownedVars.contains(tvar)` iff `tvar.owningState.get eq tstate` */ private var myOwnedVars: TypeVars = _ def ownedVars: TypeVars = myOwnedVars @@ -176,7 +176,7 @@ class TyperState() { 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) @@ -196,12 +196,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) From c47a2d3e74bebebbd9ed3aedc914d00e715a0f5e Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 18 May 2021 18:30:35 +0200 Subject: [PATCH 2/3] Fix constraint merging in FunProto After the previous commit a few tests started failing due to TyperStates attempting to instantiate a type variable with an owningState pointing to a different TyperState. The issue is that after `mergeConstraintWith`, multiple TyperState can own the same type variable. This is fine as long as only one of them is committable since the other ones won't attempt to instantiate any type variable, but that's not necessarily the case in FunProto#typedArgs where we merge the constraints of the FunProto context into the passed context. This commit fixes this by instantiating any type variable in the constraints of the FunProto which do not exist in the passed TyperState before calling `mergeConstraintWith`. This ensures that merging can be done without changing the ownership of any type variable, thus keeping both TyperState safely committable. --- .../dotty/tools/dotc/core/Constraint.scala | 5 ++ .../tools/dotc/core/OrderingConstraint.scala | 6 ++ .../dotty/tools/dotc/core/TyperState.scala | 25 ++++++--- .../dotty/tools/dotc/typer/Implicits.scala | 6 +- .../dotty/tools/dotc/typer/ProtoTypes.scala | 55 ++++++++++++++----- 5 files changed, 73 insertions(+), 24 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Constraint.scala b/compiler/src/dotty/tools/dotc/core/Constraint.scala index 097397375c87..d19d5add778a 100644 --- a/compiler/src/dotty/tools/dotc/core/Constraint.scala +++ b/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 diff --git a/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala b/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala index 0faadac35e36..d6971027682b 100644 --- a/compiler/src/dotty/tools/dotc/core/OrderingConstraint.scala +++ b/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)) diff --git a/compiler/src/dotty/tools/dotc/core/TyperState.scala b/compiler/src/dotty/tools/dotc/core/TyperState.scala index 0904e5ab7b27..3624c36730ea 100644 --- a/compiler/src/dotty/tools/dotc/core/TyperState.scala +++ b/compiler/src/dotty/tools/dotc/core/TyperState.scala @@ -82,7 +82,8 @@ class TyperState() { /** The set of uninstantiated type variables which have this state as their owning state. * * Invariant: - * `tstate.ownedVars.contains(tvar)` iff `tvar.owningState.get eq tstate` + * 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.find(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,6 +169,12 @@ 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) @@ -180,7 +183,15 @@ class TyperState() { 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 diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 7b743aaab49f..7556b689f77f 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/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") diff --git a/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala b/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala index 9f9be499f077..eff4a8cb9c94 100644 --- a/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala +++ b/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. From acc0e9d04a62bd7ee741bf692f37d93d4bd5fd85 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Wed, 26 May 2021 17:18:56 +0200 Subject: [PATCH 3/3] TyperState#ensureNotConflicting: multiple lambdas might conflict The use of find instead of filter appears to be a typo. --- compiler/src/dotty/tools/dotc/core/TyperState.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/core/TyperState.scala b/compiler/src/dotty/tools/dotc/core/TyperState.scala index 3624c36730ea..6fd7f48bc54f 100644 --- a/compiler/src/dotty/tools/dotc/core/TyperState.scala +++ b/compiler/src/dotty/tools/dotc/core/TyperState.scala @@ -159,7 +159,7 @@ class TyperState() { * in this constraint and its predecessors where necessary. */ def ensureNotConflicting(other: Constraint)(using Context): Unit = - val conflicting = constraint.domainLambdas.find(constraint.hasConflictingTypeVarsFor(_, other)) + 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