Modify pattern matcher to raise an error instead of generating illegal isInstanceOf[Null] and isInstanceOf[Nothing]

[SuperCl4sh]

Fixes #23243.

[SuperCl4sh]

@noti0na1 @olhotak If you don't mind, can you take a look? Leaving it as a draft for now since the code isn't finalized.

[som-snytt]

IMO there is nothing illegal about the extractor definition.

The problem is that patmat should always do a null check first, even if the extractor "handles" Null. (Or correct instanceof check. The point is that the extractor is never invoked with null.)

[olhotak]

IMO there is nothing illegal about the extractor definition.

The problem is that patmat should always do a null check first, even if the extractor "handles" Null. (Or correct instanceof check. The point is that the extractor is never invoked with null.)

It is already the case that

  def unapply(s: String|Null): Boolean = true

fails to compile, even without explicit nulls, but with an unhelpful error message (see #23243). All that this PR does is to improve the error message.

The spec is silent on whether such an extractor should or should not compile. We need a spec decision on that question first.

If the decision is that such an extractor should compile (and should behave the same as unapply(s: String)), then the fix would be to make it compile, instead of improving the error message.

How do we make a spec decision?

[som-snytt]

The spec is silent on whether such an extractor should or should not compile.

As with Supreme Court decisions, case law can result in a change of interpretation, but I think silence means it is not forbidden, although "opinionated Scala" opens the door to forbidding things just for aesthetic reasons.

To me, it just looks like a bug. A spec tweak should have a motivating example in the sense of what DX is improved by disallowing the extractor. (Extractor methods are also used outside of pattern matches.)

I don't think a spec tweak requires a SIP, especially if it documents existing behavior. "Possession is 9/10ths of the law."

[sjrd]

A method that happens to be named unapply only takes on an extractor meaning in the context of pattern matching, which is a use site. There is no reason to disallow the definition. The method can be used directly, without pattern matching.

It's the same reason for which we wouldn't disallow the definition of a method named foreach on the grounds that it takes an Int parameter, instead of a function type.

[noti0na1]

Can we just allow test test on nullable types which is not equivalent to Null?

For example, transformTypeTest(x, String | Null) becomes x.isInstanceOf[String], but transformTypeTest(x, Null | Null) will still be error.

Then, we can raise a warning for matching a nullable type, stating null value will not be matched in this case.

[noti0na1]

Considering the case String | (Null | Null), is it easier to call stripNull which can remove all Null from a union type?

[SuperCl4sh]

@noti0na1 When you're free, would you mind checking the latest commit? Right now, code like

object Extractor:
  def unapply(s: String|Null): Boolean = true

class A

def main =
  ("foo": (A|String)) match
    case Extractor() => println("foo matched")
    case _ => println("foo didn't match")

will compile and pattern matching a "true" Null (like Null | Null) won't, but the change I made seems a bit suspicious (https://github.com/scala/scala3/pull/23288/files#diff-4bb92ba355ad252b32b60cd7bc2c64126356048004ea7e44e4bc00a73feec3d6R327). Specifically, it strips all the | Null's before doing anything. I ran testCompilation and all the tests passed, but I'd ideally want another pair of eyes to make sure it's OK. If it is fine, I'll add some more tests, clean up the code, and add the warning you mentioned, then set this PR as ready for review.

[noti0na1]

Forcing stripping null looks good to me. It would be useful if we can produce a warning when we try to match a nullable type (if the type before and after stripping is different), saying something like a null value wouldn't match to this case.