Skip to content

Fix #6288: Allow Singletons in Unions #6299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
May 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b066425
widenUnion also widens singletoons and ExprTypes
odersky Apr 13, 2019
4ca09d8
Drop special case skipping singletons in TypeComparer
odersky Apr 13, 2019
399fc02
Clean up and document tricky logic in TypeComparer
odersky Apr 14, 2019
8e2221d
Don't widen in lubs
odersky Apr 14, 2019
4c5988f
Another fix to ElimOpaque
odersky Apr 14, 2019
dbf2866
Improve efficiency of subtype tests involving unions of singletons
odersky Apr 14, 2019
f484851
Specialize lub for unions over singletons
odersky Apr 14, 2019
d95a239
WIP: Try to simplify widenInferred algorithm
odersky Apr 15, 2019
73eb00b
Combine new widenInferred scheme with singleton unions
odersky Apr 15, 2019
a3933f3
Remove duplication in widenInferred
odersky Apr 15, 2019
8a06b0b
Update docs
odersky Apr 15, 2019
f7bf314
Tweak for safe atoms comparisons after Typer
odersky Apr 16, 2019
be6eb06
Print stack trace only under -Ydebug
odersky Apr 16, 2019
ca73e1d
More test cases
odersky Apr 16, 2019
ca8f1ae
Don't widen instance if bound is subtype of Singleton
odersky Apr 16, 2019
eed8f2d
Normalize atoms in depth
odersky Apr 16, 2019
b27932e
widenUnion now also widens Singletons
odersky Apr 17, 2019
2306e9a
Refine join algorithm
odersky Apr 17, 2019
5d41c0a
Clarify comment
odersky Apr 17, 2019
e46bcd1
Update check file
odersky Apr 29, 2019
227dfa9
Address review comments
odersky Apr 29, 2019
f15016a
Fix check file
nicolasstucki Apr 30, 2019
1083d9d
Fix comment
odersky May 1, 2019
ef8ccf7
Last fixes
odersky May 2, 2019
b940f11
Blacklist a from tasty test
odersky May 2, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 20 additions & 29 deletions compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala
Original file line number Diff line number Diff line change
Expand Up @@ -272,40 +272,31 @@ trait ConstraintHandling[AbstractContext] {
}
}

