Fix scala#9776: Don't issue a @switch warning when fewer than 4 cases

In the spirit of scala/scala#4418, which fixed SI-8731. The consensus seems to be that the intent of the @switch annotation is to warn when the generated bytecode may be poor, not merely if the compiler elects to not emit a tableswitch/lookupswitch. Note that the case threshold for determining whether a switch is emitted is implementation dependent, and currently varies between scalac and dotc. Also note that this implementation will not warn in instances where a switch will never be emitted (e.g. because the match is on a non-integral type) but the number of cases is below the warning threshold. This behavior is consistent with scalac, but may be surprising to the user if another case is added later and a warning suddenly appears. Not all spurious @switch warnings are addressed by this commit, see issue scala#5070 for an example.
griggt · Jan 11, 2021 · e298e21 · e298e21
1 parent 609d8d6
commit e298e21
Show file tree

Hide file tree

Showing 12 changed files with 109 additions and 39 deletions.
diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala
@@ -2029,10 +2029,9 @@ import transform.SymUtils._
           |whose behavior may have changed since version change."""
   }
 
-  class UnableToEmitSwitch(tooFewCases: Boolean)(using Context)
+  class UnableToEmitSwitch()(using Context)
   extends SyntaxMsg(UnableToEmitSwitchID) {
-    def tooFewStr: String = if (tooFewCases) " since there are not enough cases" else ""
-    def msg = em"Could not emit switch for ${hl("@switch")} annotated match$tooFewStr"
+    def msg = em"Could not emit switch for ${hl("@switch")} annotated match"
     def explain = {
       val codeExample =
         """val ConstantB = 'B'

diff --git a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala
@@ -976,30 +976,28 @@ object PatternMatcher {
     /** If match is switch annotated, check that it translates to a switch
      *  with at least as many cases as the original match.
      */
-    private def checkSwitch(original: Match, result: Tree) = original.selector match {
+    private def checkSwitch(original: Match, result: Tree) = original.selector match
       case Typed(_, tpt) if tpt.tpe.hasAnnotation(defn.SwitchAnnot) =>
-        val resultCases = result match {
+        val resultCases = result match
           case Match(_, cases) => cases
           case Block(_, Match(_, cases)) => cases
           case _ => Nil
-        }
-        def typesInPattern(pat: Tree): List[Type] = pat match {
+        def typesInPattern(pat: Tree): List[Type] = pat match
           case Alternative(pats) => pats.flatMap(typesInPattern)
           case _ => pat.tpe :: Nil
-        }
         def typesInCases(cdefs: List[CaseDef]): List[Type] =
           cdefs.flatMap(cdef => typesInPattern(cdef.pat))
         def numTypes(cdefs: List[CaseDef]): Int =
           typesInCases(cdefs).toSet.size: Int // without the type ascription, testPickling fails because of #2840.
-        if (numTypes(resultCases) < numTypes(original.cases)) {
+        val numTypesInOriginal = numTypes(original.cases)
+        if numTypesInOriginal >= MinSwitchCases && numTypes(resultCases) < numTypesInOriginal then
           patmatch.println(i"switch warning for ${ctx.compilationUnit}")
           patmatch.println(i"original types: ${typesInCases(original.cases)}%, %")
           patmatch.println(i"switch types  : ${typesInCases(resultCases)}%, %")
           patmatch.println(i"tree = $result")
-          report.warning(UnableToEmitSwitch(numTypes(original.cases) < MinSwitchCases), original.srcPos)
-        }
+          report.warning(UnableToEmitSwitch(), original.srcPos)
       case _ =>
-    }
+    end checkSwitch
 
     val optimizations: List[(String, Plan => Plan)] = List(
       "mergeTests" -> mergeTests,

diff --git a/tests/neg-custom-args/fatal-warnings/i3561.scala b/tests/neg-custom-args/fatal-warnings/i3561.scala
@@ -13,10 +13,4 @@ class Test {
     case '5' | Constant => 3
     case '4' => 4
   }
-
-  def test3(x: Any) = (x: @annotation.switch) match { // error: could not emit switch - too few cases
-    case 1 => 1
-    case 2 => 2
-    case x: String => 4
-  }
 }
diff --git a/tests/neg-custom-args/fatal-warnings/i9776.scala b/tests/neg-custom-args/fatal-warnings/i9776.scala
@@ -0,0 +1,59 @@
+import scala.annotation.switch
+
+sealed trait Fruit
+
+object Fruit {
+  case object Apple extends Fruit
+  case object Banana extends Fruit
+  case object Lemon extends Fruit
+  case object Lime extends Fruit
+  case object Orange extends Fruit
+
+  def isCitrus(fruit: Fruit): Boolean =
+    (fruit: @switch) match {  // error Could not emit switch for @switch annotated match
+      case Orange => true
+      case Lemon  => true
+      case Lime   => true
+      case _      => false
+    }
+}
+
+
+sealed trait TaggedFruit {
+  def tag: Int
+}
+
+object TaggedFruit {
+  case object Apple extends TaggedFruit {
+    val tag = 1
+  }
+  case object Banana extends TaggedFruit {
+    val tag = 2
+  }
+  case object Orange extends TaggedFruit {
+    val tag = 3
+  }
+
+  def isCitrus(fruit: TaggedFruit): Boolean =
+    (fruit.tag: @switch) match {  // error Could not emit switch for @switch annotated match
+      case Apple.tag => true
+      case 2         => true
+      case 3         => true
+      case _         => false
+  }
+
+  // fewer than four cases, so no warning
+  def succ1(fruit: TaggedFruit): Boolean =
+    (fruit.tag: @switch) match {
+      case 3             => false
+      case 2 | Apple.tag => true
+    }
+
+  // fewer than four cases, so no warning
+  def succ2(fruit: TaggedFruit): Boolean =
+    (fruit.tag: @switch) match {
+      case 3          => false
+      case 2          => true
+      case Apple.tag  => true
+  }
+}
diff --git a/tests/neg-custom-args/fatal-warnings/switches.scala b/tests/neg-custom-args/fatal-warnings/switches.scala
@@ -38,13 +38,15 @@ object Main {
   // thinks a val in an object is constant... so naive
   def fail1(c: Char) = (c: @switch @unchecked) match { // error: Could not emit switch for @switch annotated match
     case 'A'        => true
+    case 'B'        => true
     case Other.C1   => true
     case _          => false
   }
 
   // more naivete
   def fail2(c: Char) = (c: @unchecked @switch) match { // error: Could not emit switch for @switch annotated match
     case 'A'        => true
+    case 'B'        => true
     case Other.C3   => true
     case _          => false
   }

diff --git a/tests/untried/neg/t5830.scala → ...eg-custom-args/fatal-warnings/t5830.scala b/tests/untried/neg/t5830.scala → ...eg-custom-args/fatal-warnings/t5830.scala
@@ -3,7 +3,7 @@ import scala.annotation.switch
 class Test {
   def unreachable(ch: Char) = (ch: @switch) match {
     case 'a' => println("b") // ok
-    case 'a' => println("b") // unreachable
+    case 'a' => println("b") // error: unreachable case
     case 'c' =>
   }
 }
diff --git a/tests/pos-special/fatal-warnings/i9776.scala b/tests/pos-special/fatal-warnings/i9776.scala
@@ -0,0 +1,38 @@
+import scala.annotation.switch
+
+sealed trait Fruit
+
+object Fruit {
+  case object Apple extends Fruit
+  case object Banana extends Fruit
+  case object Orange extends Fruit
+
+  def isCitrus(fruit: Fruit): Boolean =
+    (fruit: @switch) match {
+      case Orange => true
+      case _      => false
+    }
+}
+
+
+sealed trait TaggedFruit {
+  def tag: Int
+}
+
+object TaggedFruit {
+  case object Apple extends TaggedFruit {
+    val tag = 1
+  }
+  case object Banana extends TaggedFruit {
+    val tag = 2
+  }
+  case object Orange extends TaggedFruit {
+    val tag = 3
+  }
+
+  def isCitrus(fruit: TaggedFruit): Boolean =
+    (fruit.tag: @switch) match {
+      case 3 => true
+      case _ => false
+  }
+}
diff --git a/tests/pos/t6595.scala → tests/pos-special/fatal-warnings/t6595.scala b/tests/pos/t6595.scala → tests/pos-special/fatal-warnings/t6595.scala
diff --git a/tests/untried/neg/switch.check b/tests/untried/neg/switch.check
diff --git a/tests/untried/neg/switch.flags b/tests/untried/neg/switch.flags
diff --git a/tests/untried/neg/t5830.check b/tests/untried/neg/t5830.check
diff --git a/tests/untried/neg/t5830.flags b/tests/untried/neg/t5830.flags