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

griggt · 2020-09-23T05:42:57Z

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 PR, see #5070 for an example.

Fixes #9776.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

smarter · 2020-09-25T12:56:58Z

Any reason this is still a draft?

griggt · 2020-09-25T14:46:39Z

Almost done, found some issues related to value classes.

griggt · 2020-09-25T17:14:47Z

So, it turns out that the naive fix for this broke @switch warnings on value classes.

Wait, what? Switches on value classes? Yes, it turns out that dotc can emit switches when matching on value classes. I don't know if this is an advertised feature (I got the impression from #3926 (comment) that it may not be well known.)

Before this PR, simple @switch annotated matches on value classes would elicit a warning:

case class IntAnyVal(x: Int) extends AnyVal
def testS(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
}

[warn] -- [E115] Syntax Warning: /src/dotty-issues/i9776/src/main/scala/testS.scala:31:41
[warn] 31 |  def testS(x: IntAnyVal) = (x: @switch) match {
[warn]    |                            ^
[warn]    |Could not emit switch for @switch annotated match since there are not enough cases
[warn] 32 |    case IntAnyVal(1) => 0
[warn] 33 |    case IntAnyVal(10) => 1
[warn] 34 |    case IntAnyVal(100) => 2
[warn] 35 |    case IntAnyVal(1000) => 3
[warn] 36 |    case IntAnyVal(10000) => 4
[warn] 37 |  }

This warning is spurious, since a lookupswitch is actually emitted. However, it will also issue a warning in cases where a switch is not generated, such as:

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

[warn] -- [E115] Syntax Warning: /src/dotty-issues/i9776/src/main/scala/testS.scala:61:43
[warn] 61 |  def testBad(x: IntAnyVal) = (x: @switch) match {
[warn]    |                              ^
[warn]    |Could not emit switch for @switch annotated match since there are not enough cases
[warn] 62 |    case IntAnyVal(1) => 0
[warn] 63 |    case Ten           => 1
[warn] 64 |    case IntAnyVal(100) => 2
[warn] 65 |    case IntAnyVal(1000) => 3
[warn] 66 |    case IntAnyVal(10000) => 4
[warn] 67 |  }

In this instance, the warning is valid, although it is issued for the wrong reason. There are sufficient cases, but the use of a val rather than an extractor breaks the switch generation.

The "too few cases" message is issued because the shape of the result tree when matching on value classes is different than, say, matching on an Int. This prevents any of the result cases of the generated switch from being picked up by the warning logic, and so every (I think) @switch annotated match on a value class issued a warning, valid or not. Additionally, the types of the original cases are not singleton types as with matching on an Int, which complicates the warning threshold logic.

When the original version of this PR simply disabled the issuance of @switch warnings with too few cases, it suppressed all the valid warnings when matching on value classes.

I have added an additional commit to address this. I am not particularly pleased with the implementation, it feels like a bit of a brittle hack to me. Suggestions are welcomed. I have kept it is a separate commit for independent review and so it can be easily reverted if desired.

griggt · 2020-09-27T21:24:48Z

Updated to reflect the recently merged changes to widen and typeSymbol.

sjrd

Thanks for the PR, and sorry for the very long delay before someone looked at it.

I have two comments. Also, could you rebase on top of the latest master, even if there is no conflict, to make sure the CI runs against a recent version of the codebase? Thank you.

compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala

sjrd · 2021-01-06T11:22:05Z

compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala

+        val caseThreshold =
+          if ValueClasses.isDerivedValueClass(tpt.tpe.typeSymbol) then 1
+          else MinSwitchCases


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

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.

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.

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.

griggt · 2021-01-11T02:21:16Z

Rebased on current master and incorporated review suggestion.

smarter · 2021-03-12T15:27:42Z

Ping @sjrd, is this good to merge?

dottybot reviewed Sep 23, 2020

View reviewed changes

griggt force-pushed the fix-#9776 branch from a7954d0 to 30b3cbc Compare September 23, 2020 22:29

smarter approved these changes Sep 25, 2020

View reviewed changes

griggt force-pushed the fix-#9776 branch 2 times, most recently from 365f5ad to b59eced Compare September 25, 2020 17:11

griggt marked this pull request as ready for review September 25, 2020 17:17

griggt force-pushed the fix-#9776 branch from b59eced to 66583b8 Compare September 27, 2020 21:22

smarter assigned sjrd Jan 5, 2021

sjrd reviewed Jan 6, 2021

View reviewed changes

griggt added 2 commits January 10, 2021 17:21

griggt force-pushed the fix-#9776 branch from 66583b8 to f8acad1 Compare January 11, 2021 02:19

smarter mentioned this pull request Mar 12, 2021

Improve UnableToEmitSwitch warning message #11680

Closed

sjrd approved these changes Mar 12, 2021

View reviewed changes

smarter merged commit 75b08ac into scala:master Mar 12, 2021

griggt deleted the fix-#9776 branch March 12, 2021 17:13

Kordyjan added this to the 3.0.0 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

griggt commented Sep 23, 2020 •

edited

Loading

Uh oh!

dottybot left a comment

Uh oh!

smarter commented Sep 25, 2020

Uh oh!

griggt commented Sep 25, 2020

Uh oh!

griggt commented Sep 25, 2020

Uh oh!

griggt commented Sep 27, 2020

Uh oh!

sjrd left a comment

Uh oh!

Uh oh!

sjrd Jan 6, 2021

Uh oh!

griggt Jan 11, 2021

Uh oh!

griggt commented Jan 11, 2021

Uh oh!

smarter commented Mar 12, 2021

Uh oh!

Uh oh!

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

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

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

Uh oh!

Conversation

griggt commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

smarter commented Sep 25, 2020

Uh oh!

griggt commented Sep 25, 2020

Uh oh!

griggt commented Sep 25, 2020

Uh oh!

griggt commented Sep 27, 2020

Uh oh!

sjrd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjrd Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

griggt Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

griggt commented Jan 11, 2021

Uh oh!

smarter commented Mar 12, 2021

Uh oh!

Uh oh!

griggt commented Sep 23, 2020 •

edited

Loading