From e298e21964ba1397f83febc0902675bb468427c7 Mon Sep 17 00:00:00 2001 From: Tom Grigg Date: Tue, 22 Sep 2020 16:57:12 -0700 Subject: [PATCH 1/2] Fix #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 #5070 for an example. --- .../dotty/tools/dotc/reporting/messages.scala | 5 +- .../tools/dotc/transform/PatternMatcher.scala | 16 +++-- .../fatal-warnings/i3561.scala | 6 -- .../fatal-warnings/i9776.scala | 59 +++++++++++++++++++ .../fatal-warnings/switches.scala | 2 + .../fatal-warnings}/t5830.scala | 2 +- tests/pos-special/fatal-warnings/i9776.scala | 38 ++++++++++++ .../fatal-warnings}/t6595.scala | 0 tests/untried/neg/switch.check | 9 --- tests/untried/neg/switch.flags | 1 - tests/untried/neg/t5830.check | 9 --- tests/untried/neg/t5830.flags | 1 - 12 files changed, 109 insertions(+), 39 deletions(-) create mode 100644 tests/neg-custom-args/fatal-warnings/i9776.scala rename tests/{untried/neg => neg-custom-args/fatal-warnings}/t5830.scala (73%) create mode 100644 tests/pos-special/fatal-warnings/i9776.scala rename tests/{pos => pos-special/fatal-warnings}/t6595.scala (100%) delete mode 100644 tests/untried/neg/switch.check delete mode 100644 tests/untried/neg/switch.flags delete mode 100644 tests/untried/neg/t5830.check delete mode 100644 tests/untried/neg/t5830.flags diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala index 2431fc969675..aa61c04ade96 100644 --- 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 index 169f68e2cad9..7db82c0646bc 100644 --- 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 index ead859e5d277..f6e754e9ec8c 100644 --- 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 new file mode 100644 index 000000000000..0de811e2adb3 --- /dev/null +++ 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 index e23f7f27cb82..71d29ebb18d0 100644 --- a/tests/neg-custom-args/fatal-warnings/switches.scala +++ b/tests/neg-custom-args/fatal-warnings/switches.scala @@ -38,6 +38,7 @@ 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 } @@ -45,6 +46,7 @@ object Main { // 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 b/tests/neg-custom-args/fatal-warnings/t5830.scala similarity index 73% rename from tests/untried/neg/t5830.scala rename to tests/neg-custom-args/fatal-warnings/t5830.scala index 18e89c20f184..629b345d3737 100644 --- a/tests/untried/neg/t5830.scala +++ b/tests/neg-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 new file mode 100644 index 000000000000..e1c65dff268f --- /dev/null +++ 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 b/tests/pos-special/fatal-warnings/t6595.scala similarity index 100% rename from tests/pos/t6595.scala rename to tests/pos-special/fatal-warnings/t6595.scala diff --git a/tests/untried/neg/switch.check b/tests/untried/neg/switch.check deleted file mode 100644 index 16c5ca1ba867..000000000000 --- a/tests/untried/neg/switch.check +++ /dev/null @@ -1,9 +0,0 @@ -switch.scala:38: warning: could not emit switch for @switch annotated match - def fail2(c: Char) = (c: @switch @unchecked) match { - ^ -switch.scala:45: warning: could not emit switch for @switch annotated match - def fail3(c: Char) = (c: @unchecked @switch) match { - ^ -error: No warnings can be incurred under -Xfatal-warnings. -2 warnings found -1 error found diff --git a/tests/untried/neg/switch.flags b/tests/untried/neg/switch.flags deleted file mode 100644 index e8fb65d50c20..000000000000 --- a/tests/untried/neg/switch.flags +++ /dev/null @@ -1 +0,0 @@ --Xfatal-warnings \ No newline at end of file diff --git a/tests/untried/neg/t5830.check b/tests/untried/neg/t5830.check deleted file mode 100644 index 58c3a1be38f5..000000000000 --- a/tests/untried/neg/t5830.check +++ /dev/null @@ -1,9 +0,0 @@ -t5830.scala:6: warning: unreachable code - case 'a' => println("b") // unreachable - ^ -t5830.scala:4: warning: could not emit switch for @switch annotated match - def unreachable(ch: Char) = (ch: @switch) match { - ^ -error: No warnings can be incurred under -Xfatal-warnings. -two warnings found -one error found diff --git a/tests/untried/neg/t5830.flags b/tests/untried/neg/t5830.flags deleted file mode 100644 index e8fb65d50c20..000000000000 --- a/tests/untried/neg/t5830.flags +++ /dev/null @@ -1 +0,0 @@ --Xfatal-warnings \ No newline at end of file From f8acad1b977b61e258812e6b73a4da1e7b8fd667 Mon Sep 17 00:00:00 2001 From: Tom Grigg Date: Fri, 25 Sep 2020 08:07:21 -0700 Subject: [PATCH 2/2] Fix @switch warnings for matches on value classes Matches on value classes that have an underlying switchable type may be emitted as tableswitch or lookupswitch, but the shape of the result tree differs from ordinary switch-compiled matches. Previously, this caused a spurious warning to be issued if such a match was annotated with @switch, as none of the result cases were discovered by the warning logic, and hence the match was warned as having too few cases. With the warning for too few cases now removed, we have the opposite issue: in no circumstance is a switch warning issued for @switch annotated matches on value classes. This commit attempts to address this issue and restore the @switch warnings for those matches on value classes where a tableswitch or lookupswitch is not emitted. A complicating factor is that the original case types for matches on value class extractors are not singleton types, and so counting the number of unique types is not useful for determining the number of original cases. --- .../tools/dotc/transform/PatternMatcher.scala | 6 +++++- .../fatal-warnings/switches.scala | 21 +++++++++++++++++++ .../pos-special/fatal-warnings/switches.scala | 10 +++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala index 7db82c0646bc..6af047d74622 100644 --- a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala +++ b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala @@ -981,7 +981,11 @@ object PatternMatcher { val resultCases = result match case Match(_, cases) => cases case Block(_, Match(_, cases)) => cases + case Block((_: ValDef) :: Block(_, Match(_, cases)) :: Nil, _) => cases case _ => Nil + val caseThreshold = + if ValueClasses.isDerivedValueClass(tpt.tpe.typeSymbol) then 1 + else MinSwitchCases def typesInPattern(pat: Tree): List[Type] = pat match case Alternative(pats) => pats.flatMap(typesInPattern) case _ => pat.tpe :: Nil @@ -990,7 +994,7 @@ object PatternMatcher { def numTypes(cdefs: List[CaseDef]): Int = typesInCases(cdefs).toSet.size: Int // without the type ascription, testPickling fails because of #2840. val numTypesInOriginal = numTypes(original.cases) - if numTypesInOriginal >= MinSwitchCases && numTypes(resultCases) < numTypesInOriginal then + 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)}%, %") diff --git a/tests/neg-custom-args/fatal-warnings/switches.scala b/tests/neg-custom-args/fatal-warnings/switches.scala index 71d29ebb18d0..a327ab88758a 100644 --- a/tests/neg-custom-args/fatal-warnings/switches.scala +++ b/tests/neg-custom-args/fatal-warnings/switches.scala @@ -76,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 + } } diff --git a/tests/pos-special/fatal-warnings/switches.scala b/tests/pos-special/fatal-warnings/switches.scala index 723727df493c..371ee312722c 100644 --- a/tests/pos-special/fatal-warnings/switches.scala +++ b/tests/pos-special/fatal-warnings/switches.scala @@ -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'