From 83d6fdaabd7cd01323504623c8d5a7fd039f790e Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 5 Mar 2020 13:35:09 +0100 Subject: [PATCH 1/3] Fix #6635: Improve subtype tests for aliases and singleton types 1. backtrack in more cases when we we fail after dealiasing 2. fall back to atoms comparisons when comparing two singleton types The widened criterion for (1) is still provisional. We need to come up with a criterion that is sound and has a low risk of leading to combinatorial explosion when comparing two long alias chains. --- .../dotty/tools/dotc/core/TypeComparer.scala | 146 ++++++++++-------- tests/pos/i6635.scala | 46 ++++++ 2 files changed, 130 insertions(+), 62 deletions(-) create mode 100644 tests/pos/i6635.scala diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index 2ee4b6c6da5b..2b4d9a034e03 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -231,56 +231,69 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w } } + def singleBounds(tp: Type): List[Type] = tp.widenExpr.dealias match + case tp1: SingletonType => tp1 :: Nil + case AndType(tp11, tp12) => singleBounds(tp1) ::: singleBounds(tp2) + case _ => Nil + + def isSingletonAlias(tp: Type): Boolean = tp match + case tp: TermRef => singleBounds(tp).nonEmpty || isSingletonAlias(tp.prefix) + case _ => false + + // THIS IS STILL PROVISIONAL + def canDropAlias(tp: NamedType): Boolean = !isSingletonAlias(tp.prefix) + 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 + 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 canDropAlias(tp2) 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.prefix.isStable && canDropAlias(tp1) then 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 + end compareNamed compareNamed(tp1, tp2) case tp2: ProtoType => isMatchedByProto(tp2, tp1) @@ -753,20 +766,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) case tp1: RefinedType => isNewSubType(tp1.parent) case tp1: RecType => @@ -1174,8 +1183,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 @@ -1209,9 +1228,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 diff --git a/tests/pos/i6635.scala b/tests/pos/i6635.scala new file mode 100644 index 000000000000..dacd1ef5cd8b --- /dev/null +++ b/tests/pos/i6635.scala @@ -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 + } +} \ No newline at end of file From e2399f146e0c8dd7461fcdc7066a5a3634bb4d02 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 5 Mar 2020 14:38:40 +0100 Subject: [PATCH 2/3] A better criterion for avoiding backtracking after aliasing A better criterion for avoiding backtracking after comparing a dealiased type. --- .../dotty/tools/dotc/core/TypeComparer.scala | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index 2b4d9a034e03..b83efcb637bd 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -231,17 +231,27 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w } } - def singleBounds(tp: Type): List[Type] = tp.widenExpr.dealias match - case tp1: SingletonType => tp1 :: Nil - case AndType(tp11, tp12) => singleBounds(tp1) ::: singleBounds(tp2) - case _ => Nil - - def isSingletonAlias(tp: Type): Boolean = tp match - case tp: TermRef => singleBounds(tp).nonEmpty || isSingletonAlias(tp.prefix) - case _ => false - - // THIS IS STILL PROVISIONAL - def canDropAlias(tp: NamedType): Boolean = !isSingletonAlias(tp.prefix) + /** 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(tp: NamedType): Boolean = + val sym = tp.symbol + !sym.canMatchInheritedSymbols + || !tp.prefix.baseClasses.exists( + _.info.nonPrivateDecl(sym.name).symbol.is(Deferred)) def firstTry: Boolean = tp2 match { case tp2: NamedType => @@ -258,17 +268,7 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w tp1.info match { case info1: TypeAlias => if recur(info1.alias, tp2) then return true - if tp1.prefix.isStable && canDropAlias(tp1) then 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. + if canDropAlias(tp1) then return false case _ => } val sym2 = tp2.symbol From 3a2831e688add5a99be0643d099d6150f21bd403 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 5 Mar 2020 18:58:36 +0100 Subject: [PATCH 3/3] Optimize canDropAlias Cache canDopAlias calls in TypeRefs. Performance tests pointed to a ~1% showdown before, which is really in the noise. However, stats showed that the number of iterations in the exists call of `canDropAlias` is significant (a bit more than total member operations). With this optimization, the number of calls gets reduced to 1% of what it was. --- .../dotty/tools/dotc/core/TypeComparer.scala | 26 ++---------------- .../src/dotty/tools/dotc/core/Types.scala | 27 +++++++++++++++++++ 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index b83efcb637bd..5d11294b290e 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -231,28 +231,6 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w } } - /** 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(tp: NamedType): Boolean = - val sym = tp.symbol - !sym.canMatchInheritedSymbols - || !tp.prefix.baseClasses.exists( - _.info.nonPrivateDecl(sym.name).symbol.is(Deferred)) - def firstTry: Boolean = tp2 match { case tp2: NamedType => def compareNamed(tp1: Type, tp2: NamedType): Boolean = @@ -261,14 +239,14 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w info2 match case info2: TypeAlias => if recur(tp1, info2.alias) then return true - if canDropAlias(tp2) then return false + 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 canDropAlias(tp1) then return false + if tp1.asInstanceOf[TypeRef].canDropAlias then return false case _ => } val sym2 = tp2.symbol diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index 46d72821e2f7..06b664381521 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -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