From 32a753546e0f7ef30e3e9c08b39a503ea93bc95a Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Thu, 24 Nov 2011 05:57:03 +0000 Subject: Reduced accumulation of repackExistentials. Was enjoying watching adriaan go for the record for redundant implementations of repackExistential, but eventually everyone has to join Club Code Reuse. Trimmed 2/3 of the implementations and put the remaining third somewhere it can be enjoyed by all. Continued by tearing apart and reassembling TypeVar. Review by moors. --- src/compiler/scala/reflect/internal/Types.scala | 94 ++++++++++++---------- src/compiler/scala/tools/nsc/Global.scala | 4 + .../scala/tools/nsc/typechecker/Implicits.scala | 6 +- .../tools/nsc/typechecker/PatMatVirtualiser.scala | 16 ---- .../scala/tools/nsc/typechecker/Typers.scala | 1 - 5 files changed, 58 insertions(+), 63 deletions(-) (limited to 'src') diff --git a/src/compiler/scala/reflect/internal/Types.scala b/src/compiler/scala/reflect/internal/Types.scala index a69db8e0f4..a9dded3cd1 100644 --- a/src/compiler/scala/reflect/internal/Types.scala +++ b/src/compiler/scala/reflect/internal/Types.scala @@ -90,6 +90,8 @@ trait Types extends api.Types { self: SymbolTable => /** In case anyone wants to turn off lub verification without reverting anything. */ private final val verifyLubs = true + protected val enableTypeVarExperimentals = settings.Xexperimental.value + /** The current skolemization level, needed for the algorithms * in isSameType, isSubType that do constraint solving under a prefix. */ @@ -2398,6 +2400,30 @@ A type's typeSymbol should never be inspected directly. new TypeVar(origin, constr, args, params) } + // TODO: I don't really know why this happens -- maybe because + // the owner hierarchy changes? the other workaround (besides + // repackExistential) is to explicitly pass expectedTp as the type + // argument for the call to guard, but repacking the existential + // somehow feels more robust + // + // TODO: check if optimization makes a difference, try something else + // if necessary (cache?) + + /** Repack existential types, otherwise they sometimes get unpacked in the + * wrong location (type inference comes up with an unexpected skolem) + */ + def repackExistential(tp: Type): Type = ( + if (tp == NoType) tp + else existentialAbstraction(existentialsInType(tp), tp) + ) + def containsExistential(tpe: Type) = + tpe exists (_.typeSymbol.isExistentiallyBound) + + def existentialsInType(tpe: Type) = ( + for (tp <- tpe ; if tp.typeSymbol.isExistentiallyBound) yield + tp.typeSymbol + ) + /** A class representing a type variable: not used after phase `typer`. * * A higher-kinded TypeVar has params (Symbols) and typeArgs (Types). @@ -2423,22 +2449,15 @@ A type's typeSymbol should never be inspected directly. /** The variable's skolemization level */ val level = skolemizationLevel + // When comparing to types containing skolems, remember the highest level + // of skolemization. If that highest level is higher than our initial + // skolemizationLevel, we can't re-use those skolems as the solution of this + // typevar, which means we'll need to repack our constr.inst into a fresh + // existential. // were we compared to skolems at a higher skolemizationLevel? - // EXPERIMENTAL: will never be true unless settings.Xexperimental.value + // EXPERIMENTAL: value will not be considered unless enableTypeVarExperimentals is true private var encounteredHigherLevel = false - - // set `encounteredHigherLevel` if sym.asInstanceOf[TypeSkolem].level > level - private def updateEncounteredHigherLevel(sym: Symbol): Unit = - sym match { - case ts: TypeSkolem if ts.level > level => encounteredHigherLevel = true - case _ => - } - - // if we were compared against later typeskolems, repack the existential, - // because skolems are only compatible if they were created at the same level - private def repackExistential(tp: Type): Type = if(!encounteredHigherLevel) tp - else existentialAbstraction((tp filter {t => t.typeSymbol.isExistentiallyBound}) map (_.typeSymbol), tp) - + private def shouldRepackType = enableTypeVarExperimentals && encounteredHigherLevel /** Two occurrences of a higher-kinded typevar, e.g. `?CC[Int]` and `?CC[String]`, correspond to * ''two instances'' of `TypeVar` that share the ''same'' `TypeConstraint`. @@ -2466,7 +2485,9 @@ A type's typeSymbol should never be inspected directly. def setInst(tp: Type) { // assert(!(tp containsTp this), this) undoLog record this - constr.inst = repackExistential(tp) + // if we were compared against later typeskolems, repack the existential, + // because skolems are only compatible if they were created at the same level + constr.inst = if (shouldRepackType) repackExistential(tp) else tp } def addLoBound(tp: Type, isNumericBound: Boolean = false) { @@ -2593,8 +2614,6 @@ A type's typeSymbol should never be inspected directly. else if (constr.instValid) // type var is already set checkSubtype(tp, constr.inst) else isRelatable(tp) && { - // registerSkolemizationLevel checks for type skolems which cannot be understood at this level - registerSkolemizationLevel(tp) unifySimple || unifyFull(tp) || ( // only look harder if our gaze is oriented toward Any isLowerBound && ( @@ -2617,12 +2636,8 @@ A type's typeSymbol should never be inspected directly. if (suspended) tp =:= origin else if (constr.instValid) checkIsSameType(tp) else isRelatable(tp) && { - registerSkolemizationLevel(tp) val newInst = wildcardToTypeVarMap(tp) - if (constr.isWithinBounds(newInst)) { - setInst(tp) - true - } else false + (constr isWithinBounds newInst) && { setInst(tp); true } } } @@ -2636,32 +2651,23 @@ A type's typeSymbol should never be inspected directly. registerBound(HasTypeMember(sym.name.toTypeName, tp), false) } + private def isSkolemAboveLevel(tp: Type) = tp.typeSymbol match { + case ts: TypeSkolem => ts.level > level + case _ => false + } + // side-effects encounteredHigherLevel + private def containsSkolemAboveLevel(tp: Type) = + (tp exists isSkolemAboveLevel) && { encounteredHigherLevel = true ; true } + /** Can this variable be related in a constraint to type `tp`? * This is not the case if `tp` contains type skolems whose * skolemization level is higher than the level of this variable. - * - * EXPERIMENTAL: always say we're relatable, track whether we need to deal with the consquences (registerSkolemizationLevel) */ - def isRelatable(tp: Type): Boolean = (settings.Xexperimental.value || - !tp.exists { t => - t.typeSymbol match { - case ts: TypeSkolem => ts.level > level - case _ => false - } - }) - - /** When comparing to types containing skolems, remember the highest level of skolemization - * - * If that highest level is higher than our initial skolemizationLevel, - * we can't re-use those skolems as the solution of this typevar, - * so repack them in a fresh existential. - */ - def registerSkolemizationLevel(tp: Type): Unit = if (settings.Xexperimental.value) { - // don't care about the result, just stop as soon as encounteredHigherLevel == true, - // which means we'll need to repack our constr.inst into a fresh existential - encounteredHigherLevel || tp.exists { t => updateEncounteredHigherLevel(t.typeSymbol); encounteredHigherLevel } - } - + def isRelatable(tp: Type) = ( + shouldRepackType // short circuit if we already know we've seen higher levels + || !containsSkolemAboveLevel(tp) // side-effects tracking boolean + || enableTypeVarExperimentals // -Xexperimental: always say we're relatable, track consequences + ) override val isHigherKinded = typeArgs.isEmpty && params.nonEmpty override def normalize: Type = diff --git a/src/compiler/scala/tools/nsc/Global.scala b/src/compiler/scala/tools/nsc/Global.scala index 947a31cd7a..b4f14dd21b 100644 --- a/src/compiler/scala/tools/nsc/Global.scala +++ b/src/compiler/scala/tools/nsc/Global.scala @@ -307,6 +307,10 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb // to create it on that side. For this one my strategy is a constant def at the file // where I need it, and then an override in Global with the setting. override protected val etaExpandKeepsStar = settings.etaExpandKeepsStar.value + // Here comes another one... + override protected val enableTypeVarExperimentals = ( + settings.Xexperimental.value || settings.YvirtPatmat.value + ) // True if -Xscript has been set, indicating a script run. def isScriptRun = opt.script.isDefined diff --git a/src/compiler/scala/tools/nsc/typechecker/Implicits.scala b/src/compiler/scala/tools/nsc/typechecker/Implicits.scala index 08dd44dcf4..92be241951 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Implicits.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Implicits.scala @@ -1149,8 +1149,10 @@ trait Implicits { val tp1 = tp0.normalize tp1 match { - case ThisType(_) | SingleType(_, _) if !(tp1 exists {tp => tp.typeSymbol.isExistentiallyBound}) => // can't generate a reference to a value that's abstracted over by an existential - manifestFactoryCall("singleType", tp, gen.mkAttributedQualifier(tp1)) + case ThisType(_) | SingleType(_, _) => + // can't generate a reference to a value that's abstracted over by an existential + if (containsExistential(tp1)) EmptyTree + else manifestFactoryCall("singleType", tp, gen.mkAttributedQualifier(tp1)) case ConstantType(value) => manifestOfType(tp1.deconst, full) case TypeRef(pre, sym, args) => diff --git a/src/compiler/scala/tools/nsc/typechecker/PatMatVirtualiser.scala b/src/compiler/scala/tools/nsc/typechecker/PatMatVirtualiser.scala index 76d3a58597..0464741d0a 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatMatVirtualiser.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatMatVirtualiser.scala @@ -845,22 +845,6 @@ trait PatMatVirtualiser extends ast.TreeDSL { self: Analyzer => new TermSymbol(NoSymbol, pos, vpmName.counted(prefix, ctr)) setInfo repackExistential(tp) } - // repack existential types, otherwise they sometimes get unpacked in the wrong location (type inference comes up with an unexpected skolem) - // TODO: I don't really know why this happens -- maybe because the owner hierarchy changes? - // the other workaround (besides repackExistential) is to explicitly pass expectedTp as the type argument for the call to guard, but repacking the existential somehow feels more robust - // TODO: check if optimization makes a difference, try something else if necessary (cache?) - def repackExistential(tp: Type): Type = if(tp == NoType) tp - else { - val existentials = new collection.mutable.ListBuffer[Symbol] - tp foreach { t => - val sym = t.typeSymbol - if (sym.isExistentiallyBound) existentials += sym - } - if (existentials isEmpty) tp - else existentialAbstraction(existentials toList, tp) - // existentialAbstraction((tp filter {t => t.typeSymbol.isExistentiallyBound}) map (_.typeSymbol), tp) - } - object vpmName { val caseResult = "caseResult".toTermName val drop = "drop".toTermName diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index ee1d29a07d..4da98d0fef 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -3214,7 +3214,6 @@ trait Typers extends Modes with Adaptations with PatMatVirtualiser { } treeCopy.Match(tree, selector1, cases1) setType owntype } else { // don't run translator after typers (see comments in PatMatVirtualiser) - def repackExistential(tp: Type): Type = existentialAbstraction((tp filter {t => t.typeSymbol.isExistentiallyBound}) map (_.typeSymbol), tp) val (owntype0, needAdapt) = ptOrLub(cases1 map (x => repackExistential(x.tpe))) val owntype = elimAnonymousClass(owntype0) if (needAdapt) cases1 = cases1 map (adaptCase(_, owntype)) -- cgit v1.2.3