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

[griggt]

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]

Any reason this is still a draft?

[griggt]

Almost done, found some issues related to value classes.

[griggt]

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]

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.

[sjrd]

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

[griggt]

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.

[griggt]

Rebased on current master and incorporated review suggestion.

[smarter]

Ping @sjrd, is this good to merge?