From c61892842cebdee0dbb0c7a80bb468ae20ea57e1 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 22 Jun 2015 11:08:22 +0200 Subject: [PATCH 1/4] Avoid junk produced by Constraint#replace. There were two instances where a constraint undergoing a replace would still refer to poly params that are no longer bound after the replace. 1. In an ordering the replaced parameters was n ot removed from the bounds of the others. 2. When a parameter refers to the replaced parameter in a type, (not a TypeBounds), the replaced parameter was not replaced. We now have checking in place that in globally committable typer states, TypeVars are not instantiated to PolyParams and (configurable) that constraints of such typer states are always closed. Fixes #670. --- src/dotty/tools/dotc/config/Config.scala | 3 ++ src/dotty/tools/dotc/core/Constraint.scala | 3 ++ .../tools/dotc/core/OrderingConstraint.scala | 30 ++++++++++++++++--- src/dotty/tools/dotc/core/TyperState.scala | 8 +++-- src/dotty/tools/dotc/core/Types.scala | 3 ++ tests/{pending => }/pos/t8230a.scala | 10 +++---- 6 files changed, 46 insertions(+), 11 deletions(-) rename tests/{pending => }/pos/t8230a.scala (71%) diff --git a/src/dotty/tools/dotc/config/Config.scala b/src/dotty/tools/dotc/config/Config.scala index 782a2f2d369e..9e9974bdce85 100644 --- a/src/dotty/tools/dotc/config/Config.scala +++ b/src/dotty/tools/dotc/config/Config.scala @@ -32,6 +32,9 @@ object Config { */ final val checkConstraintsPropagated = false + /** Check that constraints of globally committable typer states are closed */ + final val checkConstraintsClosed = true + /** Check that no type appearing as the info of a SymDenotation contains * skolem types. */ diff --git a/src/dotty/tools/dotc/core/Constraint.scala b/src/dotty/tools/dotc/core/Constraint.scala index 5a758f1443be..19f93ce47211 100644 --- a/src/dotty/tools/dotc/core/Constraint.scala +++ b/src/dotty/tools/dotc/core/Constraint.scala @@ -146,4 +146,7 @@ abstract class Constraint extends Showable { /** Check that no constrained parameter contains itself as a bound */ def checkNonCyclic()(implicit ctx: Context): Unit + + /** Check that constraint only refers to PolyParams bound by itself */ + def checkClosed()(implicit ctx: Context): Unit } diff --git a/src/dotty/tools/dotc/core/OrderingConstraint.scala b/src/dotty/tools/dotc/core/OrderingConstraint.scala index 21d003451fc0..4c52f58e830e 100644 --- a/src/dotty/tools/dotc/core/OrderingConstraint.scala +++ b/src/dotty/tools/dotc/core/OrderingConstraint.scala @@ -399,7 +399,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds, def removeParam(ps: List[PolyParam]) = ps.filterNot(p => p.binder.eq(poly) && p.paramNum == idx) - def replaceParam(tp: Type, atPoly: PolyType, atIdx: Int) = tp match { + def replaceParam(tp: Type, atPoly: PolyType, atIdx: Int): Type = tp match { case bounds @ TypeBounds(lo, hi) => def recombine(andor: AndOrType, op: (Type, Boolean) => Type, isUpper: Boolean): Type = { @@ -424,7 +424,8 @@ class OrderingConstraint(private val boundsMap: ParamBounds, } bounds.derivedTypeBounds(replaceIn(lo, isUpper = false), replaceIn(hi, isUpper = true)) - case _ => tp + case _ => + tp.substParam(param, replacement) } var current = @@ -438,8 +439,16 @@ class OrderingConstraint(private val boundsMap: ParamBounds, } } - def remove(pt: PolyType)(implicit ctx: Context): This = - newConstraint(boundsMap.remove(pt), lowerMap.remove(pt), upperMap.remove(pt)) + def remove(pt: PolyType)(implicit ctx: Context): This = { + def removeFromOrdering(po: ParamOrdering) = { + def removeFromBoundss(key: PolyType, bndss: Array[List[PolyParam]]): Array[List[PolyParam]] = { + val bndss1 = bndss.map(bnds => bnds.filterConserve(_.binder ne pt)) + if ((0 until bndss.length).forall(i => bndss(i) eq bndss1(i))) bndss else bndss1 + } + po.remove(pt).mapValues(removeFromBoundss) + } + newConstraint(boundsMap.remove(pt), removeFromOrdering(lowerMap), removeFromOrdering(upperMap)) + } def isRemovable(pt: PolyType, removedParam: Int = -1): Boolean = { val entries = boundsMap(pt) @@ -491,6 +500,19 @@ class OrderingConstraint(private val boundsMap: ParamBounds, } } + override def checkClosed()(implicit ctx: Context): Unit = { + def isFreePolyParam(tp: Type) = tp match { + case PolyParam(binder, _) => !contains(binder) + case _ => false + } + def checkClosedType(tp: Type, where: String) = + if (tp != null) + assert(!tp.existsPart(isFreePolyParam), i"unclosed constraint: $this refers to $tp in $where") + boundsMap.foreachBinding((_, tps) => tps.foreach(checkClosedType(_, "bounds"))) + lowerMap.foreachBinding((_, paramss) => paramss.foreach(_.foreach(checkClosedType(_, "lower")))) + upperMap.foreachBinding((_, paramss) => paramss.foreach(_.foreach(checkClosedType(_, "upper")))) + } + private var myUninstVars: mutable.ArrayBuffer[TypeVar] = _ /** The uninstantiated typevars of this constraint */ diff --git a/src/dotty/tools/dotc/core/TyperState.scala b/src/dotty/tools/dotc/core/TyperState.scala index 91cda1dd824a..ba7a6a8063fd 100644 --- a/src/dotty/tools/dotc/core/TyperState.scala +++ b/src/dotty/tools/dotc/core/TyperState.scala @@ -9,6 +9,7 @@ import util.{SimpleMap, DotClass} import reporting._ import printing.{Showable, Printer} import printing.Texts._ +import config.Config import collection.mutable class TyperState(r: Reporter) extends DotClass with Showable { @@ -19,7 +20,7 @@ class TyperState(r: Reporter) extends DotClass with Showable { /** The current constraint set */ def constraint: Constraint = new OrderingConstraint(SimpleMap.Empty, SimpleMap.Empty, SimpleMap.Empty) - def constraint_=(c: Constraint): Unit = {} + def constraint_=(c: Constraint)(implicit ctx: Context): Unit = {} /** The uninstantiated variables */ def uninstVars = constraint.uninstVars @@ -85,7 +86,10 @@ extends TyperState(r) { private var myConstraint: Constraint = previous.constraint override def constraint = myConstraint - override def constraint_=(c: Constraint) = myConstraint = c + override def constraint_=(c: Constraint)(implicit ctx: Context) = { + if (Config.checkConstraintsClosed && isGlobalCommittable) c.checkClosed() + myConstraint = c + } private var myEphemeral: Boolean = previous.ephemeral diff --git a/src/dotty/tools/dotc/core/Types.scala b/src/dotty/tools/dotc/core/Types.scala index 026e695395f6..1270466e9cb1 100644 --- a/src/dotty/tools/dotc/core/Types.scala +++ b/src/dotty/tools/dotc/core/Types.scala @@ -2458,6 +2458,9 @@ object Types { if (fromBelow && isOrType(inst) && isFullyDefined(inst) && !isOrType(upperBound)) inst = inst.approximateUnion + if (ctx.typerState.isGlobalCommittable) + assert(!inst.isInstanceOf[PolyParam], i"bad inst $this := $inst, constr = ${ctx.typerState.constraint}") + instantiateWith(inst) } diff --git a/tests/pending/pos/t8230a.scala b/tests/pos/t8230a.scala similarity index 71% rename from tests/pending/pos/t8230a.scala rename to tests/pos/t8230a.scala index 405aa86f552c..f878eacf894b 100644 --- a/tests/pending/pos/t8230a.scala +++ b/tests/pos/t8230a.scala @@ -15,12 +15,12 @@ object Test { object Okay { Arr("1") - import I.{ arrToTrav, longArrToTrav } - foo(Arr("2")) - } + import I.{ arrToTrav, longArrToTrav } + val x = foo(Arr("2")) + } object Fail { - import I.arrToTrav - foo(Arr("3")) // found String, expected Long +// import I.arrToTrav +// foo(Arr("3")) // found String, expected Long } } From d5b14bf614fe9f25d10f63b71ca081a828562169 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 23 Jun 2015 10:39:10 +0200 Subject: [PATCH 2/4] Revert test to original Uncomment two lines that were commented out by accident. --- tests/pos/t8230a.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/pos/t8230a.scala b/tests/pos/t8230a.scala index f878eacf894b..dfbae51eeee2 100644 --- a/tests/pos/t8230a.scala +++ b/tests/pos/t8230a.scala @@ -20,7 +20,7 @@ object Test { } object Fail { -// import I.arrToTrav -// foo(Arr("3")) // found String, expected Long + import I.arrToTrav + foo(Arr("3")) // found String, expected Long } } From 53996d2952ddaeae9fd80a5ad79452d56be72678 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 23 Jun 2015 10:39:33 +0200 Subject: [PATCH 3/4] Polish code. Find a nicer way to express the same logic. --- src/dotty/tools/dotc/core/OrderingConstraint.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dotty/tools/dotc/core/OrderingConstraint.scala b/src/dotty/tools/dotc/core/OrderingConstraint.scala index 4c52f58e830e..115d0f8c0ef8 100644 --- a/src/dotty/tools/dotc/core/OrderingConstraint.scala +++ b/src/dotty/tools/dotc/core/OrderingConstraint.scala @@ -442,8 +442,8 @@ class OrderingConstraint(private val boundsMap: ParamBounds, def remove(pt: PolyType)(implicit ctx: Context): This = { def removeFromOrdering(po: ParamOrdering) = { def removeFromBoundss(key: PolyType, bndss: Array[List[PolyParam]]): Array[List[PolyParam]] = { - val bndss1 = bndss.map(bnds => bnds.filterConserve(_.binder ne pt)) - if ((0 until bndss.length).forall(i => bndss(i) eq bndss1(i))) bndss else bndss1 + val bndss1 = bndss.map(_.filterConserve(_.binder ne pt)) + if (bndss.corresponds(bndss1)(_ eq _)) bndss else bndss1 } po.remove(pt).mapValues(removeFromBoundss) } From d973e5d15e51aa8a74f4b1141eef6c4064509dd3 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 23 Jun 2015 10:42:33 +0200 Subject: [PATCH 4/4] Rename SimpleMap#mapValues -> mapValuesNow The operation on SimpleMap is eager. As suggested by @retronym we should find a name different from the lazy Map#mapValues operation. --- src/dotty/tools/dotc/core/OrderingConstraint.scala | 2 +- src/dotty/tools/dotc/util/SimpleMap.scala | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/dotty/tools/dotc/core/OrderingConstraint.scala b/src/dotty/tools/dotc/core/OrderingConstraint.scala index 115d0f8c0ef8..7e27ee628174 100644 --- a/src/dotty/tools/dotc/core/OrderingConstraint.scala +++ b/src/dotty/tools/dotc/core/OrderingConstraint.scala @@ -445,7 +445,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds, val bndss1 = bndss.map(_.filterConserve(_.binder ne pt)) if (bndss.corresponds(bndss1)(_ eq _)) bndss else bndss1 } - po.remove(pt).mapValues(removeFromBoundss) + po.remove(pt).mapValuesNow(removeFromBoundss) } newConstraint(boundsMap.remove(pt), removeFromOrdering(lowerMap), removeFromOrdering(upperMap)) } diff --git a/src/dotty/tools/dotc/util/SimpleMap.scala b/src/dotty/tools/dotc/util/SimpleMap.scala index 7bd263f0f35e..b8668d7e439d 100644 --- a/src/dotty/tools/dotc/util/SimpleMap.scala +++ b/src/dotty/tools/dotc/util/SimpleMap.scala @@ -8,7 +8,7 @@ abstract class SimpleMap[K <: AnyRef, +V >: Null <: AnyRef] extends (K => V) { def remove(k: K): SimpleMap[K, V] def updated[V1 >: V <: AnyRef](k: K, v: V1): SimpleMap[K, V1] def contains(k: K): Boolean = apply(k) != null - def mapValues[V1 >: V <: AnyRef](f: (K, V1) => V1): SimpleMap[K, V1] + def mapValuesNow[V1 >: V <: AnyRef](f: (K, V1) => V1): SimpleMap[K, V1] def foreachBinding(f: (K, V) => Unit): Unit def map2[T](f: (K, V) => T): List[T] = { val buf = new ListBuffer[T] @@ -32,7 +32,7 @@ object SimpleMap { def apply(k: AnyRef) = null def remove(k: AnyRef) = this def updated[V1 >: Null <: AnyRef](k: AnyRef, v: V1) = new Map1(k, v) - def mapValues[V1 >: Null <: AnyRef](f: (AnyRef, V1) => V1) = this + def mapValuesNow[V1 >: Null <: AnyRef](f: (AnyRef, V1) => V1) = this def foreachBinding(f: (AnyRef, Null) => Unit) = () } @@ -49,7 +49,7 @@ object SimpleMap { def updated[V1 >: V <: AnyRef](k: K, v: V1) = if (k == k1) new Map1(k, v) else new Map2(k1, v1, k, v) - def mapValues[V1 >: V <: AnyRef](f: (K, V1) => V1) = { + def mapValuesNow[V1 >: V <: AnyRef](f: (K, V1) => V1) = { val w1 = f(k1, v1) if (v1 eq w1) this else new Map1(k1, w1) } @@ -70,7 +70,7 @@ object SimpleMap { if (k == k1) new Map2(k, v, k2, v2) else if (k == k2) new Map2(k1, v1, k, v) else new Map3(k1, v1, k2, v2, k, v) - def mapValues[V1 >: V <: AnyRef](f: (K, V1) => V1) = { + def mapValuesNow[V1 >: V <: AnyRef](f: (K, V1) => V1) = { val w1 = f(k1, v1); val w2 = f(k2, v2) if ((v1 eq w1) && (v2 eq w2)) this else new Map2(k1, w1, k2, w2) @@ -95,7 +95,7 @@ object SimpleMap { else if (k == k2) new Map3(k1, v1, k, v, k3, v3) else if (k == k3) new Map3(k1, v1, k2, v2, k, v) else new Map4(k1, v1, k2, v2, k3, v3, k, v) - def mapValues[V1 >: V <: AnyRef](f: (K, V1) => V1) = { + def mapValuesNow[V1 >: V <: AnyRef](f: (K, V1) => V1) = { val w1 = f(k1, v1); val w2 = f(k2, v2); val w3 = f(k3, v3) if ((v1 eq w1) && (v2 eq w2) && (v3 eq w3)) this else new Map3(k1, w1, k2, w2, k3, w3) @@ -123,7 +123,7 @@ object SimpleMap { else if (k == k3) new Map4(k1, v1, k2, v2, k, v, k4, v4) else if (k == k4) new Map4(k1, v1, k2, v2, k3, v3, k, v) else new MapMore(Array[AnyRef](k1, v1, k2, v2, k3, v3, k4, v4, k, v)) - def mapValues[V1 >: V <: AnyRef](f: (K, V1) => V1) = { + def mapValuesNow[V1 >: V <: AnyRef](f: (K, V1) => V1) = { val w1 = f(k1, v1); val w2 = f(k2, v2); val w3 = f(k3, v3); val w4 = f(k4, v4) if ((v1 eq w1) && (v2 eq w2) && (v3 eq w3) && (v4 eq w4)) this else new Map4(k1, w1, k2, w2, k3, w3, k4, w4) @@ -197,7 +197,7 @@ object SimpleMap { false } - def mapValues[V1 >: V <: AnyRef](f: (K, V1) => V1) = { + def mapValuesNow[V1 >: V <: AnyRef](f: (K, V1) => V1) = { var bindings1: Array[AnyRef] = bindings var i = 0 while (i < bindings.length) {