From f914c28e9c09fb58d4358869a5a30918a07cdd15 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 23 Feb 2022 18:47:15 +0000 Subject: [PATCH 1/2] Preserve the intersection of disjoint numbers in match analysis The use of provablyDisjoint is a way of simplifying an intersection of two types into the Empty space, using type comparing logic. However, for types like (42L: Long) and (it: Int) (a constant type and a term ref, of two different number types) the two types are provably disjoint, but that doesn't mean that a "case `it` =>" won't match a "42L" scrutinee. So we extend the logic to keep the intersection as it is when numeric value types are in play. --- .../tools/dotc/transform/patmat/Space.scala | 31 ++++++++++++------- tests/patmat/i14407.dupe.check | 1 + tests/patmat/i14407.dupe.scala | 7 +++++ tests/patmat/i14407.scala | 6 ++++ 4 files changed, 33 insertions(+), 12 deletions(-) create mode 100644 tests/patmat/i14407.dupe.check create mode 100644 tests/patmat/i14407.dupe.scala create mode 100644 tests/patmat/i14407.scala diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index ce1a5d9a10c2..a6948db799c3 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -4,6 +4,7 @@ package patmat import core._ import Types._ +import TypeUtils._ import Contexts._ import Flags._ import ast._ @@ -333,9 +334,11 @@ class SpaceEngine(using Context) extends SpaceLogic { // Since projections of types don't include null, intersection with null is empty. Empty else - val res = TypeComparer.provablyDisjoint(tp1, tp2) - if res then Empty - else Typ(AndType(tp1, tp2), decomposed = true) + val intersection = Typ(AndType(tp1, tp2), decomposed = true) + // unrelated numeric value classes can equal each other, so let's not consider type space interection empty + if tp1.classSymbol.isNumericValueClass && tp2.classSymbol.isNumericValueClass then intersection + else if TypeComparer.provablyDisjoint(tp1, tp2) then Empty + else intersection } /** Return the space that represents the pattern `pat` */ @@ -407,7 +410,7 @@ class SpaceEngine(using Context) extends SpaceLogic { case tp => Typ(tp, decomposed = true) } - private def unapplySeqInfo(resTp: Type, pos: SrcPos)(using Context): (Int, Type, Type) = { + private def unapplySeqInfo(resTp: Type, pos: SrcPos): (Int, Type, Type) = { var resultTp = resTp var elemTp = unapplySeqTypeElemTp(resultTp) var arity = productArity(resultTp, pos) @@ -501,19 +504,20 @@ class SpaceEngine(using Context) extends SpaceLogic { } /** Numeric literals, while being constant values of unrelated types (e.g. Char and Int), - * when used in a case may end up matching at runtime, because their equals may returns true. - * Because these are universally available, general purpose types, it would be good to avoid - * returning false positive warnings, such as in `(c: Char) match { case 67 => ... }` emitting a + * when used in a case may end up matching at runtime as their equals may returns true. + * Because these are universally available, general purpose types, it would be good to avoid, + * for example in `(c: Char) match { case 67 => ... }`, emitting a false positive * reachability warning on the case. So the type `ConstantType(Constant(67, IntTag))` is * converted to `ConstantType(Constant(67, CharTag))`. #12805 */ - def convertConstantType(tp: Type, pt: Type): Type = tp match + def convertConstantType(tp: Type, pt: Type): Type = trace(i"convertConstantType($tp, $pt)", show = true)(tp match case tp @ ConstantType(const) => val converted = const.convertTo(pt) if converted == null then tp else ConstantType(converted) case _ => tp + ) - def isPrimToBox(tp: Type, pt: Type) = - tp.classSymbol.isPrimitiveValueClass && (defn.boxedType(tp).classSymbol eq pt.classSymbol) + def isPrimToBox(tp: Type, pt: Type): Boolean = + tp.isPrimitiveValueType && (defn.boxedType(tp).classSymbol eq pt.classSymbol) /** Adapt types by performing primitive value unboxing or boxing, or numeric constant conversion. #12805 * @@ -539,7 +543,10 @@ class SpaceEngine(using Context) extends SpaceLogic { def isSubType(tp1: Type, tp2: Type): Boolean = trace(i"$tp1 <:< $tp2", debug, show = true) { if tp1 == constantNullType && !ctx.mode.is(Mode.SafeNulls) then tp2 == constantNullType - else adaptType(tp1, tp2) <:< tp2 + else + val tp1a = adaptType(tp1, tp2) + if tp1a eq tp1 then tp1 <:< tp2 + else trace(i"$tp1a <:< $tp2 (adapted)", debug, show = true)(tp1a <:< tp2) } def isSameUnapply(tp1: TermRef, tp2: TermRef): Boolean = @@ -872,7 +879,7 @@ class SpaceEngine(using Context) extends SpaceLogic { /** Return the underlying type of non-module, non-constant, non-enum case singleton types. * Also widen ExprType to its result type, and rewrap any annotation wrappers. * For example, with `val opt = None`, widen `opt.type` to `None.type`. */ - def toUnderlying(tp: Type)(using Context): Type = trace(i"toUnderlying($tp)", show = true)(tp match { + def toUnderlying(tp: Type): Type = trace(i"toUnderlying($tp)", show = true)(tp match { case _: ConstantType => tp case tp: TermRef if tp.symbol.is(Module) => tp case tp: TermRef if tp.symbol.isAllOf(EnumCase) => tp diff --git a/tests/patmat/i14407.dupe.check b/tests/patmat/i14407.dupe.check new file mode 100644 index 000000000000..b0605bcd95e5 --- /dev/null +++ b/tests/patmat/i14407.dupe.check @@ -0,0 +1 @@ +6: Match case Unreachable diff --git a/tests/patmat/i14407.dupe.scala b/tests/patmat/i14407.dupe.scala new file mode 100644 index 000000000000..acc019e41e8a --- /dev/null +++ b/tests/patmat/i14407.dupe.scala @@ -0,0 +1,7 @@ +// scalac: -Werror +@main def Test = + val it: Int = 42 + 42L match + case `it` => println("pass") + case `it` => println("dupe") // error + case _ => println("fail") diff --git a/tests/patmat/i14407.scala b/tests/patmat/i14407.scala new file mode 100644 index 000000000000..a1f710bd6acb --- /dev/null +++ b/tests/patmat/i14407.scala @@ -0,0 +1,6 @@ +// scalac: -Werror +@main def Test = + val it: Int = 42 + 42L match + case `it` => println("pass") + case _ => println("fail") From 9ff512b06c799eb2acac18cc37541f71571959ef Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Thu, 24 Feb 2022 16:35:30 +0000 Subject: [PATCH 2/2] Strip back adapting & converting types in match analysis Rather than trying to fix the types such that subtyping returns true for these cases, we can rely on the numeric opt-out of using provablyDisjoint and extend it to boxed numerics. --- .../tools/dotc/transform/patmat/Space.scala | 38 ++----------------- .../dotc/transform/SpaceEngineTest.scala | 25 ------------ 2 files changed, 4 insertions(+), 59 deletions(-) delete mode 100644 compiler/test/dotty/tools/dotc/transform/SpaceEngineTest.scala diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index a6948db799c3..e69f563de149 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -334,9 +334,10 @@ class SpaceEngine(using Context) extends SpaceLogic { // Since projections of types don't include null, intersection with null is empty. Empty else - val intersection = Typ(AndType(tp1, tp2), decomposed = true) - // unrelated numeric value classes can equal each other, so let's not consider type space interection empty + val intersection = Typ(AndType(tp1, tp2), decomposed = false) + // unrelated numeric value classes can equal each other, so let's not consider type space intersection empty if tp1.classSymbol.isNumericValueClass && tp2.classSymbol.isNumericValueClass then intersection + else if isPrimToBox(tp1, tp2) || isPrimToBox(tp2, tp1) then intersection else if TypeComparer.provablyDisjoint(tp1, tp2) then Empty else intersection } @@ -503,37 +504,9 @@ class SpaceEngine(using Context) extends SpaceLogic { } } - /** Numeric literals, while being constant values of unrelated types (e.g. Char and Int), - * when used in a case may end up matching at runtime as their equals may returns true. - * Because these are universally available, general purpose types, it would be good to avoid, - * for example in `(c: Char) match { case 67 => ... }`, emitting a false positive - * reachability warning on the case. So the type `ConstantType(Constant(67, IntTag))` is - * converted to `ConstantType(Constant(67, CharTag))`. #12805 */ - def convertConstantType(tp: Type, pt: Type): Type = trace(i"convertConstantType($tp, $pt)", show = true)(tp match - case tp @ ConstantType(const) => - val converted = const.convertTo(pt) - if converted == null then tp else ConstantType(converted) - case _ => tp - ) - def isPrimToBox(tp: Type, pt: Type): Boolean = tp.isPrimitiveValueType && (defn.boxedType(tp).classSymbol eq pt.classSymbol) - /** Adapt types by performing primitive value unboxing or boxing, or numeric constant conversion. #12805 - * - * This makes these isSubType cases work like this: - * {{{ - * 1 <:< Integer => ( : Integer) <:< Integer = true - * ONE <:< Int => ( : Int) <:< Int = true - * Integer <:< (1: Int) => ( : Int) <:< (1: Int) = false - * }}} - */ - def adaptType(tp1: Type, tp2: Type): Type = trace(i"adaptType($tp1, $tp2)", show = true) { - if isPrimToBox(tp1, tp2) then defn.boxedType(tp1).narrow - else if isPrimToBox(tp2, tp1) then defn.unboxedType(tp1).narrow - else convertConstantType(tp1, tp2) - } - private val isSubspaceCache = mutable.HashMap.empty[(Space, Space, Context), Boolean] override def isSubspace(a: Space, b: Space)(using Context): Boolean = @@ -543,10 +516,7 @@ class SpaceEngine(using Context) extends SpaceLogic { def isSubType(tp1: Type, tp2: Type): Boolean = trace(i"$tp1 <:< $tp2", debug, show = true) { if tp1 == constantNullType && !ctx.mode.is(Mode.SafeNulls) then tp2 == constantNullType - else - val tp1a = adaptType(tp1, tp2) - if tp1a eq tp1 then tp1 <:< tp2 - else trace(i"$tp1a <:< $tp2 (adapted)", debug, show = true)(tp1a <:< tp2) + else tp1 <:< tp2 } def isSameUnapply(tp1: TermRef, tp2: TermRef): Boolean = diff --git a/compiler/test/dotty/tools/dotc/transform/SpaceEngineTest.scala b/compiler/test/dotty/tools/dotc/transform/SpaceEngineTest.scala deleted file mode 100644 index 09fb5ee960fd..000000000000 --- a/compiler/test/dotty/tools/dotc/transform/SpaceEngineTest.scala +++ /dev/null @@ -1,25 +0,0 @@ -package dotty.tools -package dotc -package transform - -import org.junit.*, Assert.* - -import core.*, Constants.*, Contexts.*, Decorators.*, Symbols.*, Types.* - -class SpaceEngineTest extends DottyTest: - @Test def testAdaptTest(): Unit = - given Context = ctx - val defn = ctx.definitions - import defn._ - val e = patmat.SpaceEngine() - - val BoxedIntType = BoxedIntClass.typeRef - val ConstOneType = ConstantType(Constant(1)) - - assertTrue(e.isPrimToBox(IntType, BoxedIntType)) - assertFalse(e.isPrimToBox(BoxedIntType, IntType)) - assertTrue(e.isPrimToBox(ConstOneType, BoxedIntType)) - - assertEquals(BoxedIntType, e.adaptType(IntType, BoxedIntType).widenSingleton) - assertEquals(IntType, e.adaptType(BoxedIntType, IntType).widenSingleton) - assertEquals(IntType, e.adaptType(BoxedIntType, ConstOneType).widenSingleton)