Skip to content

Emit deferred reachability warnings & fix boxing/unboxing adapting #13485

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 7 commits into from
Sep 13, 2021
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
14 changes: 14 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1731,6 +1731,20 @@ class Definitions {
else sys.error(s"Not a primitive value type: $tp")
}.typeRef

def unboxedType(tp: Type)(using Context): TypeRef = {
val cls = tp.classSymbol
if (cls eq BoxedByteClass) ByteType
else if (cls eq BoxedShortClass) ShortType
else if (cls eq BoxedCharClass) CharType
else if (cls eq BoxedIntClass) IntType
else if (cls eq BoxedLongClass) LongType
else if (cls eq BoxedFloatClass) FloatType
else if (cls eq BoxedDoubleClass) DoubleType
else if (cls eq BoxedUnitClass) UnitType
else if (cls eq BoxedBooleanClass) BooleanType
else sys.error(s"Not a boxed primitive value type: $tp")
}

/** The JVM tag for `tp` if it's a primitive, `java.lang.Object` otherwise. */
def typeTag(tp: Type)(using Context): Name = typeTags(scalaClassName(tp))

Expand Down
69 changes: 44 additions & 25 deletions compiler/src/dotty/tools/dotc/transform/patmat/Space.scala
Original file line number Diff line number Diff line change
Expand Up @@ -512,23 +512,28 @@ 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
def isPrimToBox(tp: Type, pt: Type) =
tp.classSymbol.isPrimitiveValueClass && (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 => (<skolem> : Integer) <:< Integer = true
* ONE <:< Int => (<skolem> : Int) <:< Int = true
* Integer <:< (1: Int) => (<skolem> : 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)
}

/** Is `tp1` a subtype of `tp2`? */
def isSubType(_tp1: Type, tp2: Type): Boolean = {
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
def isSubType(tp1: Type, tp2: Type): Boolean = trace(i"$tp1 <:< $tp2", debug, show = true) {
if tp1 == constantNullType && !ctx.explicitNulls then tp2 == constantNullType
else adaptType(tp1, tp2) <:< tp2
}

def isSameUnapply(tp1: TermRef, tp2: TermRef): Boolean =
Expand Down Expand Up @@ -911,7 +916,8 @@ class SpaceEngine(using Context) extends SpaceLogic {
&& !sel.tpe.widen.isRef(defn.QuotedTypeClass)

def checkRedundancy(_match: Match): Unit = {
val Match(sel, cases) = _match
val Match(sel, _) = _match
val cases = _match.cases.toIndexedSeq
debug.println(i"checking redundancy in $_match")

if (!redundancyCheckable(sel)) return
Expand All @@ -925,7 +931,14 @@ class SpaceEngine(using Context) extends SpaceLogic {
else project(selTyp)
debug.println(s"targetSpace: ${show(targetSpace)}")

cases.iterator.zipWithIndex.foldLeft(Nil: List[Space]) { case (prevs, (CaseDef(pat, guard, _), i)) =>
var i = 0
val len = cases.length
var prevs = List.empty[Space]
var deferred = List.empty[Tree]

while (i < len) {
val CaseDef(pat, guard, _) = cases(i)

debug.println(i"case pattern: $pat")

val curr = project(pat)
Expand All @@ -937,18 +950,24 @@ class SpaceEngine(using Context) extends SpaceLogic {
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
if prev == Empty && covered == Empty then // defer until a case is reachable
deferred ::= pat
else {
for (pat <- deferred.reverseIterator)
report.warning(MatchCaseUnreachable(), pat.srcPos)
if pat != EmptyTree // rethrow case of catch uses EmptyTree
&& isSubspace(covered, prev)
then {
val nullOnly = isNullable && i == len - 1 && isWildcardArg(pat)
val msg = if nullOnly then MatchCaseOnlyNullWarning() else MatchCaseUnreachable()
report.warning(msg, pat.srcPos)
}
deferred = Nil
}

// in redundancy check, take guard as false in order to soundly approximate
(if guard.isEmpty then covered else Empty) :: prevs
prevs ::= (if guard.isEmpty then covered else Empty)
i += 1
}
}
}
25 changes: 25 additions & 0 deletions compiler/test/dotty/tools/dotc/transform/SpaceEngineTest.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
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)
7 changes: 5 additions & 2 deletions compiler/test/dotty/tools/vulpix/FileDiff.scala
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,12 @@ object FileDiff {
val outFilePath = checkFilePath + ".out"
FileDiff.check(sourceTitle, actualLines, checkFilePath) match {
case Some(msg) if dotty.Properties.testsUpdateCheckfile =>
FileDiff.dump(checkFilePath, actualLines)
Files.deleteIfExists(Paths.get(outFilePath))
if actualLines.isEmpty
then Files.deleteIfExists(Paths.get(checkFilePath))
else FileDiff.dump(checkFilePath, actualLines)
println("Updated checkfile: " + checkFilePath)
false
true
case Some(msg) =>
FileDiff.dump(outFilePath, actualLines)
println(msg)
Expand Down
12 changes: 6 additions & 6 deletions tests/neg-custom-args/fatal-warnings/i8711.check
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
-- Error: tests/neg-custom-args/fatal-warnings/i8711.scala:7:9 ---------------------------------------------------------
-- [E030] Match case Unreachable Error: tests/neg-custom-args/fatal-warnings/i8711.scala:7:9 ---------------------------
7 | case x: B => x // error: this case is unreachable since class A is not a subclass of class B
| ^
| this case is unreachable since type A and class B are unrelated
-- Error: tests/neg-custom-args/fatal-warnings/i8711.scala:12:9 --------------------------------------------------------
| ^^^^
| Unreachable case
-- [E030] Match case Unreachable Error: tests/neg-custom-args/fatal-warnings/i8711.scala:12:9 --------------------------
12 | case x: C => x // error
| ^
| this case is unreachable since type A | B and class C are unrelated
| ^^^^
| Unreachable case
2 changes: 2 additions & 0 deletions tests/patmat/i13485.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
11: Match case Unreachable
16: Match case Unreachable
16 changes: 16 additions & 0 deletions tests/patmat/i13485.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// The intent of this test is test that changing the order of cases doesn't affect whether
// warnings, originally reachability warnings but exhaustivity warnings too, are emitted.
// To do so we need a case that typechecks but is statically assessed to be unreachable.
// How about... a type pattern on a sealed trait that the scrutinee type doesn't extend?

sealed trait Foo

class Bar

def test1(bar: Bar) = bar match
case _: Foo => 1
case _: Bar => 2

def test2(bar: Bar) = bar match
case _: Bar => 2
case _: Foo => 1