Skip to content

Commit 29bcade

Browse files
author
Adriaan Moors
committed
SI-5189 fixed: safe type infer for constr pattern
several fixes to the standard library due to - the safer type checker this fix gives us (thus, some casts had to be inserted) - SI-5548 - type inference gets a bit more complicated, it needs help (chainl1 in combinator.Parsers) To deal with the type slack between actual (run-time) types and statically known types, for each abstract type T, reflect its variance as a skolem that is upper-bounded by T (covariant position), or lower-bounded by T (contravariant). Consider the following example: class AbsWrapperCov[+A] case class Wrapper[B](x: Wrapped[B]) extends AbsWrapperCov[B] def unwrap[T](x: AbsWrapperCov[T]): Wrapped[T] = x match { case Wrapper(wrapped) => // Wrapper's type parameter must not be assumed to be equal to T, // it's *upper-bounded* by it wrapped // : Wrapped[_ <: T] } this method should type check if and only if Wrapped is covariant in its type parameter before inferring Wrapper's type parameter B from x's type AbsWrapperCov[T], we must take into account that x's actual type is: AbsWrapperCov[Tactual] forSome {type Tactual <: T} since AbsWrapperCov is covariant in A -- in other words, we must not assume we know T exactly, all we know is its upper bound since method application is the only way to generate this slack between run-time and compile-time types (TODO: right!?), we can simply replace skolems that represent method type parameters as seen from the method's body by other skolems that are (upper/lower)-bounded by that type-parameter skolem (depending on the variance position of the skolem in the statically assumed type of the scrutinee, pt) this type slack is introduced by adaptConstrPattern: before it calls inferConstructorInstance, it creates a new context that holds the new existential skolems the context created by adaptConstrPattern must not be a CaseDef, since that confuses instantiateTypeVar and the whole pushTypeBounds/restoreTypeBounds dance (CaseDef contexts remember the bounds of the type params that we clobbered during GADT typing) typedCase deskolemizes the existential skolems back to the method skolems, since they don't serve any further purpose (except confusing the old pattern matcher) typedCase is now better at finding that context (using nextEnclosing)
1 parent 0cffdf3 commit 29bcade

File tree

12 files changed

+180
-31
lines changed

12 files changed

+180
-31
lines changed

src/compiler/scala/reflect/internal/Symbols.scala

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -269,9 +269,9 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
269269
/** Create a new existential type skolem with this symbol its owner,
270270
* based on the given symbol and origin.
271271
*/
272-
def newExistentialSkolem(basis: Symbol, origin: AnyRef): TypeSkolem = {
273-
val skolem = newTypeSkolemSymbol(basis.name.toTypeName, origin, basis.pos, (basis.flags | EXISTENTIAL) & ~PARAM)
274-
skolem setInfo (basis.info cloneInfo skolem)
272+
def newExistentialSkolem(basis: Symbol, origin: AnyRef, name: TypeName = null, info: Type = null): TypeSkolem = {
273+
val skolem = newTypeSkolemSymbol(if (name eq null) basis.name.toTypeName else name, origin, basis.pos, (basis.flags | EXISTENTIAL) & ~PARAM)
274+
skolem setInfo (if (info eq null) basis.info cloneInfo skolem else info)
275275
}
276276

277277
final def newExistential(name: TypeName, pos: Position = NoPosition, newFlags: Long = 0L): Symbol =

src/compiler/scala/tools/nsc/io/Pickler.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ object Pickler {
165165
def pkl[T: Pickler] = implicitly[Pickler[T]]
166166

167167
/** A class represenenting `~`-pairs */
168-
case class ~[S, T](fst: S, snd: T)
168+
case class ~[+S, +T](fst: S, snd: T)
169169

170170
/** A wrapper class to be able to use `~` s an infix method */
171171
class TildeDecorator[S](x: S) {

src/compiler/scala/tools/nsc/typechecker/Infer.scala

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1090,7 +1090,15 @@ trait Infer {
10901090
inferFor(pt.instantiateTypeParams(ptparams, ptparams map (x => WildcardType))) flatMap { targs =>
10911091
val ctorTpInst = tree.tpe.instantiateTypeParams(undetparams, targs)
10921092
val resTpInst = skipImplicit(ctorTpInst.finalResultType)
1093-
val ptvars = ptparams map freshVar
1093+
val ptvars =
1094+
ptparams map {
1095+
// since instantiateTypeVar wants to modify the skolem that corresponds to the method's type parameter,
1096+
// and it uses the TypeVar's origin to locate it, deskolemize the existential skolem to the method tparam skolem
1097+
// (the existential skolem was created by adaptConstrPattern to introduce the type slack necessary to soundly deal with variant type parameters)
1098+
case skolem if skolem.isExistentialSkolem => freshVar(skolem.deSkolemize.asInstanceOf[TypeSymbol])
1099+
case p => freshVar(p)
1100+
}
1101+
10941102
val ptV = pt.instantiateTypeParams(ptparams, ptvars)
10951103

10961104
if (isPopulated(resTpInst, ptV)) {

src/compiler/scala/tools/nsc/typechecker/Typers.scala

Lines changed: 79 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -852,6 +852,33 @@ trait Typers extends Modes with Adaptations with PatMatVirtualiser {
852852
}
853853
}
854854

855+
/**
856+
* To deal with the type slack between actual (run-time) types and statically known types, for each abstract type T,
857+
* reflect its variance as a skolem that is upper-bounded by T (covariant position), or lower-bounded by T (contravariant).
858+
*
859+
* Consider the following example:
860+
*
861+
* class AbsWrapperCov[+A]
862+
* case class Wrapper[B](x: Wrapped[B]) extends AbsWrapperCov[B]
863+
*
864+
* def unwrap[T](x: AbsWrapperCov[T]): Wrapped[T] = x match {
865+
* case Wrapper(wrapped) => // Wrapper's type parameter must not be assumed to be equal to T, it's *upper-bounded* by it
866+
* wrapped // : Wrapped[_ <: T]
867+
* }
868+
*
869+
* this method should type check if and only if Wrapped is covariant in its type parameter
870+
*
871+
* when inferring Wrapper's type parameter B from x's type AbsWrapperCov[T],
872+
* we must take into account that x's actual type is AbsWrapperCov[Tactual] forSome {type Tactual <: T}
873+
* as AbsWrapperCov is covariant in A -- in other words, we must not assume we know T exactly, all we know is its upper bound
874+
*
875+
* since method application is the only way to generate this slack between run-time and compile-time types (TODO: right!?),
876+
* we can simply replace skolems that represent method type parameters as seen from the method's body
877+
* by other skolems that are (upper/lower)-bounded by that type-parameter skolem
878+
* (depending on the variance position of the skolem in the statically assumed type of the scrutinee, pt)
879+
*
880+
* see test/files/../t5189*.scala
881+
*/
855882
def adaptConstrPattern(): Tree = { // (5)
856883
val extractor = tree.symbol.filter(sym => reallyExists(unapplyMember(sym.tpe)))
857884
if (extractor != NoSymbol) {
@@ -865,7 +892,32 @@ trait Typers extends Modes with Adaptations with PatMatVirtualiser {
865892
val tree1 = TypeTree(clazz.primaryConstructor.tpe.asSeenFrom(prefix, clazz.owner))
866893
.setOriginal(tree)
867894

868-
inferConstructorInstance(tree1, clazz.typeParams, pt)
895+
val skolems = new mutable.ListBuffer[TypeSymbol]
896+
object variantToSkolem extends VariantTypeMap {
897+
def apply(tp: Type) = mapOver(tp) match {
898+
case TypeRef(NoPrefix, tpSym, Nil) if variance != 0 && tpSym.isTypeParameterOrSkolem && tpSym.owner.isTerm =>
899+
val bounds = if (variance == 1) TypeBounds.upper(tpSym.tpe) else TypeBounds.lower(tpSym.tpe)
900+
val skolem = context.owner.newExistentialSkolem(tpSym, tpSym, unit.freshTypeName("?"+tpSym.name), bounds)
901+
// println("mapping "+ tpSym +" to "+ skolem + " : "+ bounds +" -- pt= "+ pt)
902+
skolems += skolem
903+
skolem.tpe
904+
case tp1 => tp1
905+
}
906+
}
907+
908+
// have to open up the existential and put the skolems in scope
909+
// can't simply package up pt in an ExistentialType, because that takes us back to square one (List[_ <: T] == List[T] due to covariance)
910+
val ptSafe = variantToSkolem(pt) // TODO: pt.skolemizeExistential(context.owner, tree) ?
911+
val freeVars = skolems.toList
912+
913+
// use "tree" for the context, not context.tree: don't make another CaseDef context,
914+
// as instantiateTypeVar's bounds would end up there
915+
val ctorContext = context.makeNewScope(tree, context.owner)
916+
freeVars foreach ctorContext.scope.enter
917+
newTyper(ctorContext).infer.inferConstructorInstance(tree1, clazz.typeParams, ptSafe)
918+
919+
// tree1's type-slack skolems will be deskolemized (to the method type parameter skolems)
920+
// once the containing CaseDef has been type checked (see typedCase)
869921
tree1
870922
} else {
871923
tree
@@ -1986,15 +2038,35 @@ trait Typers extends Modes with Adaptations with PatMatVirtualiser {
19862038
val guard1: Tree = if (cdef.guard == EmptyTree) EmptyTree
19872039
else typed(cdef.guard, BooleanClass.tpe)
19882040
var body1: Tree = typed(cdef.body, pt)
1989-
if (!context.savedTypeBounds.isEmpty) {
1990-
body1.tpe = context.restoreTypeBounds(body1.tpe)
1991-
if (isFullyDefined(pt) && !(body1.tpe <:< pt)) {
1992-
// @M no need for pt.normalize here, is done in erasure
2041+
2042+
val contextWithTypeBounds = context.nextEnclosing(_.tree.isInstanceOf[CaseDef])
2043+
if (contextWithTypeBounds.savedTypeBounds nonEmpty) {
2044+
body1.tpe = contextWithTypeBounds restoreTypeBounds body1.tpe
2045+
2046+
// insert a cast if something typechecked under the GADT constraints,
2047+
// but not in real life (i.e., now that's we've reset the method's type skolems'
2048+
// infos back to their pre-GADT-constraint state)
2049+
if (isFullyDefined(pt) && !(body1.tpe <:< pt))
19932050
body1 = typedPos(body1.pos)(gen.mkCast(body1, pt))
1994-
}
2051+
19952052
}
2053+
19962054
// body1 = checkNoEscaping.locals(context.scope, pt, body1)
1997-
treeCopy.CaseDef(cdef, pat1, guard1, body1) setType body1.tpe
2055+
val treeWithSkolems = treeCopy.CaseDef(cdef, pat1, guard1, body1) setType body1.tpe
2056+
2057+
// undo adaptConstrPattern's evil deeds, as they confuse the old pattern matcher
2058+
// TODO: Paul, can we do the deskolemization lazily in the old pattern matcher
2059+
object deskolemizeOnce extends TypeMap {
2060+
def apply(tp: Type): Type = mapOver(tp) match {
2061+
case TypeRef(pre, sym, args) if sym.isExistentialSkolem && sym.deSkolemize.isSkolem && sym.deSkolemize.owner.isTerm =>
2062+
typeRef(NoPrefix, sym.deSkolemize, args)
2063+
case tp1 => tp1
2064+
}
2065+
}
2066+
2067+
new TypeMapTreeSubstituter(deskolemizeOnce).traverse(treeWithSkolems)
2068+
2069+
treeWithSkolems // now without skolems, actually
19982070
}
19992071

20002072
def typedCases(cases: List[CaseDef], pattp: Type, pt: Type): List[CaseDef] =

src/library/scala/collection/JavaConversions.scala

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ object JavaConversions {
6969
* @return A Java Iterator view of the argument.
7070
*/
7171
implicit def asJavaIterator[A](it: Iterator[A]): ju.Iterator[A] = it match {
72-
case JIteratorWrapper(wrapped) => wrapped
72+
case JIteratorWrapper(wrapped) => wrapped.asInstanceOf[ju.Iterator[A]]
7373
case _ => IteratorWrapper(it)
7474
}
7575

@@ -87,7 +87,7 @@ object JavaConversions {
8787
* @return A Java Enumeration view of the argument.
8888
*/
8989
implicit def asJavaEnumeration[A](it: Iterator[A]): ju.Enumeration[A] = it match {
90-
case JEnumerationWrapper(wrapped) => wrapped
90+
case JEnumerationWrapper(wrapped) => wrapped.asInstanceOf[ju.Enumeration[A]]
9191
case _ => IteratorWrapper(it)
9292
}
9393

@@ -105,7 +105,7 @@ object JavaConversions {
105105
* @return A Java Iterable view of the argument.
106106
*/
107107
implicit def asJavaIterable[A](i: Iterable[A]): jl.Iterable[A] = i match {
108-
case JIterableWrapper(wrapped) => wrapped
108+
case JIterableWrapper(wrapped) => wrapped.asInstanceOf[jl.Iterable[A]]
109109
case _ => IterableWrapper(i)
110110
}
111111

@@ -121,7 +121,7 @@ object JavaConversions {
121121
* @return A Java Collection view of the argument.
122122
*/
123123
implicit def asJavaCollection[A](it: Iterable[A]): ju.Collection[A] = it match {
124-
case JCollectionWrapper(wrapped) => wrapped
124+
case JCollectionWrapper(wrapped) => wrapped.asInstanceOf[ju.Collection[A]]
125125
case _ => new IterableWrapper(it)
126126
}
127127

@@ -179,7 +179,7 @@ object JavaConversions {
179179
* @return A Java List view of the argument.
180180
*/
181181
implicit def seqAsJavaList[A](seq: Seq[A]): ju.List[A] = seq match {
182-
case JListWrapper(wrapped) => wrapped
182+
case JListWrapper(wrapped) => wrapped.asInstanceOf[ju.List[A]]
183183
case _ => new SeqWrapper(seq)
184184
}
185185

@@ -286,7 +286,7 @@ object JavaConversions {
286286
*/
287287
implicit def mapAsJavaMap[A, B](m: Map[A, B]): ju.Map[A, B] = m match {
288288
//case JConcurrentMapWrapper(wrapped) => wrapped
289-
case JMapWrapper(wrapped) => wrapped
289+
case JMapWrapper(wrapped) => wrapped.asInstanceOf[ju.Map[A, B]]
290290
case _ => new MapWrapper(m)
291291
}
292292

src/library/scala/collection/immutable/IntMap.scala

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -353,19 +353,19 @@ extends AbstractMap[Int, T]
353353
def unionWith[S >: T](that : IntMap[S], f : (Int, S, S) => S) : IntMap[S] = (this, that) match{
354354
case (IntMap.Bin(p1, m1, l1, r1), that@(IntMap.Bin(p2, m2, l2, r2))) =>
355355
if (shorter(m1, m2)) {
356-
if (!hasMatch(p2, p1, m1)) join(p1, this, p2, that);
356+
if (!hasMatch(p2, p1, m1)) join[S](p1, this, p2, that); // TODO: remove [S] when SI-5548 is fixed
357357
else if (zero(p2, m1)) IntMap.Bin(p1, m1, l1.unionWith(that, f), r1);
358358
else IntMap.Bin(p1, m1, l1, r1.unionWith(that, f));
359359
} else if (shorter(m2, m1)){
360-
if (!hasMatch(p1, p2, m2)) join(p1, this, p2, that);
360+
if (!hasMatch(p1, p2, m2)) join[S](p1, this, p2, that); // TODO: remove [S] when SI-5548 is fixed
361361
else if (zero(p1, m2)) IntMap.Bin(p2, m2, this.unionWith(l2, f), r2);
362362
else IntMap.Bin(p2, m2, l2, this.unionWith(r2, f));
363363
}
364364
else {
365365
if (p1 == p2) IntMap.Bin(p1, m1, l1.unionWith(l2,f), r1.unionWith(r2, f));
366-
else join(p1, this, p2, that);
366+
else join[S](p1, this, p2, that); // TODO: remove [S] when SI-5548 is fixed
367367
}
368-
case (IntMap.Tip(key, value), x) => x.updateWith(key, value, (x, y) => f(key, y, x));
368+
case (IntMap.Tip(key, value), x) => x.updateWith[S](key, value, (x, y) => f(key, y, x));
369369
case (x, IntMap.Tip(key, value)) => x.updateWith[S](key, value, (x, y) => f(key, x, y));
370370
case (IntMap.Nil, x) => x;
371371
case (x, IntMap.Nil) => x;

src/library/scala/collection/immutable/LongMap.scala

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -349,19 +349,19 @@ extends AbstractMap[Long, T]
349349
def unionWith[S >: T](that : LongMap[S], f : (Long, S, S) => S) : LongMap[S] = (this, that) match{
350350
case (LongMap.Bin(p1, m1, l1, r1), that@(LongMap.Bin(p2, m2, l2, r2))) =>
351351
if (shorter(m1, m2)) {
352-
if (!hasMatch(p2, p1, m1)) join(p1, this, p2, that);
352+
if (!hasMatch(p2, p1, m1)) join[S](p1, this, p2, that); // TODO: remove [S] when SI-5548 is fixed
353353
else if (zero(p2, m1)) LongMap.Bin(p1, m1, l1.unionWith(that, f), r1);
354354
else LongMap.Bin(p1, m1, l1, r1.unionWith(that, f));
355355
} else if (shorter(m2, m1)){
356-
if (!hasMatch(p1, p2, m2)) join(p1, this, p2, that);
356+
if (!hasMatch(p1, p2, m2)) join[S](p1, this, p2, that); // TODO: remove [S] when SI-5548 is fixed
357357
else if (zero(p1, m2)) LongMap.Bin(p2, m2, this.unionWith(l2, f), r2);
358358
else LongMap.Bin(p2, m2, l2, this.unionWith(r2, f));
359359
}
360360
else {
361361
if (p1 == p2) LongMap.Bin(p1, m1, l1.unionWith(l2,f), r1.unionWith(r2, f));
362-
else join(p1, this, p2, that);
362+
else join[S](p1, this, p2, that); // TODO: remove [S] when SI-5548 is fixed
363363
}
364-
case (LongMap.Tip(key, value), x) => x.updateWith(key, value, (x, y) => f(key, y, x));
364+
case (LongMap.Tip(key, value), x) => x.updateWith[S](key, value, (x, y) => f(key, y, x)); // TODO: remove [S] when SI-5548 is fixed
365365
case (x, LongMap.Tip(key, value)) => x.updateWith[S](key, value, (x, y) => f(key, x, y));
366366
case (LongMap.Nil, x) => x;
367367
case (x, LongMap.Nil) => x;

src/library/scala/util/parsing/combinator/Parsers.scala

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -794,7 +794,7 @@ trait Parsers {
794794
*/
795795
def chainl1[T, U](first: => Parser[T], p: => Parser[U], q: => Parser[(T, U) => T]): Parser[T]
796796
= first ~ rep(q ~ p) ^^ {
797-
case x ~ xs => xs.foldLeft(x){(_, _) match {case (a, f ~ b) => f(a, b)}}
797+
case x ~ xs => xs.foldLeft(x: T){case (a, f ~ b) => f(a, b)} // x's type annotation is needed to deal with changed type inference due to SI-5189
798798
}
799799

800800
/** A parser generator that generalises the `rep1sep` generator so that `q`,
@@ -812,8 +812,7 @@ trait Parsers {
812812
*/
813813
def chainr1[T, U](p: => Parser[T], q: => Parser[(T, U) => U], combine: (T, U) => U, first: U): Parser[U]
814814
= p ~ rep(q ~ p) ^^ {
815-
case x ~ xs => (new ~(combine, x) :: xs).
816-
foldRight(first){(_, _) match {case (f ~ a, b) => f(a, b)}}
815+
case x ~ xs => (new ~(combine, x) :: xs).foldRight(first){case (f ~ a, b) => f(a, b)}
817816
}
818817

819818
/** A parser generator for optional sub-phrases.

src/scalap/scala/tools/scalap/scalax/rules/Rules.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ trait StateRules {
130130
def rep(in : S, t : T) : Result[S, T, X] = {
131131
if (finished(t)) Success(in, t)
132132
else rule(in) match {
133-
case Success(out, f) => rep(out, f(t))
133+
case Success(out, f) => rep(out, f(t)) // SI-5189 f.asInstanceOf[T => T]
134134
case Failure => Failure
135135
case Error(x) => Error(x)
136136
}

test/files/jvm/typerep.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ object TypeRep {
161161
}).asInstanceOf[TypeRep[Option[A]]]
162162

163163
def getType[A](x: List[A])(implicit rep: TypeRep[A]): TypeRep[List[A]] = (x match {
164-
case h :: t => ListRep(getType(h))
164+
case h :: t => ListRep(rep)
165165
case Nil => NilRep
166166
}).asInstanceOf[TypeRep[List[A]]]
167167

0 commit comments

Comments
 (0)