Skip to content

Pattern Match with Default Case Cannot Be Optimized To Switch Statement #9776

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

Closed
adamgfraser opened this issue Sep 12, 2020 · 9 comments · Fixed by #9852
Closed

Pattern Match with Default Case Cannot Be Optimized To Switch Statement #9776

adamgfraser opened this issue Sep 12, 2020 · 9 comments · Fixed by #9852

Comments

@adamgfraser
Copy link
Contributor

Minimized code

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
    }
}

Output

// Could not emit switch for @switch annotated match since there are not enough cases

Expectation

I would have expected this to work as it appears to on Scala 2. I can understand that there are only two cases in the match statement versus three types of fruit but I would have hoped the compiler would be smart enough to know that using a wildcard is equivalent to matching against each of the remaining cases. If I enumerate them individually as in case Apple | Banana => ??? this works, but it doesn't seem like I should have to do that.

@smarter
Copy link
Member

smarter commented Sep 12, 2020

it appears to on Scala 2

Neither Scala 2 nor Dotty emit a tableswitch or lookupswitch for this code (which is what @switch checks for), they both just return Orange.equals(fruit) which is optimal.

If I enumerate them individually as in case Apple | Banana => ??? this works

That would be wrong, since this again won't emit a switch (because there's no reason to), but it doesn't appear to be true. I think you may have minimized that code sample too much: does the original actually match on objects or are you matching on integers?

@adamgfraser
Copy link
Contributor Author

@smarter I think I may indeed have minimized too much. Here is another version:

import scala.annotation.switch

sealed trait Fruit {
  def tag: Int
}

object Fruit {

  case object Apple extends Fruit {
    val tag = 1
  }
  case object Banana extends Fruit {
    val tag = 2
  }
  case object Orange extends Fruit {
    val tag = 3
  }
  
  def isCitrus(fruit: Fruit): Boolean =
  (fruit.tag: @switch) match {
    case 3 => true
    case _      => false
  }
}

object Example extends App {
  println("Hello, World!")
}

The original code that generated the issue is here. Where we got an error like the above when we tried to compile this on Dotty.

@smarter
Copy link
Member

smarter commented Sep 12, 2020

This is intentional, we only emit a tableswitch instruction if there are at least four cases, otherwise we just use if/else, I assume this is for performance but I don't know how the exact value of 4 was arrived at, I would haany idea @odersky, @sjrd? I traced it to this commit: 2e7294c

@adamgfraser
Copy link
Contributor Author

Hmmm, interesting. In the original example there are a total of seven cases but we want to treat four of them the same way as part of the default branch.

Maybe there needs to be some change in the warning message to better communicate this? Adding case a | b | c | d => ??? silences the warning which gives the impression that is more efficient but it seems like that is not the case and maybe is even less efficient? Maybe if the warning said something about switch statements not being used if there are fewer than four branches? It seems like in our actual code the thing we should be doing is just not using the switch annotation there but that isn't exactly obvious from the warning message.

@smarter
Copy link
Member

smarter commented Sep 12, 2020

Adding case a | b | c | d => ??? silences the warning which gives the impression that is more efficient but it seems like that is not the case and maybe is even less efficient?

It's probably less efficient yes.

Maybe if the warning said something about switch statements not being used if there are fewer than four branches?

I'm thinking maybe it just shouldn't say anything in this case. People don't really care about the exact instructions used on the JVM to represent the pattern match even if that's what the documentation of @switch mentions, they only care that it's efficient and they hope that putting an @switch asserts that.

@adamgfraser
Copy link
Contributor Author

That makes sense to me.

@sjrd
Copy link
Member

sjrd commented Sep 12, 2020

I'm thinking maybe it just shouldn't say anything in this case. People don't really care about the exact instructions used on the JVM to represent the pattern match even if that's what the documentation of @switch mentions, they only care that it's efficient and they hope that putting an @switch asserts that.

Yes, that is what the latest scalac does, IIRC. It used to emit a warning but I believe it's been changed over that specific concern.

sideeffffect added a commit to sideeffffect/zio-prelude that referenced this issue Sep 12, 2020
adamgfraser pushed a commit to zio/zio-prelude that referenced this issue Sep 12, 2020
* Add Dotty to the CI

* Add Dotty to the CI

* fix yaml formatting

* fix yaml formatting

* Add sbt-dotty and add Silencer the dotty way

* Dotty 0.26

* Dotty 0.26.0

* kind-projector only in non-Dotty

* scalafix only in non-Dotty

* scalafix only in non-Dotty (sbt fix)

* semanticdbEnabled := !isDotty.value

* scalacOptions for Dotty

* rework scalacOptions

* Dotty platform specific

* BuildHelper from ZIO

* proper silencer version

* consolidate Scala version specific dirs

* Add unmanagedSourceDirectories

* compositionLaw: Equal.DeriveEqual

* compositionLaw: Equal.DeriveEqual, more

* Modify[-S1, +S2, +A](run: S1 => (S2, A)) extends ZPure[S1, S2, Any, Nothing, A]

* compositionLaw: Equal.DeriveEqual explicit

* ZPure: Modify.run0

* Dotty can't kind-projector style +* nor -*

* Enough cases for @switch

* case that: NonEmptySet[_] => ...

* explicit implicits in tests

* val dummy =

* Don't use @switch on a branch with a default case

As per scala/scala3#9776 (comment)

* Reduce Equal.DeriveEqual

* Simplify TraversableSpec

* NewtypeSpec scala2Only

* Add links to uncovered issues

* Use Scala 2.12 by default
@sjrd
Copy link
Member

sjrd commented Sep 14, 2020

This is the scalac PR from 2015 that stopped emitting a warning in the same situation: scala/scala#4418

@odersky
Copy link
Contributor

odersky commented Sep 14, 2020

The scalac PR makes sense and should be adopted. We still should make sure that the warning is emitted for large pattern matches that cannot be turned into switches.

griggt added a commit to griggt/dotty that referenced this issue Sep 23, 2020
griggt added a commit to griggt/dotty that referenced this issue Sep 23, 2020
griggt added a commit to griggt/dotty that referenced this issue Sep 25, 2020
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.
griggt added a commit to griggt/dotty that referenced this issue Sep 25, 2020
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.
griggt added a commit to griggt/dotty that referenced this issue Jan 11, 2021
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.
smarter added a commit that referenced this issue Mar 12, 2021
Fix #9776: Don't issue a @switch warning when fewer than 4 cases
michelou pushed a commit to michelou/scala3 that referenced this issue Mar 15, 2021
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.
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants