SI-8731 warning if @switch is ignored

lrytz · lrytz · commit 0c2597924487 · 2014-10-06T20:56:32.000+02:00
For matches with two or fewer cases, @switch is ignored. This should not happen silently.
diff --git a/src/compiler/scala/tools/nsc/transform/patmat/MatchTreeMaking.scala b/src/compiler/scala/tools/nsc/transform/patmat/MatchTreeMaking.scala
@@ -550,8 +550,24 @@ trait MatchTreeMaking extends MatchCodeGen with Debugging {
                 case _ => false
               }
               val suppression = Suppression(suppressExhaustive, supressUnreachable)
+              val hasSwitchAnnotation = treeInfo.isSwitchAnnotation(tpt.tpe)
               // matches with two or fewer cases need not apply for switchiness (if-then-else will do)
-              val requireSwitch = treeInfo.isSwitchAnnotation(tpt.tpe) && casesNoSubstOnly.lengthCompare(2) > 0
+              // `case 1 | 2` is considered as two cases.
+              def exceedsTwoCasesOrAlts = {
+                // avoids traversing the entire list if there are more than 3 elements
+                def lengthMax3[T](l: List[T]): Int = l match {
+                  case a :: b :: c :: _ => 3
+                  case cases =>
+                    cases.map({
+                      case AlternativesTreeMaker(_, alts, _) :: _ => lengthMax3(alts)
+                      case c => 1
+                    }).sum
+                }
+                lengthMax3(casesNoSubstOnly) > 2
+              }
+              val requireSwitch = hasSwitchAnnotation && exceedsTwoCasesOrAlts
+              if (hasSwitchAnnotation && !requireSwitch)
+                reporter.warning(scrut.pos, "matches with two cases or fewer are emitted using if-then-else instead of switch")
               (suppression, requireSwitch)
             case _ =>
               (Suppression.NoSuppression, false)
