scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

Exhaustive match checking doesn't work on strings #12770

Closed coreywoodfield closed 1 year ago

coreywoodfield commented 1 year ago

Reproduction steps

Scala version: 2.13.10

compile the following with scalac -Xlint Test.scala

object Test {
  def doesntWarn(input: String) = input match {
    case "a" =>
    case "b" =>
  }
  def warns(input: String) = input match {
    case "b" =>
  }
}

The output is

Test.scala:8: warning: match may not be exhaustive.
It would fail on the following input: (x: String forSome x not in "b")
  def warns(input: String) = input match {
                             ^
1 warning

Problem

There are two inexhaustive matches, there should be two warnings

Both match statements are clearly inexhaustive, but match statements on Strings (or Ints or Chars and probably other things too) with two or more literal cases don't trigger a match not may be exhaustive warning.

Adding a third case "c" if false => to doesntWarn results in an inexhaustive match warning and an unreachable code warning.

Adding just case "c" => or case other if other.isEmpty => (any condition will do as long as it's not a boolean literal, afaict) has no effect.

som-snytt commented 1 year ago

It doesn't warn for switches. Since the single-case match doesn't compile to a switch, it was subject to the exhaustiveness warning. The fix is to quash the warning. The reason not to warn is that the types eligible for switches (ints and strings) will never be exhausted. Arguably, the warning should be quashed only if the match is marked @switch, but also arguably it is always obvious.

It may not be obvious that this may fail if state is expanded to include 4:

def next(state: Int) = state match { case 1 => 0 case 2 | 3 => 4 }
coreywoodfield commented 1 year ago

It seems to me that "the fix" would be to warn for all switches that don't have a default case. I would love to know at compile time that all my match statements (whether they compile to switches or not) are safe. It seems silly to warn on some unsafe matches and not warn on others based on what the bytecode ends up looking like. From the programmer's perspective, I'd argue that there's little difference between

sealed trait Trait
object A extends Trait
object B extends Trait

(???: Trait) match {
  case A =>
}

and

(???: String) match {
  case "A" =>
}

Yes, one has two possible cases and the other has infinite possible cases, but for both, the compiler can tell that the provided cases are not exhaustive

som-snytt commented 1 year ago

This would make a good Scalafix lint.

Some prefer to always have a default case, as a matter of policy, even for closed domains like enums, because maybe they'll add an enum and we didn't recompile.

The analogy for a string scrutinee is an unsealed class, which does not warn, even if the compiler knows there are other child classes not covered by the match.

BTW, worth adding that when I wrote "the fix" I only intended "the fix in the PR", and not to say it's the only reasonable fix.

The bit about switch was just mechanism, not metaphor.

Feel free to lobby on the forum for a noisier warning.

SethTisue commented 11 months ago

related: #12247