Missing exhaustivity warning on ints (and other switchable types)

[nrinaudo]

reproduction steps

Scala 2.13.4 is supposed to implement the following:

Guards (case .. if .. =>) no longer disable the exhaustivity checker. Rather, the checker checks for exhaustivity using the "pessimistic" assumption that the guard may be false.

(Source)

Yet, the following code compiles without warning:

def test(i: Int) = i match {
  case value if value < 0 => true
  case value if value > 10 => false
}

This has been tried with and without the -Xlint:strict-unsealed-patmat scalac option.

problem

The way I understand the quoted sentence is: the compiler will consider both branches of the pattern match as potentially being false at the same time, which will cause it to emit a non-exhaustivity warning.

This is not what's happening.

[SethTisue]

agree this is a bug.

a weird thing here is that if you add another clause, the checking comes back!

scala 2.13.4> def test(i: Int) = i match {
            |   case value if value < 0 => true
            |   case value if value > 10 => false
            |   case 5 => true
            | }
                                 ^
              warning: match may not be exhaustive.

[dwijnand]

So -Vpatmat is the flag to get some pattern matching output, and normally it's an overwhelming amount of noise (that took me a few weeks to grok and be comfortable with) but very unexpectedly this code shows close to none!

$ m Ints.scala
class Test {
  def test(i: Int) = i match {
    case value if value < 0 => true
    case value if value > 10 => false
  }
}
$ scalac -Vpatmat Ints.scala
translating {case (value @ _) if value.<(0) => true
case (value @ _) if value.>(10) => false}
combining cases: {G(value.<(0)) >> B(true,Boolean)
G(value.>(10)) >> B(false,Boolean)}
def: (case <synthetic> val x1: Int = i,List(value x1),List(method test, class Test, package <empty>, package <root>))

Why is that? Because it can be emitted by a switch, so it doesn't do any analysis, because analysis was for those non-switchable sealed types! I'll try to fix this for the next release. I'll also try to suss out what it is about case 5 that changes that, as I think that should be switchable too (a separate bug, I guess).

[dwijnand]

I have a branch where I got this working. But making switchable types emit warnings fixes this and then highlights all the times (e.g. in the compiler) where internal implementations are switching on ints because of how fast it is, for the (let's say) 6 ints that are tracked internally. I think I ran out of steam/patience to fix all the cases in the compiler before I gave myself a break and considered that this might be just bad idea...

[martijnhoekstra]

could something like an annotation @openswitch that combines the functionality of @switch and @unchecked be useful there. This got me thinking about the two kinds of unchecked matches:

• The match is exhaustive in the domain of the scrutinee, but scalac doesn't know it: The programmer knows more than the compiler about the generators. @unchecked fits this case perfectly: you tell scalac it's ok it can't check exhaustiveness, and the reader that scalac isn't going to help you check exhaustiveness. This is the "not known to be exhaustive" variety.

• The domain of the scrutinee is smaller than the domain of the static type of the scrutinee (example: HTTP Status code represented as Short). You accept as a programmer that not all values of the type of the scrutinee are represented in the match. This is the "know to be not exhaustive" variety -- which is the one that I initially expected to be less of a problem to warn for. In this case the programmer knows more about the domain of some value than the compiler. @unchecked sounds like the wrong name for communicating this, and feels wrong to use.

In the case of switch matches, you commonly want to use a type that allows for more values than your domain, because you're restricted by performance considerations. Maybe it makes sense to combine the annotation for not caring about non-exhaustiveness and the annotation for caring about the switch performance.

[dwijnand]

Can we reuse @switch? Two questions:

1. Are there legit cases where I @switch an int and I want to get exhaustivity checks?
2. Are there legit cases where I don't @switch an int and I don't want to get exhaustivity checks?

(I say int but I mean any switchable type: byte, short, int, char, String.)

I think the second one is easier: if you're not using a switch table and you've got an incomplete match, then @unchecked it. But is the first one ok? 🤔

[martijnhoekstra]

Figuring out what I mean exactly: I always want exhaustiveness checks on the domain of the match, but I can't always communicate the domain to the compiler. Even if I can't, I may still want the compiler to warn you that you don't have a default case. But maybe that's uncommon enough to either not support it, or to support it with a new opt-in annotation.

As for concrete answers to your questions:

1. Yes, you may to want a switch with a default case, especially (but not only) when you're switching on actual ints as numbers, not as labels representing something else. Inspired by a recent actual scenario, I can imagine:

def rep(p: Parser[A], max: Int, fact: Factory[A, B]): ParserB = (max @switch) match {
  case 0 => Parser.pure(fact.newBuilder().result)
  case 1 => p.map(a => fact.newBuilder().addOne(a).result()).?.map(_.getOrElse(fact.newBuilder().result))
  case n => Repeater(p, n -1, fact)
}

2. Yes. For example, I can easily imagine a match on a HTTP status code represented as an Int or short, matching stuff like

status match {
  case info if info >= 100 && info <  200 =>
  case 200 =>
  case ok if ok > 200 && ok < 300 =>
  case 400 =>
  case 401 =>
  case 403 => 
  case agentError if agentError >= 400 && agentError  < 500 =>
  case serverError if serverError >= 500 =>
}

For the second maybe (status @unchecked) match is good enough, and outside the scope of this issue.

For the first, maybe it's enough to suck it up and don't have exhaustiveness checking even though you would prefer it, maybe introduce, I don't know, (max @checkedswitch) match

[SethTisue]

related: https://github.com/scala/bug/issues/12770