Skip to content

Commit

Permalink
Fix scala#9776: Don't issue a @switch warning when fewer than 4 cases
Browse files Browse the repository at this point in the history
In the spirit of scala/scala#4148, 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.
  • Loading branch information
griggt committed Sep 25, 2020
1 parent f9e2428 commit 64ec380
Show file tree
Hide file tree
Showing 12 changed files with 108 additions and 39 deletions.
5 changes: 2 additions & 3 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1969,10 +1969,9 @@ import ast.tpd
|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'
Expand Down
15 changes: 6 additions & 9 deletions compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala
Original file line number Diff line number Diff line change
Expand Up @@ -968,30 +968,27 @@ 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)) {
if numTypes(original.cases) >= MinSwitchCases && numTypes(resultCases) < numTypes(original.cases) 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,
Expand Down
6 changes: 0 additions & 6 deletions tests/neg-custom-args/fatal-warnings/i3561.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
59 changes: 59 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i9776.scala
Original file line number Diff line number Diff line change
@@ -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
}
}
2 changes: 2 additions & 0 deletions tests/neg-custom-args/fatal-warnings/switches.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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' =>
}
}
38 changes: 38 additions & 0 deletions tests/pos-special/fatal-warnings/i9776.scala
Original file line number Diff line number Diff line change
@@ -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
}
}
File renamed without changes.
9 changes: 0 additions & 9 deletions tests/untried/neg/switch.check

This file was deleted.

1 change: 0 additions & 1 deletion tests/untried/neg/switch.flags

This file was deleted.

9 changes: 0 additions & 9 deletions tests/untried/neg/t5830.check

This file was deleted.

1 change: 0 additions & 1 deletion tests/untried/neg/t5830.flags

This file was deleted.

0 comments on commit 64ec380

Please sign in to comment.