Pattern matching on unchecked generic types (with isInstanceOf guard) does not work correctly

[liosedhel]

Compiler version

scala 3.1.1

Minimized code

trait Fruit
case class Apple() extends Fruit
case class Orange() extends Fruit

case class Box[C](fruit: C) extends Fruit

val apple = Box(fruit = Apple())
val orange = Box(fruit = Orange())


val result = List(apple, orange).map {
  case appleBox: Box[Apple] @unchecked if appleBox.fruit.isInstanceOf[Apple] => //contains apple
    "apple"
  case _ => 
    "orange"
}

assert(result == List("apple", "orange"))

Output

result == List("apple", "apple")

Expectation

Only Box with Apple inside should be matched as "apple". It works for 2.13.x scala version. It can be workarounded with

case appleBox: Box[_] @unchecked if appleBox.fruit.isInstanceOf[Apple] =>
  val a = appleBox.asInstanceOf[Box[Apple]]

[som-snytt]

I think the Scala 3 behavior is correct, but on the isInstanceOf, it should warn,

fruitless type test

with the correct pun on fruit.

[lbialy]

Good afternoon,

I'd like to oppose to the notion that Scala 3 behavior is the correct one in this case. For all the time I spent with Scala my understanding of @unchecked in pattern matches, especially the ones on a generic datatype with a provided type parameter, was that I provide a type parameter I'd like to get an implicit cast for and I am aware that I can't match on a generic type, because it's erased. Therefore I have to provide another check to make sure that this implicit cast is a correct one and that guard relying on isInstanceOf was used in exactly that context as it was understood to be a check done in runtime. In this case using @unchecked annotation meant basically "I am aware that this is unchecked and I've done due dilligence to make this patmat safe, don't issue a warning for me".

The warning emitted in Scala 2 informed about exactly that:

example.scala:14: warning: non-variable type argument Main.Apple in type pattern Main.Box[Main.Apple] is unchecked since it is eliminated by erasure
  case appleBox: Box[Apple] if appleBox.fruit.isInstanceOf[Apple] => //contains apple
                 ^

This new behavior puts this whole assumption and learned experience on its head. Now the @unchecked somehow mean "in this pattern match I am sure that the first thing you are going to match on is definitely Box[Apple]". This is, no offense, ridiculous and completely misses the point of using pattern matching. In this case @unchecked is basically unusable and introduces wrong behavior into applications while simultaneously suppressing a warning about shady stuff.

tl;dr: folks depended on this behavior of scala 2 to get free implicit casts and used @unchecked to suppress warning because they knew that they are going off the rails and introduced a proper check in a guard. This change breaks existing code and understanding of erasure in patmat.

Source: I work with @liosedhel, we have encountered this as a bug leading to ClassCastExceptions on prod in a system migrated to Scala 3.

[som-snytt]

I think the PR respects the extended comment. I think that is OK if folks want to "de-optimize" the isInstanceOf.

I stand by my pun, of course.

The spec should clarify that if I say case box: Box[C] (with or without warning) then box is-a Box[C]. No subsequent behavior should depend on whether a warning was emitted.

But that is not what the previous comment asks.

So the spec must clarify that case box: Box[C] might be a lie. That doesn't sound right. If you get a warning, and you prefer to heed warnings, make it case box: Box[_] as shown. I'm not sure what words the spec should use.

Full disclosure, I don't use the idiom that warns, so I don't know what is most useful.

[sjrd]

Now the @unchecked somehow mean "in this pattern match I am sure that the first thing you are going to match on is definitely Box[Apple]".

No, it means that "in this pattern March, if the thing you are going to match is a Box[_], then I am sure that it is definitely a Box[Apple].

The meaning was the same in Scala 2. The difference is only that Scala 2 did not use the knowledge to constant fold the isInstanceOf to true.

---

Now, there's a case to be made that such a constant fold should emit a warning as a tautological test.

[odersky]

@lbialy has a point. What would be an alternative to

case appleBox: Box[Apple] @unchecked if appleBox.fruit.isInstanceOf[Apple] =>

if we rejected the isInstanceOf as redundant? I don't see a good alternative. I did not know about this pattern before, but now that I have seen it, I see several places where it could simplify our own code in dotc.

Note also that we do in general flag redundant isInstanceOf checks as warnings. Only here, it is not recognized to be redundant since appleBox.fruit could be null. To be sure we could flag that situation as well, that would be a solution to the issue.

So the choice seems to be between two alternatives:

1. Behave like Scala 2 and don't optimize isInstanceOf checks, at least not if the scrutinee has an @unchecked type.
2. Always flag redundant up to null isInstanceOf checks as warnings.

(2) poses backwards compatibility problems. First, a warning is a weak message to signal a change in runtime behavior. Second, we are still lacking a good replacement for such code. That's why #14709 implements (1).

[sjrd]

1. Behave like Scala 2 and don't optimize isInstanceOf checks, at least not if the scrutinee has an @unchecked type.

If it is only "if the scrutinee has an @unchecked type", it means that

case appleBox: Box[Apple] @unchecked if appleBox.fruit.isInstanceOf[Apple] =>
  1
case _ =>
  2

is not equivalent to

case appleBox: Box[Apple] @unchecked =>
  if appleBox.fruit.isInstanceOf[Apple] then
    1
  else
    2
case _ =>
  2

anymore. Is that acceptable?

In other words, it's not really a question of optimizing or not. It's a question of changing the specified semantics of types inside the guard of a pattern match that has @unchecked. Do you want to put that in the spec? We would have to say something like, in the guard of a case, we don't trust the types beyond their erasure. What does that even mean in terms of type system? Can we still remotely reason about soundness in that situation? What about GADT soundness in this context? There is so much we don't know once we "relax" run-time semantics like that. Previously, I could explain the meaning of @unchecked in one sentenced (see my previous comment); now I need to make sweeping changes to my understanding of the type system.

[odersky]

In fact the two snippets are still equivalent. The scrutinee has an @unchecked type in both.
applebox is of type Box[Apple] @unchecked.

[odersky]

Another approach would be to never optimize isInstanceOfs that were written by the user. Only compiler-generated tests could be optimized away. That avoids the problems in explaining the semantics.