From 10aa78db5961284e0f4fb2fcb5a1f3bd7c357385 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Thu, 11 Aug 2016 17:17:38 +1000 Subject: Address review comments - clarify the intent of tests - Consolidate stripExistentialsAndTypeVars with similar logic in mergePrefixAndArgs - Refactor special cases in maybeRewrap The name isn't great, but I'm struggling to come up with a pithy way to describe the rogue band of types. --- src/reflect/scala/reflect/internal/Types.scala | 63 +++++++++++++++------- .../scala/reflect/internal/tpe/GlbLubs.scala | 18 ------- test/files/neg/lub-from-hell-2.scala | 7 +++ test/files/pos/lub-from-hell.scala | 9 +--- 4 files changed, 52 insertions(+), 45 deletions(-) diff --git a/src/reflect/scala/reflect/internal/Types.scala b/src/reflect/scala/reflect/internal/Types.scala index 523cb968e7..34949a2bb8 100644 --- a/src/reflect/scala/reflect/internal/Types.scala +++ b/src/reflect/scala/reflect/internal/Types.scala @@ -172,13 +172,16 @@ trait Types trait RewrappingTypeProxy extends SimpleTypeProxy { protected def maybeRewrap(newtp: Type) = ( if (newtp eq underlying) this - // BoundedWildcardTypes reach here during erroneous compilation: neg/t6258 - // Higher-kinded exclusion is because [x]CC[x] compares =:= to CC: pos/t3800 - // Avoid reusing the existing Wrapped(RefinedType) when we've be asked to wrap an =:= RefinementTypeRef, the - // distinction is important in base type sequences. - // Otherwise, if newtp =:= underlying, don't rewrap it. - else if (!newtp.isWildcard && !newtp.isHigherKinded && !newtp.isInstanceOf[RefinementTypeRef] && (newtp =:= underlying)) this - else rewrap(newtp) + else { + // - BoundedWildcardTypes reach here during erroneous compilation: neg/t6258 + // - Higher-kinded exclusion is because [x]CC[x] compares =:= to CC: pos/t3800 + // - Avoid reusing the existing Wrapped(RefinedType) when we've be asked to wrap an =:= RefinementTypeRef, the + // distinction is important in base type sequences. See TypesTest.testExistentialRefinement + // - Otherwise, if newtp =:= underlying, don't rewrap it. + val hasSpecialMeaningBeyond_=:= = newtp.isWildcard || newtp.isHigherKinded || newtp.isInstanceOf[RefinementTypeRef] + if (!hasSpecialMeaningBeyond_=:= && (newtp =:= underlying)) this + else rewrap(newtp) + } ) protected def rewrap(newtp: Type): Type @@ -1596,7 +1599,6 @@ trait Types (parents forall typeIsHigherKinded) && !phase.erasedTypes ) - override def typeParams = if (isHigherKinded) firstParent.typeParams else super.typeParams @@ -4415,6 +4417,37 @@ trait Types finally foreach2(tvs, saved)(_.suspended = _) } + final def stripExistentialsAndTypeVars(ts: List[Type], expandLazyBaseType: Boolean = false): (List[Type], List[Symbol]) = { + val needsStripping = ts.exists { + case _: RefinedType | _: TypeVar | _: ExistentialType => true + case _ => false + } + if (!needsStripping) (ts, Nil) // fast path for common case + else { + val tparams = mutable.ListBuffer[Symbol]() + val stripped = mutable.ListBuffer[Type]() + def stripType(tp: Type): Unit = tp match { + case rt: RefinedType if isIntersectionTypeForLazyBaseType(rt) => + if (expandLazyBaseType) + rt.parents foreach stripType + else { + devWarning(s"Unexpected RefinedType in stripExistentialsAndTypeVars $ts, not expanding") + stripped += tp + } + case ExistentialType(qs, underlying) => + tparams ++= qs + stripType(underlying) + case tv@TypeVar(_, constr) => + if (tv.instValid) stripType(constr.inst) + else if (tv.untouchable) stripped += tv + else abort("trying to do lub/glb of typevar " + tv) + case tp => stripped += tp + } + ts foreach stripType + (stripped.toList, tparams.toList) + } + } + /** Compute lub (if `variance == Covariant`) or glb (if `variance == Contravariant`) of given list * of types `tps`. All types in `tps` are typerefs or singletypes * with the same symbol. @@ -4422,17 +4455,7 @@ trait Types * Return `NoType` if the computation fails. */ def mergePrefixAndArgs(tps0: List[Type], variance: Variance, depth: Depth): Type = { - var tparams = mutable.ListBuffer[Symbol]() - val tps = tps0.flatMap { - case rt: RefinedType if isIntersectionTypeForLazyBaseType(rt) => rt.parents - case ExistentialType(qs, underlying) => - tparams ++= qs - underlying match { - case rt: RefinedType if isIntersectionTypeForLazyBaseType(rt) => rt.parents - case tp => tp :: Nil - } - case tp => tp :: Nil - } + val (tps, tparams) = stripExistentialsAndTypeVars(tps0, expandLazyBaseType = true) val merged = tps match { case tp :: Nil => tp @@ -4507,7 +4530,7 @@ trait Types case _ => abort(s"mergePrefixAndArgs($tps, $variance, $depth): unsupported tps") } - existentialAbstraction(tparams.toList, merged) + existentialAbstraction(tparams, merged) } def addMember(thistp: Type, tp: Type, sym: Symbol): Unit = addMember(thistp, tp, sym, AnyDepth) diff --git a/src/reflect/scala/reflect/internal/tpe/GlbLubs.scala b/src/reflect/scala/reflect/internal/tpe/GlbLubs.scala index c997dd30eb..108ce45cca 100644 --- a/src/reflect/scala/reflect/internal/tpe/GlbLubs.scala +++ b/src/reflect/scala/reflect/internal/tpe/GlbLubs.scala @@ -210,24 +210,6 @@ private[internal] trait GlbLubs { } } - private def stripExistentialsAndTypeVars(ts: List[Type]): (List[Type], List[Symbol]) = { - val quantified = ts flatMap { - case ExistentialType(qs, _) => qs - case t => List() - } - def stripType(tp: Type): Type = tp match { - case ExistentialType(_, res) => - res - case tv@TypeVar(_, constr) => - if (tv.instValid) stripType(constr.inst) - else if (tv.untouchable) tv - else abort("trying to do lub/glb of typevar "+tp) - case t => t - } - val strippedTypes = ts mapConserve stripType - (strippedTypes, quantified) - } - /** Does this set of types have the same weak lub as * it does regular lub? This is exposed so lub callers * can discover whether the trees they are typing will diff --git a/test/files/neg/lub-from-hell-2.scala b/test/files/neg/lub-from-hell-2.scala index 96760c6edf..18c99dfada 100644 --- a/test/files/neg/lub-from-hell-2.scala +++ b/test/files/neg/lub-from-hell-2.scala @@ -4,3 +4,10 @@ class Test { def bar(a: Boolean, b: scala.collection.mutable.SetLike[Any,scala.collection.mutable.Set[Any]], c: scala.collection.mutable.Buffer[Any]) = if (a) b else c // bar produces an ill-bounded LUB in 2.11.8. After this commit, which fixes a bug in existential+refinement lubs, foo also fails. } +// This test case minimizes a case that stated to fail compile after my fixes in SI-5294. +// `foo` used to compile for the wrong reason, `mergePrefixAndArgs` failed to transpose a +// ragged matrix and skipped to the next level of the base type sequences to find a common type symbol. +// +// My changes fixed the root cause of the ragged matrix, which uncovered the latent bug. +// For comparison, `bar` failed to compile before _and_ after my changes for the same reason: +// f-bounded types involved in LUBs can sometimes produce an ill-bounded LUB. diff --git a/test/files/pos/lub-from-hell.scala b/test/files/pos/lub-from-hell.scala index a7d2a99b08..cb4b1733c7 100644 --- a/test/files/pos/lub-from-hell.scala +++ b/test/files/pos/lub-from-hell.scala @@ -1,11 +1,6 @@ class Test { trait Tree def foo(b: Boolean, buf: collection.mutable.ArrayBuffer[Any], acc: StringBuilder) = if (b) buf else acc - - // def bar(b: Boolean, - // buf: scala.collection.IndexedSeqLike[Any,Cloneable with Mutable with Equals], - // acc: scala.collection.IndexedSeqLike[_18,scala.collection.mutable.IndexedSeq[_18]] with scala.collection.IndexedSeqLike[_18,IndexedSeq[_18]] forSome { type _18 >: String with Char } - // ) = if (b) buf else acc - - } +// This test case minimizes a case that failed to compile due to a bug in my work on +// SI-5294. After refining my patches, it compiles again, as expected. \ No newline at end of file -- cgit v1.2.3