From 455aa50a5cea78518179f7ef2e2cd3c482e99778 Mon Sep 17 00:00:00 2001 From: Yichen Xu Date: Sat, 15 Jul 2023 07:22:41 +0800 Subject: [PATCH 1/5] Fix spurious subtype check pruning when both sides have unions --- .../dotty/tools/dotc/core/TypeComparer.scala | 45 +++++++++++++------ tests/pos/i17465.scala | 45 +++++++++++++++++++ 2 files changed, 76 insertions(+), 14 deletions(-) create mode 100644 tests/pos/i17465.scala diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index f2b2a1661ebd..5a91b5c24882 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -1479,9 +1479,39 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling /** Like tp1 <:< tp2, but returns false immediately if we know that * the case was covered previously during subtyping. + * + * A type has been covered previously in subtype checking if it + * is some combination of TypeRefs that point to classes, where the + * combiners are AppliedTypes, RefinedTypes, RecTypes, And/Or-Types or AnnotatedTypes. + * + * The exception is that if both sides contain OrTypes, the check hasn't been covered. + * See #17465. */ def isNewSubType(tp1: Type): Boolean = - if (isCovered(tp1) && isCovered(tp2)) + + def isCovered(tp: Type): (Boolean, Boolean) = + var containsOr: Boolean = false + @annotation.tailrec def recur(todos: List[Type]): Boolean = todos match + case tp :: todos => + tp.dealiasKeepRefiningAnnots.stripTypeVar match + case tp: TypeRef => + if tp.symbol.isClass && tp.symbol != NothingClass && tp.symbol != NullClass then recur(todos) + else false + case tp: AppliedType => recur(tp.tycon :: todos) + case tp: RefinedOrRecType => recur(tp.parent :: todos) + case tp: AndType => recur(tp.tp1 :: tp.tp2 :: todos) + case tp: OrType => + containsOr = true + recur(tp.tp1 :: tp.tp2 :: todos) + case _ => false + case Nil => true + val result = recur(tp :: Nil) + (result, containsOr) + + val (covered1, hasOr1) = isCovered(tp1) + val (covered2, hasOr2) = isCovered(tp2) + + if covered1 && covered2 && !(hasOr1 && hasOr2) then //println(s"useless subtype: $tp1 <:< $tp2") false else isSubType(tp1, tp2, approx.addLow) @@ -2091,19 +2121,6 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling tp1.parent.asInstanceOf[RefinedType], tp2.parent.asInstanceOf[RefinedType], limit)) - /** A type has been covered previously in subtype checking if it - * is some combination of TypeRefs that point to classes, where the - * combiners are AppliedTypes, RefinedTypes, RecTypes, And/Or-Types or AnnotatedTypes. - */ - private def isCovered(tp: Type): Boolean = tp.dealiasKeepRefiningAnnots.stripTypeVar match { - case tp: TypeRef => tp.symbol.isClass && tp.symbol != NothingClass && tp.symbol != NullClass - case tp: AppliedType => isCovered(tp.tycon) - case tp: RefinedOrRecType => isCovered(tp.parent) - case tp: AndType => isCovered(tp.tp1) && isCovered(tp.tp2) - case tp: OrType => isCovered(tp.tp1) && isCovered(tp.tp2) - case _ => false - } - /** Defer constraining type variables when compared against prototypes */ def isMatchedByProto(proto: ProtoType, tp: Type): Boolean = tp.stripTypeVar match { case tp: TypeParamRef if constraint contains tp => true diff --git a/tests/pos/i17465.scala b/tests/pos/i17465.scala new file mode 100644 index 000000000000..00a59f7681d2 --- /dev/null +++ b/tests/pos/i17465.scala @@ -0,0 +1,45 @@ +def test1[A, B]: Unit = { + def f[T](x: T{ def *(y: Int): T }): T = ??? + def test = f[scala.collection.StringOps | String]("Hello") + locally: + val test1 : (scala.collection.StringOps | String) { def *(y: Int): (scala.collection.StringOps | String) } = ??? + val test2 : (scala.collection.StringOps | String) { def *(y: Int): (scala.collection.StringOps | String) } = test1 + + locally: + val test1 : (Int | String) { def foo(x: Int): Int } = ??? + val test2 : (Int | String) { def foo(x: Int): Int } = test1 + + locally: + val test1 : ((Int | String) & Any) { def foo(): Int } = ??? + val test2 : ((Int | String) & Any) { def foo(): Int } = test1 + + locally: + val test1 : Int { def foo(): Int } = ??? + val test2 : Int { def foo(): Int } = test1 + + locally: + val test1 : (Int | String) { def foo(): Int } = ??? + val test2 : (Int | String) & Any = test1 + + locally: + val test1 : (Int | B) { def *(y: Int): Int } = ??? + val test2 : (Int | B) { def *(y: Int): Int } = test1 + + locally: + val test1 : (Int | String) = ??? + val test2 : (Int | String) = test1 + + type Foo = Int | String + locally: + val test1 : Foo { type T = Int } = ??? + val test2 : (Int | String) = test1 +} + +def test2: Unit = { + import reflect.Selectable.reflectiveSelectable + + trait A[T](x: T{ def *(y: Int): T }): + def f: T = x * 2 + + class B extends A("Hello") +} From 380bd1b91a6ca0edbf4a555ea48455bdd221c5a5 Mon Sep 17 00:00:00 2001 From: Yichen Xu Date: Tue, 18 Jul 2023 01:24:19 +0800 Subject: [PATCH 2/5] Speedup isCovered - Revert tailrec rewrite to reduce allocation - Use a integer-based representation for the result of `isCovered` The results are now aggregated and inspected with bitwise operations. --- .../dotty/tools/dotc/core/TypeComparer.scala | 61 +++++++++++-------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index 5a91b5c24882..54b3102c07a3 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -1488,30 +1488,18 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling * See #17465. */ def isNewSubType(tp1: Type): Boolean = - - def isCovered(tp: Type): (Boolean, Boolean) = - var containsOr: Boolean = false - @annotation.tailrec def recur(todos: List[Type]): Boolean = todos match - case tp :: todos => - tp.dealiasKeepRefiningAnnots.stripTypeVar match - case tp: TypeRef => - if tp.symbol.isClass && tp.symbol != NothingClass && tp.symbol != NullClass then recur(todos) - else false - case tp: AppliedType => recur(tp.tycon :: todos) - case tp: RefinedOrRecType => recur(tp.parent :: todos) - case tp: AndType => recur(tp.tp1 :: tp.tp2 :: todos) - case tp: OrType => - containsOr = true - recur(tp.tp1 :: tp.tp2 :: todos) - case _ => false - case Nil => true - val result = recur(tp :: Nil) - (result, containsOr) - - val (covered1, hasOr1) = isCovered(tp1) - val (covered2, hasOr2) = isCovered(tp2) - - if covered1 && covered2 && !(hasOr1 && hasOr2) then + def isCovered(tp: Type): CoveredStatus = + tp.dealiasKeepRefiningAnnots.stripTypeVar match + case tp: TypeRef if tp.symbol.isClass && tp.symbol != NothingClass && tp.symbol != NullClass => CoveredStatus.Covered + case tp: AppliedType => isCovered(tp.tycon) + case tp: RefinedOrRecType => isCovered(tp.parent) + case tp: AndType => isCovered(tp.tp1) combinedWith isCovered(tp.tp2) + case tp: OrType => isCovered(tp.tp1) combinedWith isCovered(tp.tp2) + case _ => CoveredStatus.Uncovered + + val covered1 = isCovered(tp1) + val covered2 = isCovered(tp2) + if CoveredStatus.bothCovered(covered1, covered2) && !CoveredStatus.bothHaveOr(covered1, covered2) then //println(s"useless subtype: $tp1 <:< $tp2") false else isSubType(tp1, tp2, approx.addLow) @@ -3008,6 +2996,31 @@ object TypeComparer { end ApproxState type ApproxState = ApproxState.Repr + /** Result of `isCovered` check. */ + object CoveredStatus: + opaque type Repr = Int + + private inline val IsCovered = 2 + private inline val NotHasOr = 1 + + /** The type is not covered. */ + val Uncovered: Repr = 1 + + /** The type is covered and contains OrTypes. */ + val CoveredWithOr: Repr = 2 + + /** The type is covered and free from OrTypes. */ + val Covered: Repr = 3 + + object Repr: + extension (s: Repr) + inline def combinedWith(that: Repr): Repr = s min that + + inline def bothHaveOr(s1: Repr, s2: Repr): Boolean = ~(s1 | s2 & NotHasOr) != 0 + inline def bothCovered(s1: Repr, s2: Repr): Boolean = (s1 & s2 & IsCovered) != 0 + end CoveredStatus + type CoveredStatus = CoveredStatus.Repr + def topLevelSubType(tp1: Type, tp2: Type)(using Context): Boolean = comparing(_.topLevelSubType(tp1, tp2)) From 33c8b6061c12404f408b0016d222f2253cb4ceca Mon Sep 17 00:00:00 2001 From: Yichen Xu Date: Mon, 17 Jul 2023 19:51:12 +0200 Subject: [PATCH 3/5] Fix precedence error found in review Co-authored-by: Dale Wijnand --- compiler/src/dotty/tools/dotc/core/TypeComparer.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index 54b3102c07a3..9ddfceb95a06 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -3016,7 +3016,7 @@ object TypeComparer { extension (s: Repr) inline def combinedWith(that: Repr): Repr = s min that - inline def bothHaveOr(s1: Repr, s2: Repr): Boolean = ~(s1 | s2 & NotHasOr) != 0 + inline def bothHaveOr(s1: Repr, s2: Repr): Boolean = ~((s1 | s2) & NotHasOr) != 0 inline def bothCovered(s1: Repr, s2: Repr): Boolean = (s1 & s2 & IsCovered) != 0 end CoveredStatus type CoveredStatus = CoveredStatus.Repr From 8275cef3cd2bb852928f710a574745821b71fa4a Mon Sep 17 00:00:00 2001 From: Yichen Xu Date: Tue, 18 Jul 2023 12:11:45 +0800 Subject: [PATCH 4/5] Enable short circuiting for `CoveredStatus.CombinedWith` --- compiler/src/dotty/tools/dotc/core/TypeComparer.scala | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index 9ddfceb95a06..13b90098ced8 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -3014,7 +3014,9 @@ object TypeComparer { object Repr: extension (s: Repr) - inline def combinedWith(that: Repr): Repr = s min that + def combinedWith(that: => Repr): Repr = + if s == Uncovered then Uncovered + else s min that inline def bothHaveOr(s1: Repr, s2: Repr): Boolean = ~((s1 | s2) & NotHasOr) != 0 inline def bothCovered(s1: Repr, s2: Repr): Boolean = (s1 & s2 & IsCovered) != 0 From 13c87baac8724dc96a1bd87c46da856ec88298d3 Mon Sep 17 00:00:00 2001 From: Yichen Xu Date: Thu, 20 Jul 2023 16:29:28 +0800 Subject: [PATCH 5/5] Apply suggested changes in review - Speedup pattern matching in `isCovered` - Make `CoveredStatus` private - Use direct integer comparison instead of bit operations for better readability and maintainability --- .../dotty/tools/dotc/core/TypeComparer.scala | 38 ++++++------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala index 13b90098ced8..5ea9481c96e2 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala @@ -1490,16 +1490,19 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling def isNewSubType(tp1: Type): Boolean = def isCovered(tp: Type): CoveredStatus = tp.dealiasKeepRefiningAnnots.stripTypeVar match - case tp: TypeRef if tp.symbol.isClass && tp.symbol != NothingClass && tp.symbol != NullClass => CoveredStatus.Covered + case tp: TypeRef => + if tp.symbol.isClass && tp.symbol != NothingClass && tp.symbol != NullClass + then CoveredStatus.Covered + else CoveredStatus.Uncovered case tp: AppliedType => isCovered(tp.tycon) case tp: RefinedOrRecType => isCovered(tp.parent) - case tp: AndType => isCovered(tp.tp1) combinedWith isCovered(tp.tp2) - case tp: OrType => isCovered(tp.tp1) combinedWith isCovered(tp.tp2) + case tp: AndType => isCovered(tp.tp1) min isCovered(tp.tp2) + case tp: OrType => isCovered(tp.tp1) min isCovered(tp.tp2) min CoveredStatus.CoveredWithOr case _ => CoveredStatus.Uncovered val covered1 = isCovered(tp1) val covered2 = isCovered(tp2) - if CoveredStatus.bothCovered(covered1, covered2) && !CoveredStatus.bothHaveOr(covered1, covered2) then + if (covered1 min covered2) >= CoveredStatus.CoveredWithOr && (covered1 max covered2) == CoveredStatus.Covered then //println(s"useless subtype: $tp1 <:< $tp2") false else isSubType(tp1, tp2, approx.addLow) @@ -2997,29 +3000,12 @@ object TypeComparer { type ApproxState = ApproxState.Repr /** Result of `isCovered` check. */ - object CoveredStatus: - opaque type Repr = Int - - private inline val IsCovered = 2 - private inline val NotHasOr = 1 - - /** The type is not covered. */ - val Uncovered: Repr = 1 - - /** The type is covered and contains OrTypes. */ - val CoveredWithOr: Repr = 2 - - /** The type is covered and free from OrTypes. */ - val Covered: Repr = 3 - - object Repr: - extension (s: Repr) - def combinedWith(that: => Repr): Repr = - if s == Uncovered then Uncovered - else s min that + private object CoveredStatus: + type Repr = Int - inline def bothHaveOr(s1: Repr, s2: Repr): Boolean = ~((s1 | s2) & NotHasOr) != 0 - inline def bothCovered(s1: Repr, s2: Repr): Boolean = (s1 & s2 & IsCovered) != 0 + val Uncovered: Repr = 1 // The type is not covered + val CoveredWithOr: Repr = 2 // The type is covered and contains OrTypes + val Covered: Repr = 3 // The type is covered and free from OrTypes end CoveredStatus type CoveredStatus = CoveredStatus.Repr