Skip to content

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

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 2 commits into from
Mar 12, 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
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 @@ -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'
Expand Down
20 changes: 11 additions & 9 deletions compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala
Original file line number Diff line number Diff line change
Expand Up @@ -976,30 +976,32 @@ 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 Block((_: ValDef) :: Block(_, Match(_, cases)) :: Nil, _) => cases
case _ => Nil
}
def typesInPattern(pat: Tree): List[Type] = pat match {
val caseThreshold =
if ValueClasses.isDerivedValueClass(tpt.tpe.typeSymbol) then 1
else MinSwitchCases
Comment on lines +986 to +988
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have this branch? Shouldn't it always be 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general idea is to limit the warnings to when numTypes(original.cases) >= MinSwitchCases

https://github.com/lampepfl/dotty/blob/609d8d6bdbe9be33080967cdac478a6957237356/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala#L59-L60

However value classes are special cased as an attempt at "best-effort" support. Consider:

case class IntAnyVal(x: Int) extends AnyVal

final val Ten = IntAnyVal(10)

def test(x: IntAnyVal) = (x: @switch) match {
  case IntAnyVal(1) => 0
  case Ten           => 1
  case IntAnyVal(100) => 2
  case IntAnyVal(1000) => 3
  case IntAnyVal(10000) => 4
}

which results in

Item Value Notes
numTypes(original.cases) 2 { IntAnyVal, Ten.type }
numTypes(resultCases) 0
emitted bytecode if-then

We want to warn here since the switch was not emitted despite a sufficient number of cases, thus we use a lower caseThreshold for value classes.

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 >= caseThreshold && 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,
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
}
}
23 changes: 23 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 Expand Up @@ -74,4 +76,25 @@ object Main {
case 1 | 2 | 3 => true
case _ => false
}

case class IntAnyVal(x: Int) extends AnyVal

val Ten = IntAnyVal(10)
def fail5(x: IntAnyVal) = (x: @switch) match { // error: Could not emit switch for @switch annotated match
case IntAnyVal(1) => 0
case Ten => 1
case IntAnyVal(100) => 2
case IntAnyVal(1000) => 3
case IntAnyVal(10000) => 4
}

// the generated lookupswitch covers only a subset of the cases
final val One = IntAnyVal(1)
def fail6(x: IntAnyVal) = (x: @switch) match { // error: Could not emit switch for @switch annotated match
case One => 0
case IntAnyVal(10) => 1
case IntAnyVal(100) => 2
case IntAnyVal(1000) => 3
case IntAnyVal(10000) => 4
}
}
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
}
}
10 changes: 10 additions & 0 deletions tests/pos-special/fatal-warnings/switches.scala
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,18 @@ class Test {
case 1 | 2 | 3 => true
case _ => false
}

def test6(x: IntAnyVal) = (x: @switch) match {
case IntAnyVal(1) => 0
case IntAnyVal(10) => 1
case IntAnyVal(100) => 2
case IntAnyVal(1000) => 3
case IntAnyVal(10000) => 4
}
}

case class IntAnyVal(x: Int) extends AnyVal

object Test {
final val LF = '\u000A'
final val CR = '\u000D'
Expand Down
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.