/** Widen inferred type `tp` with upper bound `bound`, according to the following rules:
* 1. If `tp` is a singleton type, yet `bound` is not a singleton type, nor a subtype
* of `scala.Singleton`, widen `tp`.
* 2. If `tp` is a union type, yet upper bound is not a union type,
* approximate the union type from above by an intersection of all common base types.
/** Widen inferred type `inst` with upper `bound`, according to the following rules:
* 1. If `inst` is a singleton type, or a union containing some singleton types,
* widen (all) the singleton type(s), provied the result is a subtype of `bound`
* (i.e. `inst.widenSingletons <:< bound` succeeds with satisfiable constraint)
* 2. If `inst` is a union type, approximate the union type from above by an intersection
* of all common base types, provied the result is a subtype of `bound`.
*
* Don't do these widenings if `bound` is a subtype of `scala.Singleton`.
*
* At this point we also drop the @Repeated annotation to avoid inferring type arguments with it,
* as those could leak the annotation to users (see run/inferred-repeated-result).
*/
def widenInferred(tp: Type, bound: Type)(implicit actx: AbstractContext): Type = {
def isMultiSingleton(tp: Type): Boolean = tp.stripAnnots match {
case tp: SingletonType => true
case AndType(tp1, tp2) => isMultiSingleton(tp1) | isMultiSingleton(tp2)
case OrType(tp1, tp2) => isMultiSingleton(tp1) & isMultiSingleton(tp2)
case tp: TypeRef => isMultiSingleton(tp.info.hiBound)
case tp: TypeVar => isMultiSingleton(tp.underlying)
case tp: TypeParamRef => isMultiSingleton(bounds(tp).hi)
case _ => false
def widenInferred(inst: Type, bound: Type)(implicit actx: AbstractContext): Type = {
def widenOr(tp: Type) = {
val tpw = tp.widenUnion
if ((tpw ne tp) && tpw <:< bound) tpw else tp
}
def isOrType(tp: Type): Boolean = tp.dealias match {
case tp: OrType => true
case tp: RefinedOrRecType => isOrType(tp.parent)
case AndType(tp1, tp2) => isOrType(tp1) | isOrType(tp2)
case WildcardType(bounds: TypeBounds) => isOrType(bounds.hi)
case _ => false
def widenSingle(tp: Type) = {
val tpw = tp.widenSingletons
if ((tpw ne tp) && tpw <:< bound) tpw else tp
}
def widenOr(tp: Type) =
if (isOrType(tp) && !isOrType(bound)) tp.widenUnion
else tp
def widenSingle(tp: Type) =
if (isMultiSingleton(tp) && !isMultiSingleton(bound) &&
!isSubTypeWhenFrozen(bound, defn.SingletonType)) tp.widen
else tp
widenOr(widenSingle(tp)).dropRepeatedAnnot
val wideInst =
if (isSubTypeWhenFrozen(bound, defn.SingletonType)) inst
else widenOr(widenSingle(inst))
wideInst.dropRepeatedAnnot
}

/** The instance type of `param` in the current constraint (which contains `param`).
Expand All @@ -316,7 +307,7 @@ trait ConstraintHandling[AbstractContext] {
*/
def instanceType(param: TypeParamRef, fromBelow: Boolean)(implicit actx: AbstractContext): Type = {
val inst = approximation(param, fromBelow).simplified
if (fromBelow) widenInferred(inst, constraint.fullUpperBound(param)) else inst
if (fromBelow) widenInferred(inst, param) else inst
}

/** Constraint `c1` subsumes constraint `c2`, if under `c2` as constraint we have
Expand Down
98 changes: 77 additions & 21 deletions compiler/src/dotty/tools/dotc/core/TypeComparer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,15 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] {
case _ =>
false
}
joinOK || recur(tp11, tp2) && recur(tp12, tp2)
def widenOK =
(tp2.widenSingletons eq tp2) &&
(tp1.widenSingletons ne tp1) &&
recur(tp1.widenSingletons, tp2)

if (tp2.atoms.nonEmpty && canCompare(tp2.atoms))
tp1.atoms.nonEmpty && tp1.atoms.subsetOf(tp2.atoms)
else
widenOK || joinOK || recur(tp11, tp2) && recur(tp12, tp2)
case tp1: MatchType =>
val reduced = tp1.reduced
if (reduced.exists) recur(reduced, tp2) else thirdTry
Expand Down Expand Up @@ -573,10 +581,28 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] {
}
compareTypeLambda
case OrType(tp21, tp22) =>
val tp1a = tp1.widenDealiasKeepRefiningAnnots
if (tp2.atoms.nonEmpty && canCompare(tp2.atoms))
return tp1.atoms.nonEmpty && tp1.atoms.subsetOf(tp2.atoms) ||
tp1.isRef(NothingClass)

// The next clause handles a situation like the one encountered in i2745.scala.
// We have:
//
// x: A | B, x.type <:< A | X where X is a type variable
//
// We should instantiate X to B instead of x.type or A | B. To do this, we widen
// the LHS to A | B and recur *without indicating that this is a lowApprox*. The
// latter point is important since otherwise we would not get to instantiate X.
// If that succeeds, fine. If not we continue and hit the `either` below.
// That second path is important to handle comparisons with unions of singletons,
// as in `1 <:< 1 | 2`.
val tp1w = tp1.widen
if ((tp1w ne tp1) && recur(tp1w, tp2))
return true

val tp1a = tp1.dealiasKeepRefiningAnnots
if (tp1a ne tp1)
// Follow the alias; this might avoid truncating the search space in the either below
// Note that it's safe to widen here because singleton types cannot be part of `|`.
// Follow the alias; this might lead to an OrType on the left which needs to be split
return recur(tp1a, tp2)

// Rewrite T1 <: (T211 & T212) | T22 to T1 <: (T211 | T22) and T1 <: (T212 | T22)
Expand Down Expand Up @@ -1038,6 +1064,23 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] {
else None
}

/** Check whether we can compare the given set of atoms with another to determine
* a subtype test between OrTypes. There is one situation where this is not
* the case, which has to do with SkolemTypes. TreeChecker sometimes expects two
* types to be equal that have different skolems. To account for this, we identify
* two different skolems in all phases `p`, where `p.isTyper` is false.
* But in that case comparing two sets of atoms that contain skolems
* for equality would give the wrong result, so we should not use the sets
* for comparisons.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't not using the sets a potential performance problem ? Have you considered having both OrType#atoms and OrType#atomsUnifySkolems instead, where the latter would replace all skolems referring to the same type by a single instance ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe skolems are sufficiently rare. If it turns out that we get a performance problem, what you suggest could be a good alternative.

*/
def canCompare(atoms: Set[Type]): Boolean =
ctx.phase.isTyper || {
val hasSkolems = new ExistsAccumulator(_.isInstanceOf[SkolemType]) {
override def stopAtStatic = true
}
!atoms.exists(hasSkolems(false, _))
}

/** Subtype test for corresponding arguments in `args1`, `args2` according to
* variances in type parameters `tparams2`.
*
Expand Down Expand Up @@ -1499,30 +1542,38 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] {
/** The greatest lower bound of a list types */
final def glb(tps: List[Type]): Type = ((AnyType: Type) /: tps)(glb)

def widenInUnions(implicit ctx: Context): Boolean = ctx.scala2Mode || ctx.erasedTypes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have examples of Scala 2 code that breaks without the scala2Mode here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried. But it's better to be conservative, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, all these inference differences (that we'll have to document at some point) make it more risky for a codebase to turn off Scala2 mode since there's a non-zero chance any of them could cause a slient change of runtime behavior (e.g. because a different implicit ends up being selected). They're also only lightly tested since most of our tests are run without Scala2 mode enabled, meaning a higher risk of unforeseen interactions. So I'd limit them to things that significantly help in cross-compilation, but we can revisit this some other time.


/** The least upper bound of two types
* @param canConstrain If true, new constraints might be added to simplify the lub.
* @note We do not admit singleton types in or-types as lubs.
*/
def lub(tp1: Type, tp2: Type, canConstrain: Boolean = false): Type = /*>|>*/ trace(s"lub(${tp1.show}, ${tp2.show}, canConstrain=$canConstrain)", subtyping, show = true) /*<|<*/ {
if (tp1 eq tp2) tp1
else if (!tp1.exists) tp1
else if (!tp2.exists) tp2
else if ((tp1 isRef AnyClass) || (tp1 isRef AnyKindClass) || (tp2 isRef NothingClass)) tp1
else if ((tp2 isRef AnyClass) || (tp2 isRef AnyKindClass) || (tp1 isRef NothingClass)) tp2
else {
val t1 = mergeIfSuper(tp1, tp2, canConstrain)
if (t1.exists) t1
else {
val t2 = mergeIfSuper(tp2, tp1, canConstrain)
if (t2.exists) t2
else {
val tp1w = tp1.widen
val tp2w = tp2.widen
if ((tp1 ne tp1w) || (tp2 ne tp2w)) lub(tp1w, tp2w)
else orType(tp1w, tp2w) // no need to check subtypes again
}
if (tp1 eq tp2) return tp1
if (!tp1.exists) return tp1
if (!tp2.exists) return tp2
if ((tp1 isRef AnyClass) || (tp1 isRef AnyKindClass) || (tp2 isRef NothingClass)) return tp1
if ((tp2 isRef AnyClass) || (tp2 isRef AnyKindClass) || (tp1 isRef NothingClass)) return tp2
val atoms1 = tp1.atoms
if (atoms1.nonEmpty && !widenInUnions) {
val atoms2 = tp2.atoms
if (atoms2.nonEmpty) {
if (atoms1.subsetOf(atoms2)) return tp2
if (atoms2.subsetOf(atoms1)) return tp1
if ((atoms1 & atoms2).isEmpty) return orType(tp1, tp2)
}
}
val t1 = mergeIfSuper(tp1, tp2, canConstrain)
if (t1.exists) return t1

val t2 = mergeIfSuper(tp2, tp1, canConstrain)
if (t2.exists) return t2

def widen(tp: Type) = if (widenInUnions) tp.widen else tp.widenIfUnstable
val tp1w = widen(tp1)
val tp2w = widen(tp2)
if ((tp1 ne tp1w) || (tp2 ne tp2w)) lub(tp1w, tp2w)
else orType(tp1w, tp2w) // no need to check subtypes again
}

/** The least upper bound of a list of types */
Expand Down Expand Up @@ -2213,6 +2264,11 @@ class ExplainingTypeComparer(initctx: Context) extends TypeComparer(initctx) {
super.isSubType(tp1, tp2, approx)
}

override def recur(tp1: Type, tp2: Type): Boolean =
traceIndented(s"${show(tp1)} <:< ${show(tp2)} recur ${if (frozenConstraint) " frozen" else ""}") {
super.recur(tp1, tp2)
}

override def hasMatchingMember(name: Name, tp1: Type, tp2: RefinedType): Boolean =
traceIndented(s"hasMatchingMember(${show(tp1)} . $name, ${show(tp2.refinedInfo)}), member = ${show(tp1.member(name).info)}") {
super.hasMatchingMember(name, tp1, tp2)
Expand Down
100 changes: 79 additions & 21 deletions compiler/src/dotty/tools/dotc/core/TypeOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,11 @@ trait TypeOps { this: Context => // TODO: Make standalone object.
* class A extends C[A] with D
* class B extends C[B] with D with E
*
* we approximate `A | B` by `C[A | B] with D`
* we approximate `A | B` by `C[A | B] with D`.
*
* Before we do that, we try to find a common non-class supertype of T1 | ... | Tn
* in a "best effort", ad-hoc way by selectively widening types in `T1, ..., Tn`
* and stopping if the resulting union simplifies to a type that is not a disjunction.
*/
def orDominator(tp: Type): Type = {

Expand Down Expand Up @@ -188,29 +192,83 @@ trait TypeOps { this: Context => // TODO: Make standalone object.
case _ => false
}

// Step 1: Get RecTypes and ErrorTypes out of the way,
tp1 match {
case tp1: RecType =>
tp1.rebind(approximateOr(tp1.parent, tp2))
case tp1: TypeProxy if !isClassRef(tp1) =>
orDominator(tp1.superType | tp2)
case err: ErrorType =>
err
case tp1: RecType => return tp1.rebind(approximateOr(tp1.parent, tp2))
case err: ErrorType => return err
case _ =>
tp2 match {
case tp2: RecType =>
tp2.rebind(approximateOr(tp1, tp2.parent))
case tp2: TypeProxy if !isClassRef(tp2) =>
orDominator(tp1 | tp2.superType)
case err: ErrorType =>
err
case _ =>
val commonBaseClasses = tp.mapReduceOr(_.baseClasses)(intersect)
val doms = dominators(commonBaseClasses, Nil)
def baseTp(cls: ClassSymbol): Type =
tp.baseType(cls).mapReduceOr(identity)(mergeRefinedOrApplied)
doms.map(baseTp).reduceLeft(AndType.apply)
}
}
tp2 match {
case tp2: RecType => return tp2.rebind(approximateOr(tp1, tp2.parent))
case err: ErrorType => return err
case _ =>
}

// Step 2: Try to widen either side. This is tricky and incomplete.
// An illustration is in test pos/padTo.scala: Here we need to compute the join of
//
// `A | C` under the constraints `B >: A` and `C <: B`
//
// where `A, B, C` are type parameters.
// Widening `A` to its upper bound would give `Any | C`, i.e. `Any`.
// But widening `C` first would give `A | B` and then `B`.
// So we need to widen `C` first. But how to decide this in general?
// In the algorithm below, we try to widen both sides (once), and then proceed as follows:
//
// 2.0. If no widening succeeds, proceed with step 3.
// 2.1. If only one widening succeeds, continue with that one.
// 2.2. If the two widened types are in a subtype relationship, continue with the smaller one.
// 2.3. If exactly one of the two types is a singleton type, continue with the widened singleton type.
// 2.4. If the widened tp2 is a supertype of tp1, return widened tp2.
// 2.5. If the widened tp1 is a supertype of tp2, return widened tp1.
// 2.6. Otherwise, continue with widened tp1
//
// At steps 4-6 we lose possible solutions, since we have to make an
// arbitrary choice which side to widen. A better solution would look at
// the constituents of each operand (if the operand is an OrType again) and
// try to widen them selectively in turn. But this might lead to a combinatorial
// explosion of possibilities.
//
// Another approach could be to store information contained in lower bounds
// on both sides. So if `B >: A` we'd also record that `A <: B` and therefore
// widening `A` would yield `B` instead of `Any`, so we'd still be on the right track.
// This looks feasible if lower bounds are type parameters, but tricky if they
// are something else. We'd have to extract the strongest possible
// constraint over all type parameters that is implied by a lower bound.
// This looks related to an algorithmic problem arising in GADT matching.
//
// However, this alone is still not enough. There are other sources of incompleteness,
// for instance arising from mis-aligned refinements.
val tp1w = tp1 match {
case tp1: TypeProxy if !isClassRef(tp1) => tp1.superType.widenExpr
case _ => tp1
}
val tp2w = tp2 match {
case tp2: TypeProxy if !isClassRef(tp2) => tp2.superType.widenExpr
case _ => tp2
}
if ((tp1w ne tp1) || (tp2w ne tp2)) {
val isSingle1 = tp1.isInstanceOf[SingletonType]
val isSingle2 = tp2.isInstanceOf[SingletonType]
return {
if (tp2w eq tp2) orDominator(tp1w | tp2) // 2.1
else if (tp1w eq tp1) orDominator(tp1 | tp2w) // 2.1
else if (tp1w frozen_<:< tp2w) orDominator(tp1w | tp2) // 2.2
else if (tp2w frozen_<:< tp1w) orDominator(tp1 | tp2w) // 2.2
else if (isSingle1 && !isSingle2) orDominator(tp1w | tp2) // 2.3
else if (isSingle2 && !isSingle1) orDominator(tp1 | tp2w) // 2.3
else if (tp1 frozen_<:< tp2w) tp2w // 2.4
else if (tp2 frozen_<:< tp1w) tp1w // 2.5
else orDominator(tp1w | tp2) // 2.6
}
}

// Step 3: Intersect base classes of both sides
val commonBaseClasses = tp.mapReduceOr(_.baseClasses)(intersect)
val doms = dominators(commonBaseClasses, Nil)
def baseTp(cls: ClassSymbol): Type =
tp.baseType(cls).mapReduceOr(identity)(mergeRefinedOrApplied)
doms.map(baseTp).reduceLeft(AndType.apply)
}

tp match {
Expand Down
Loading