Skip to content

Fix #6635: Improve subtype tests for aliases and singleton types #8443

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 3 commits into from
Mar 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
124 changes: 62 additions & 62 deletions compiler/src/dotty/tools/dotc/core/TypeComparer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -233,54 +233,45 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w

def firstTry: Boolean = tp2 match {
case tp2: NamedType =>
def compareNamed(tp1: Type, tp2: NamedType): Boolean = {
def compareNamed(tp1: Type, tp2: NamedType): Boolean =
implicit val ctx: Context = this.ctx
tp2.info match {
val info2 = tp2.info
Copy link
Contributor

Choose a reason for hiding this comment

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

This binding doesn't seem necessary

info2 match
case info2: TypeAlias =>
recur(tp1, info2.alias)
case _ => tp1 match {
case tp1: NamedType =>
tp1.info match {
case info1: TypeAlias =>
if (recur(info1.alias, tp2)) return true
if (tp1.prefix.isStable) return false
// If tp1.prefix is stable, the alias does contain all information about the original ref, so
// there's no need to try something else. (This is important for performance).
// To see why we cannot in general stop here, consider:
//
// trait C { type A }
// trait D { type A = String }
// (C & D)#A <: C#A
//
// Following the alias leads to the judgment `String <: C#A` which is false.
// However the original judgment should be true.
case _ =>
}
val sym2 = tp2.symbol
var sym1 = tp1.symbol
if (sym1.is(ModuleClass) && sym2.is(ModuleVal))
// For convenience we want X$ <:< X.type
// This is safe because X$ self-type is X.type
sym1 = sym1.companionModule
if ((sym1 ne NoSymbol) && (sym1 eq sym2))
ctx.erasedTypes ||
sym1.isStaticOwner ||
isSubType(tp1.prefix, tp2.prefix) ||
thirdTryNamed(tp2)
else
( (tp1.name eq tp2.name)
&& tp1.isMemberRef
&& tp2.isMemberRef
&& isSubType(tp1.prefix, tp2.prefix)
&& tp1.signature == tp2.signature
&& !(sym1.isClass && sym2.isClass) // class types don't subtype each other
) ||
thirdTryNamed(tp2)
case _ =>
secondTry
}
}
}
if recur(tp1, info2.alias) then return true
if tp2.asInstanceOf[TypeRef].canDropAlias then return false
case _ =>
tp1 match
case tp1: NamedType =>
tp1.info match {
case info1: TypeAlias =>
if recur(info1.alias, tp2) then return true
if tp1.asInstanceOf[TypeRef].canDropAlias then return false
case _ =>
}
val sym2 = tp2.symbol
var sym1 = tp1.symbol
if (sym1.is(ModuleClass) && sym2.is(ModuleVal))
// For convenience we want X$ <:< X.type
// This is safe because X$ self-type is X.type
sym1 = sym1.companionModule
if ((sym1 ne NoSymbol) && (sym1 eq sym2))
ctx.erasedTypes ||
sym1.isStaticOwner ||
isSubType(tp1.prefix, tp2.prefix) ||
thirdTryNamed(tp2)
else
( (tp1.name eq tp2.name)
&& tp1.isMemberRef
&& tp2.isMemberRef
&& isSubType(tp1.prefix, tp2.prefix)
&& tp1.signature == tp2.signature
&& !(sym1.isClass && sym2.isClass) // class types don't subtype each other
) ||
thirdTryNamed(tp2)
Copy link
Contributor

Choose a reason for hiding this comment

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

introduce an explicit boolean return?

case _ =>
secondTry
end compareNamed
compareNamed(tp1, tp2)
case tp2: ProtoType =>
isMatchedByProto(tp2, tp1)
Expand Down Expand Up @@ -753,20 +744,16 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w
case tp1 @ AppliedType(tycon1, args1) =>
compareAppliedType1(tp1, tycon1, args1)
case tp1: SingletonType =>
/** if `tp2 == p.type` and `p: q.type` then try `tp1 <:< q.type` as a last effort.*/
def comparePaths = tp2 match {
def comparePaths = tp2 match
case tp2: TermRef =>
tp2.info.widenExpr.dealias match {
case tp2i: SingletonType =>
recur(tp1, tp2i)
// see z1720.scala for a case where this can arise even in typer.
// Also, i1753.scala, to show why the dealias above is necessary.
case _ => false
compareAtoms(tp1, tp2, knownSingletons = true).getOrElse(false)
|| { // needed to make from-tasty work. test cases: pos/i1753.scala, pos/t839.scala
tp2.info.widenExpr.dealias match
case tp2i: SingletonType => recur(tp1, tp2i)
case _ => false
}
case _ =>
false
}
isNewSubType(tp1.underlying.widenExpr) || comparePaths
case _ => false
comparePaths || isNewSubType(tp1.underlying.widenExpr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed this change in-person w/ Martin: changing the order of arguments here is solely a performance optimization, not an actual change to logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

a || b == b || a, so yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hepin1989 thank you for your astute observation! I was thinking about things such as the old RHS assuming the old LHS already handled some type shape, or that we do some evil tricks when loading code from Tasty and the check on new LHS is necessary so that the new RHS doesn't blow up, but I suppose it never hurts to be reminded of elementary laws of Boolean algebra.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or otherwise, a || b and b || a differ when a and b are not pure :-)

case tp1: RefinedType =>
isNewSubType(tp1.parent)
case tp1: RecType =>
Expand Down Expand Up @@ -1174,8 +1161,18 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w

/** If both `tp1` and `tp2` have atoms information, compare the atoms
* in a Some, otherwise None.
* @param knownSingletons If true, we are coming from a comparison of two singleton types
* This influences the comparison as shown below:
*
* Say you have singleton types p.type and q.type the atoms of p.type are `{p.type}..{p.type}`,
* and the atoms of `q.type` are `{}..{p.type}`. Normally the atom comparison between p's
* atoms and q's atoms gives false. But in this case we know that `q.type` is an alias of `p.type`
* so we are still allowed to conclude that `p.type <:< q.type`. A situation where this happens
* is in i6635.scala. Here,
*
* p: A, q: B & p.type and we want to conclude that p.type <: q.type.
*/
def compareAtoms(tp1: Type, tp2: Type): Option[Boolean] =
def compareAtoms(tp1: Type, tp2: Type, knownSingletons: Boolean = false): Option[Boolean] =

/** 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
Expand Down Expand Up @@ -1209,9 +1206,12 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w
case Atoms.Range(lo2, hi2) if canCompareAtoms && canCompare(hi2) =>
tp1.atoms match
case Atoms.Range(lo1, hi1) =>
if hi1.subsetOf(lo2) then Some(verified(true))
else if !lo1.subsetOf(hi2) then Some(verified(false))
else None
if hi1.subsetOf(lo2) || knownSingletons && hi2.size == 1 && hi1 == hi2 then
Some(verified(true))
else if !lo1.subsetOf(hi2) then
Some(verified(false))
else
None
case _ => Some(verified(recur(tp1, NothingType)))
case _ => None

Expand Down
27 changes: 27 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2403,6 +2403,33 @@ object Types {
type ThisType = TypeRef
type ThisName = TypeName

private var myCanDropAliasPeriod: Period = Nowhere
private var myCanDropAlias: Boolean = _

/** Given an alias type `type A = B` where a recursive comparison with `B` yields
* `false`, can we conclude that the comparison is definitely false?
* This could not be the case if `A` overrides some abstract type. Example:
*
* class C { type A }
* class D { type A = Int }
* val c: C
* val d: D & c.type
* c.A <:< d.A ?
*
* The test should return true, by performing the logic in the bottom half of
* firstTry (where we check the names of types). But just following the alias
* from d.A to Int reduces the problem to `c.A <:< Int`, which returns `false`.
* So we can't drop the alias here, we need to do the backtracking to the name-
* based tests.
*/
def canDropAlias(using ctx: Context) =
if myCanDropAliasPeriod != ctx.period then
myCanDropAlias =
!symbol.canMatchInheritedSymbols
|| !prefix.baseClasses.exists(_.info.decls.lookup(name).is(Deferred))
myCanDropAliasPeriod = ctx.period
myCanDropAlias

override def designator: Designator = myDesignator
override protected def designator_=(d: Designator): Unit = myDesignator = d

Expand Down
46 changes: 46 additions & 0 deletions tests/pos/i6635.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
object Test {
abstract class ExprBase { s =>
type A
}

abstract class Lit extends ExprBase { s =>
type A = Int
val n: A
}

abstract class LitU extends ExprBase { s =>
type A <: Int
val n: A
}

abstract class LitL extends ExprBase { s =>
type A <: Int
val n: A
}

def castTest1(e1: ExprBase)(e2: e1.type)(x: e1.A): e2.A = x
def castTest2(e1: ExprBase { type A = Int })(e2: e1.type)(x: e1.A): e2.A = x
def castTest3(e1: ExprBase)(e2: ExprBase with e1.type)(x: e2.A): e1.A = x

def castTest4(e1: ExprBase { type A = Int })(e2: ExprBase with e1.type)(x: e2.A): e1.A = x

def castTest5a(e1: ExprBase)(e2: LitU with e1.type)(x: e2.A): e1.A = x
def castTest5b(e1: ExprBase)(e2: LitL with e1.type)(x: e2.A): e1.A = x

//fail:
def castTestFail1(e1: ExprBase)(e2: Lit with e1.type)(x: e2.A): e1.A = x // this is like castTest5a/b, but with Lit instead of LitU/LitL
// the other direction never works:
def castTestFail2a(e1: ExprBase)(e2: Lit with e1.type)(x: e1.A): e2.A = x
def castTestFail2b(e1: ExprBase)(e2: LitL with e1.type)(x: e1.A): e2.A = x
def castTestFail2c(e1: ExprBase)(e2: LitU with e1.type)(x: e1.A): e2.A = x

// the problem isn't about order of intersections.
def castTestFail2bFlip(e1: ExprBase)(e2: e1.type with LitL)(x: e1.A): e2.A = x
def castTestFail2cFlip(e1: ExprBase)(e2: e1.type with LitU)(x: e1.A): e2.A = x

def castTestFail3(e1: ExprBase)(e2: Lit with e1.type)(x: e1.A): e2.A = {
val y: e1.type with e2.type = e2
val z = x: y.A
z
}
}