diff --git a/test/files/neg/t6902.scala b/test/files/neg/t6902.scala
@@ -16,7 +16,7 @@ object Test {
   // at scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:50)
   // at scala.tools.nsc.Global.abort(Global.scala:249)
   // at scala.tools.nsc.backend.jvm.GenASM$JPlainBuilder$jcode$.emitSWITCH(GenASM.scala:1850)
-  ((1: Byte): @unchecked @annotation.switch) match {
+  ((1: Byte): @unchecked) match {
     case 1 => 2
     case 1 => 3 // crash
   }
diff --git a/test/files/neg/t8731.check b/test/files/neg/t8731.check
@@ -0,0 +1,9 @@
+t8731.scala:5: warning: matches with two cases or fewer are emitted using if-then-else instead of switch
+  def f(x: Int) = (x: @annotation.switch) match {
+                       ^
+t8731.scala:10: warning: could not emit switch for @switch annotated match
+  def g(x: Int) = (x: @annotation.switch) match {
+                       ^
+error: No warnings can be incurred under -Xfatal-warnings.
+two warnings found
+one error found
diff --git a/test/files/neg/t8731.flags b/test/files/neg/t8731.flags
@@ -0,0 +1 @@
+-Xfatal-warnings
diff --git a/test/files/neg/t8731.scala b/test/files/neg/t8731.scala
@@ -0,0 +1,15 @@
+class C {
+  // not a compile-time constant due to return type
+  final val K: Int = 20
+
+  def f(x: Int) = (x: @annotation.switch) match {
+    case K => 0
+    case 2 => 1
+  }
+
+  def g(x: Int) = (x: @annotation.switch) match {
+    case K => 0
+    case 2 => 1
+    case 3 => 2
+  }
+}
diff --git a/test/files/pos/switch-small.flags b/test/files/pos/switch-small.flags
diff --git a/test/files/pos/switch-small.scala b/test/files/pos/switch-small.scala
diff --git a/test/files/run/t5830.check b/test/files/run/t5830.check
@@ -1,5 +1,6 @@
 a with oef
 a with oef
+a with oef
 a
 def with oef
 def
diff --git a/test/files/run/t5830.scala b/test/files/run/t5830.scala
@@ -1,12 +1,11 @@
 import scala.annotation.switch
 
 object Test extends App {
-  // TODO: should not emit a switch
-  // def noSwitch(ch: Char, eof: Boolean) = (ch: @switch) match {
-  //   case 'a' if eof => println("a with oef") // then branch
-  // }
+  def noSwitch(ch: Char, eof: Boolean) = ch match {
+    case 'a' if eof => println("a with oef") // then branch
+  }
 
-  def onlyThen(ch: Char, eof: Boolean) = (ch: @switch) match {
+  def onlyThen(ch: Char, eof: Boolean) = ch match {
     case 'a' if eof => println("a with oef") // then branch
     case 'c' =>
   }
@@ -18,7 +17,7 @@ object Test extends App {
     case 'c' =>
   }
 
-  def defaultUnguarded(ch: Char, eof: Boolean) = (ch: @switch) match {
+  def defaultUnguarded(ch: Char, eof: Boolean) = ch match {
     case ' ' if eof => println("spacey oef")
     case _ => println("default")
   }
@@ -44,7 +43,7 @@ object Test extends App {
   //   case 'c' =>
   // }
 
-  // noSwitch('a', true)
+  noSwitch('a', true)
   onlyThen('a', true)       // 'a with oef'
   ifThenElse('a', true)     // 'a with oef'
   ifThenElse('a', false)    // 'a'
diff --git a/test/files/run/t6011c.scala b/test/files/run/t6011c.scala
@@ -6,7 +6,7 @@ object Test extends App {
   // at scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:50)
   // at scala.tools.nsc.Global.abort(Global.scala:249)
   // at scala.tools.nsc.backend.jvm.GenASM$JPlainBuilder$jcode$.emitSWITCH(GenASM.scala:1850)
-  ((1: Byte): @unchecked @annotation.switch) match {
+  ((1: Byte): @unchecked) match {
     case 1 => 2
     case 1 => 3 // crash
   }
diff --git a/test/junit/scala/issues/BytecodeTests.scala b/test/junit/scala/issues/BytecodeTests.scala
@@ -0,0 +1,39 @@
+package scala.issues
+
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+import org.junit.Test
+import scala.tools.asm.Opcodes
+import scala.tools.nsc.backend.jvm.CodeGenTools._
+import org.junit.Assert._
+import scala.collection.JavaConverters._
+import scala.tools.partest.ASMConverters._
+
+@RunWith(classOf[JUnit4])
+class BytecodeTests {
+  val compiler = newCompiler()
+
+  @Test
+  def t8731(): Unit = {
+    val code =
+      """class C {
+        |  def f(x: Int) = (x: @annotation.switch) match {
+        |    case 1 => 0
+        |    case 2 => 1
+        |    case 3 => 2
+        |  }
+        |  final val K = 10
+        |  def g(x: Int) = (x: @annotation.switch) match {
+        |    case K => 0
+        |    case 1 => 10
+        |    case 2 => 20
+        |  }
+        |}
+      """.stripMargin
+
+    val List(c) = compileClasses(compiler)(code)
+
+    assertTrue(getSingleMethod(c, "f").instructions.count(_.isInstanceOf[TableSwitch]) == 1)
+    assertTrue(getSingleMethod(c, "g").instructions.count(_.isInstanceOf[LookupSwitch]) == 1)
+  }
+}
diff --git a/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala
@@ -76,4 +76,7 @@ object CodeGenTools {
   def assertSameCode(actual: List[Instruction], expected: List[Instruction]): Unit = {
     assertTrue(s"\nExpected: $expected\nActual  : $actual", actual === expected)
   }
+
+  def getSingleMethod(classNode: ClassNode, name: String): Method =
+    convertMethod(classNode.methods.asScala.toList.find(_.name == name).get)
 }

Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@ object Test {`
`16`	`16`	`// at scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:50)`
`17`	`17`	`// at scala.tools.nsc.Global.abort(Global.scala:249)`
`18`	`18`	`// at scala.tools.nsc.backend.jvm.GenASM$JPlainBuilder$jcode$.emitSWITCH(GenASM.scala:1850)`
`19`		`- ((1: Byte): @unchecked @annotation.switch) match {`
	`19`	`+ ((1: Byte): @unchecked) match {`
`20`	`20`	`case 1 => 2`
`21`	`21`	`case 1 => 3 // crash`
`22`	`22`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,6 @@`
`1`	`1`	`a with oef`
`2`	`2`	`a with oef`
	`3`	`+a with oef`
`3`	`4`	`a`
`4`	`5`	`def with oef`
`5`	`6`	`def`
Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@ object Test extends App {`
`6`	`6`	`// at scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:50)`
`7`	`7`	`// at scala.tools.nsc.Global.abort(Global.scala:249)`
`8`	`8`	`// at scala.tools.nsc.backend.jvm.GenASM$JPlainBuilder$jcode$.emitSWITCH(GenASM.scala:1850)`
`9`		`- ((1: Byte): @unchecked @annotation.switch) match {`
	`9`	`+ ((1: Byte): @unchecked) match {`
`10`	`10`	`case 1 => 2`
`11`	`11`	`case 1 => 3 // crash`
`12`	`12`	`}`