From d17d0dc7e025dc8abd55b8c6f8fbd2b09a38ebf3 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Thu, 12 Aug 2021 21:49:54 +0100 Subject: [PATCH 1/7] Restore unreachable warnings by converting literals The fix implemented to deal with primitive types auto-converting was suppressing a series of legitimate unreachable warnings. So we removed that fix and tried to mirror the auto-conversion of the type, such that the space intersection calculation ends up with the correct results. In doing so we triggered reachability warnings being emitted by sealed trait companion object (or enum) `ordinal` synthetic methods, in some weird test cases. So we added an `@unchecked` annotation to suppress those. Co-authored-by: Seth Tisue --- .../tools/dotc/transform/patmat/Space.scala | 28 +++++++++++++------ tests/patmat/i12805.check | 3 ++ tests/patmat/i12805.scala | 22 +++++++++++++++ tests/patmat/i12805b.check | 3 ++ tests/patmat/i12805b.scala | 14 ++++++++++ 5 files changed, 62 insertions(+), 8 deletions(-) create mode 100644 tests/patmat/i12805.check create mode 100644 tests/patmat/i12805.scala create mode 100644 tests/patmat/i12805b.check create mode 100644 tests/patmat/i12805b.scala diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index af02bfed8411..804a4fd33224 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -503,8 +503,21 @@ 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 + * 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 + case tp @ ConstantType(const) => + val converted = const.convertTo(pt) + if converted == null then tp else ConstantType(converted) + case _ => tp + /** Is `tp1` a subtype of `tp2`? */ - def isSubType(tp1: Type, tp2: Type): Boolean = { + def isSubType(_tp1: Type, tp2: Type): Boolean = { + val tp1 = convertConstantType(_tp1, tp2) debug.println(TypeComparer.explained(_.isSubType(tp1, tp2))) val res = if (ctx.explicitNulls) { tp1 <:< tp2 @@ -750,6 +763,7 @@ class SpaceEngine(using Context) extends SpaceLogic { } def show(ss: Seq[Space]): String = ss.map(show).mkString(", ") + /** Display spaces */ def show(s: Space): String = { def params(tp: Type): List[Type] = tp.classSymbol.primaryConstructor.info.firstParamTypes @@ -894,13 +908,15 @@ class SpaceEngine(using Context) extends SpaceLogic { else project(OrType(selTyp, constantNullType, soft = false)) + debug.println(s"targetSpace: ${show(targetSpace)}") + // in redundancy check, take guard as false in order to soundly approximate val spaces = cases.map { x => val res = if (x.guard.isEmpty) project(x.pat) else Empty - debug.println(s"${x.pat.show} ====> ${res}") + debug.println(s"${x.pat.show} ====> ${show(res)}") res } @@ -914,12 +930,8 @@ class SpaceEngine(using Context) extends SpaceLogic { debug.println(s"---------------reachable? ${show(curr)}") debug.println(s"prev: ${show(prevs)}") - var covered = simplify(intersect(curr, targetSpace)) - debug.println(s"covered: $covered") - - // `covered == Empty` may happen for primitive types with auto-conversion - // see tests/patmat/reader.scala tests/patmat/byte.scala - if (covered == Empty && !isNullLit(pat)) covered = curr + val covered = simplify(intersect(curr, targetSpace)) + debug.println(s"covered: ${show(covered)}") if (isSubspace(covered, prevs)) { if i == cases.length - 1 diff --git a/tests/patmat/i12805.check b/tests/patmat/i12805.check new file mode 100644 index 000000000000..e855c765d801 --- /dev/null +++ b/tests/patmat/i12805.check @@ -0,0 +1,3 @@ +10: Match case Unreachable +16: Match case Unreachable +22: Match case Unreachable diff --git a/tests/patmat/i12805.scala b/tests/patmat/i12805.scala new file mode 100644 index 000000000000..78240c2f9703 --- /dev/null +++ b/tests/patmat/i12805.scala @@ -0,0 +1,22 @@ +import scala.language.implicitConversions + +type Timeframe = "1m" | "2m" | "1H" +type TimeframeN = 1 | 2 | 60 + +def manualConvertToN(tf: Timeframe): TimeframeN = tf match + case "1m" => 1 + case "2m" => 2 + case "1H" => 60 + case "4H" => ??? // was: no reachability warning + +given Conversion[Timeframe, TimeframeN] = + case "1m" => 1 + case "2m" => 2 + case "1H" => 60 + case "4H" => ??? // was: no reachability warning + +given Conversion[TimeframeN, Timeframe] = + case 1 => "1m" + case 2 => "2m" + case 60 => "1H" + case 240 => ??? // was: no reachability warning diff --git a/tests/patmat/i12805b.check b/tests/patmat/i12805b.check new file mode 100644 index 000000000000..62ba8523f65e --- /dev/null +++ b/tests/patmat/i12805b.check @@ -0,0 +1,3 @@ +4: Match case Unreachable +9: Match case Unreachable +14: Match case Unreachable diff --git a/tests/patmat/i12805b.scala b/tests/patmat/i12805b.scala new file mode 100644 index 000000000000..cbff6ae0d2ca --- /dev/null +++ b/tests/patmat/i12805b.scala @@ -0,0 +1,14 @@ +def test1(a: 1 | 2) = a match + case 1 => true + case 2 => false + case 4 => ??? // unreachable case, was: no warning + +def test2(a: 1 | 2) = a match + case 1 => true + case 2 => false + case _ => ??? // unreachable + +def test3(a: 1 | 2) = a match + case 1 => true + case 2 => false + case a if a < 0 => ??? // unreachable From 5fb2ac0ec62258c877225aa45c45768bee7efefa Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 18 Aug 2021 16:42:29 +0100 Subject: [PATCH 2/7] Add a test case representing the fallout seen in bootstrapping --- tests/patmat/i12805-fallout.scala | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 tests/patmat/i12805-fallout.scala diff --git a/tests/patmat/i12805-fallout.scala b/tests/patmat/i12805-fallout.scala new file mode 100644 index 000000000000..a8a322c3f98a --- /dev/null +++ b/tests/patmat/i12805-fallout.scala @@ -0,0 +1,26 @@ +import scala.annotation.unchecked.uncheckedVariance + +type Untyped = Null + +class Type + +abstract class Tree[-T >: Untyped] { + type ThisTree[T >: Untyped] <: Tree[T] + + protected var myTpe: T @uncheckedVariance = _ + + def withType(tpe: Type): ThisTree[Type] = { + val tree = this.asInstanceOf[ThisTree[Type]] + tree.myTpe = tpe + tree + } +} + +case class Ident[-T >: Untyped]() extends Tree[T] +case class DefDef[-T >: Untyped]() extends Tree[T] + +def test[T >: Untyped](tree: Tree[T], tp: Type) = tree.withType(tp) match { + case Ident() => 1 + case DefDef() => 2 + case _ => 3 +} From a945326033c6ae8ef73b7b4bcb710ef154eacfa1 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 25 Aug 2021 17:21:37 +0100 Subject: [PATCH 3/7] Tweak reachability to only warn after the first reachable case In the event that the first case is unreachable, the logic descended to `isSubspace(covered, prevs)` where both are the Empty Space, emitting warnings. Instead we implement reachability like the Scala 2 compiler defines it: // a case is unreachable if it implies its preceding cases --- .../src/dotty/tools/dotc/transform/patmat/Space.scala | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index 804a4fd33224..c702456b7626 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -922,10 +922,13 @@ class SpaceEngine(using Context) extends SpaceLogic { (1 until cases.length).foreach { i => val pat = cases(i).pat - - if (pat != EmptyTree) { // rethrow case of catch uses EmptyTree - val prevs = Or(spaces.take(i)) - val curr = project(pat) + val prevs = Or(spaces.take(i)) + if (pat != EmptyTree // rethrow case of catch uses EmptyTree + && simplify(intersect(prevs, targetSpace)) != Empty + // it's required that at one of the previous cases is reachable (its intersected Space isn't Empty) + // because if all the previous cases are unreachable then case i can't be unreachable + ) { + val curr = project(pat) // TODO(dnw) reuse `spaces(i)` & avoid re-computing? Or is mutability present? debug.println(s"---------------reachable? ${show(curr)}") debug.println(s"prev: ${show(prevs)}") From 202b63e50dac73d14bbbe0f426eef49c3133d2ab Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 25 Aug 2021 18:19:04 +0100 Subject: [PATCH 4/7] Change Space intersect so that the fallback for extractors matches the fallback for type spaces --- .../dotty/tools/dotc/transform/patmat/Space.scala | 6 +++--- tests/patmat/i12805-fallout.scala | 14 +++++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index c702456b7626..279e8a87e7e0 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -212,14 +212,14 @@ trait SpaceLogic { if (isSubType(tp2, tp1)) b else if (canDecompose(tp1)) tryDecompose1(tp1) else if (isSubType(tp1, tp2)) a // problematic corner case: inheriting a case class - else Empty + else intersectUnrelatedAtomicTypes(tp1, tp2) case (Prod(tp1, fun, ss), Typ(tp2, _)) => if (isSubType(tp1, tp2)) a else if (canDecompose(tp2)) tryDecompose2(tp2) else if (isSubType(tp2, tp1)) a // problematic corner case: inheriting a case class - else Empty + else intersectUnrelatedAtomicTypes(tp1, tp2) case (Prod(tp1, fun1, ss1), Prod(tp2, fun2, ss2)) => - if (!isSameUnapply(fun1, fun2)) Empty + if (!isSameUnapply(fun1, fun2)) intersectUnrelatedAtomicTypes(tp1, tp2) else if (ss1.zip(ss2).exists(p => simplify(intersect(p._1, p._2)) == Empty)) Empty else Prod(tp1, fun1, ss1.zip(ss2).map((intersect _).tupled)) } diff --git a/tests/patmat/i12805-fallout.scala b/tests/patmat/i12805-fallout.scala index a8a322c3f98a..b598b36159ea 100644 --- a/tests/patmat/i12805-fallout.scala +++ b/tests/patmat/i12805-fallout.scala @@ -16,11 +16,15 @@ abstract class Tree[-T >: Untyped] { } } -case class Ident[-T >: Untyped]() extends Tree[T] -case class DefDef[-T >: Untyped]() extends Tree[T] +case class Ident[-T >: Untyped]() extends Tree[T] +case class DefDef[-T >: Untyped]() extends Tree[T] +case class Inlined[-T >: Untyped]() extends Tree[T] +case class CaseDef[-T >: Untyped]() extends Tree[T] def test[T >: Untyped](tree: Tree[T], tp: Type) = tree.withType(tp) match { - case Ident() => 1 - case DefDef() => 2 - case _ => 3 + case Ident() => 1 + case DefDef() => 2 + case _: Inlined[_] => 3 + case CaseDef() => 4 + case _ => 5 } From 4b0f32f044f5f5a4849854e5d0f0a2bd1c782fcd Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Thu, 26 Aug 2021 07:40:07 +0100 Subject: [PATCH 5/7] Add filtering & checkfile update to PatmatExhaustivityTest --- .../dotc/transform/PatmatExhaustivityTest.scala | 8 ++++++-- compiler/test/dotty/tools/vulpix/FileDiff.scala | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/compiler/test/dotty/tools/dotc/transform/PatmatExhaustivityTest.scala b/compiler/test/dotty/tools/dotc/transform/PatmatExhaustivityTest.scala index 7938a4e5af47..3d877a46558f 100644 --- a/compiler/test/dotty/tools/dotc/transform/PatmatExhaustivityTest.scala +++ b/compiler/test/dotty/tools/dotc/transform/PatmatExhaustivityTest.scala @@ -45,7 +45,7 @@ class PatmatExhaustivityTest { val baseFilePath = path.toString.stripSuffix(".scala") val checkFilePath = baseFilePath + ".check" - FileDiff.checkAndDump(path.toString, actualLines, checkFilePath) + FileDiff.checkAndDumpOrUpdate(path.toString, actualLines, checkFilePath) } /** A single test with multiple files grouped in a folder */ @@ -57,13 +57,17 @@ class PatmatExhaustivityTest { val actualLines = compile(files) val checkFilePath = s"${path}${File.separator}expected.check" - FileDiff.checkAndDump(path.toString, actualLines, checkFilePath) + FileDiff.checkAndDumpOrUpdate(path.toString, actualLines, checkFilePath) } @Test def patmatExhaustivity: Unit = { val res = Directory(testsDir).list.toList .filter(f => f.extension == "scala" || f.isDirectory) + .filter { f => + val path = if f.isDirectory then f.path + "/" else f.path + path.contains(Properties.testsFilter.getOrElse("")) + } .map(f => if f.isDirectory then compileDir(f.jpath) else compileFile(f.jpath)) val failed = res.filter(!_) diff --git a/compiler/test/dotty/tools/vulpix/FileDiff.scala b/compiler/test/dotty/tools/vulpix/FileDiff.scala index af44dbf2076d..835e8c6a2d6f 100644 --- a/compiler/test/dotty/tools/vulpix/FileDiff.scala +++ b/compiler/test/dotty/tools/vulpix/FileDiff.scala @@ -63,4 +63,21 @@ object FileDiff { } } + def checkAndDumpOrUpdate(sourceTitle: String, actualLines: Seq[String], checkFilePath: String): Boolean = { + val outFilePath = checkFilePath + ".out" + FileDiff.check(sourceTitle, actualLines, checkFilePath) match { + case Some(msg) if dotty.Properties.testsUpdateCheckfile => + FileDiff.dump(checkFilePath, actualLines) + println("Updated checkfile: " + checkFilePath) + false + case Some(msg) => + FileDiff.dump(outFilePath, actualLines) + println(msg) + println(FileDiff.diffMessage(checkFilePath, outFilePath)) + false + case _ => + Files.deleteIfExists(Paths.get(outFilePath)) + true + } + } } From be658f66f4c64bb8cd47b1fbb4399b351d99e2aa Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Thu, 26 Aug 2021 18:58:15 +0100 Subject: [PATCH 6/7] Also perform primitive value boxing --- .../tools/dotc/transform/patmat/Space.scala | 16 +++++++++++----- tests/patmat/boxing.scala | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) create mode 100644 tests/patmat/boxing.scala diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index 279e8a87e7e0..b8cf309247a8 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -165,7 +165,7 @@ trait SpaceLogic { } /** Is `a` a subspace of `b`? Equivalent to `a - b == Empty`, but faster */ - def isSubspace(a: Space, b: Space)(using Context): Boolean = trace(s"${show(a)} < ${show(b)}", debug) { + def isSubspace(a: Space, b: Space)(using Context): Boolean = trace(s"isSubspace(${show(a)}, ${show(b)})", debug) { def tryDecompose1(tp: Type) = canDecompose(tp) && isSubspace(Or(decompose(tp)), b) def tryDecompose2(tp: Type) = canDecompose(tp) && isSubspace(a, Or(decompose(tp))) @@ -515,15 +515,22 @@ class SpaceEngine(using Context) extends SpaceLogic { if converted == null then tp else ConstantType(converted) case _ => tp + /** Adapt types by performing primitive value boxing. #12805 */ + def maybeBox(tp1: Type, tp2: Type): Type = + if tp1.classSymbol.isPrimitiveValueClass && !tp2.classSymbol.isPrimitiveValueClass then + defn.boxedType(tp1).narrow + else tp1 + /** Is `tp1` a subtype of `tp2`? */ def isSubType(_tp1: Type, tp2: Type): Boolean = { - val tp1 = convertConstantType(_tp1, tp2) - debug.println(TypeComparer.explained(_.isSubType(tp1, tp2))) + val tp1 = maybeBox(convertConstantType(_tp1, tp2), tp2) + //debug.println(TypeComparer.explained(_.isSubType(tp1, tp2))) val res = if (ctx.explicitNulls) { tp1 <:< tp2 } else { (tp1 != constantNullType || tp2 == constantNullType) && tp1 <:< tp2 } + debug.println(i"$tp1 <:< $tp2 = $res") res } @@ -663,7 +670,6 @@ class SpaceEngine(using Context) extends SpaceLogic { parts.map(Typ(_, true)) } - /** Abstract sealed types, or-types, Boolean and Java enums can be decomposed */ def canDecompose(tp: Type): Boolean = val res = tp.dealias match @@ -679,7 +685,7 @@ class SpaceEngine(using Context) extends SpaceLogic { || cls.isAllOf(JavaEnumTrait) || tp.isRef(defn.BooleanClass) || tp.isRef(defn.UnitClass) - debug.println(s"decomposable: ${tp.show} = $res") + //debug.println(s"decomposable: ${tp.show} = $res") res /** Show friendly type name with current scope in mind diff --git a/tests/patmat/boxing.scala b/tests/patmat/boxing.scala new file mode 100644 index 000000000000..d148e23deca1 --- /dev/null +++ b/tests/patmat/boxing.scala @@ -0,0 +1,18 @@ +class C { + def matchUnboxed(i: Integer) = i match { + case null => 0 + case 1 => 1 + case _ => 9 + } + + def matchBoxed(i: Int) = i match { + case C.ZERO => 0 + case C.ONE => 1 + case _ => 9 + } +} + +object C { + final val ZERO: Integer = 0 + final val ONE: Integer = 1 +} From 87769061ac74def512657f3c44af8d73ba29b59a Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Mon, 30 Aug 2021 15:47:39 +0100 Subject: [PATCH 7/7] Reuse computation in checkRedundancy --- .../tools/dotc/transform/patmat/Space.scala | 64 ++++++++----------- 1 file changed, 25 insertions(+), 39 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index b8cf309247a8..075dad5b3acf 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -908,50 +908,36 @@ class SpaceEngine(using Context) extends SpaceLogic { if (!redundancyCheckable(sel)) return - val targetSpace = - if !selTyp.classSymbol.isNullableClass then - project(selTyp) - else - project(OrType(selTyp, constantNullType, soft = false)) - + val isNullable = selTyp.classSymbol.isNullableClass + val targetSpace = if isNullable + then project(OrType(selTyp, constantNullType, soft = false)) + else project(selTyp) debug.println(s"targetSpace: ${show(targetSpace)}") - // in redundancy check, take guard as false in order to soundly approximate - val spaces = cases.map { x => - val res = - if (x.guard.isEmpty) project(x.pat) - else Empty + cases.iterator.zipWithIndex.foldLeft(Nil: List[Space]) { case (prevs, (CaseDef(pat, guard, _), i)) => + debug.println(i"case pattern: $pat") - debug.println(s"${x.pat.show} ====> ${show(res)}") - res - } + val curr = project(pat) + debug.println(i"reachable? ${show(curr)}") - (1 until cases.length).foreach { i => - val pat = cases(i).pat - val prevs = Or(spaces.take(i)) - if (pat != EmptyTree // rethrow case of catch uses EmptyTree - && simplify(intersect(prevs, targetSpace)) != Empty - // it's required that at one of the previous cases is reachable (its intersected Space isn't Empty) - // because if all the previous cases are unreachable then case i can't be unreachable - ) { - val curr = project(pat) // TODO(dnw) reuse `spaces(i)` & avoid re-computing? Or is mutability present? - - debug.println(s"---------------reachable? ${show(curr)}") - debug.println(s"prev: ${show(prevs)}") - - val covered = simplify(intersect(curr, targetSpace)) - debug.println(s"covered: ${show(covered)}") - - if (isSubspace(covered, prevs)) { - if i == cases.length - 1 - && isWildcardArg(pat) - && pat.tpe.classSymbol.isNullableClass - then - report.warning(MatchCaseOnlyNullWarning(), pat.srcPos) - else - report.warning(MatchCaseUnreachable(), pat.srcPos) - } + val prev = simplify(Or(prevs)) + debug.println(s"prev: ${show(prev)}") + + val covered = simplify(intersect(curr, targetSpace)) + debug.println(s"covered: ${show(covered)}") + + if pat != EmptyTree // rethrow case of catch uses EmptyTree + && prev != Empty // avoid isSubspace(Empty, Empty) - one of the previous cases much be reachable + && isSubspace(covered, prev) + then { + if isNullable && i == cases.length - 1 && isWildcardArg(pat) then + report.warning(MatchCaseOnlyNullWarning(), pat.srcPos) + else + report.warning(MatchCaseUnreachable(), pat.srcPos) } + + // in redundancy check, take guard as false in order to soundly approximate + (if guard.isEmpty then covered else Empty) :: prevs } } }