Improve warning for wildcard matching only null under the explicit nulls flag (scala#21577)

[HarrisL2]

Improve warning for wildcard matching only null under the explicit nulls flag

Fixes #21577

I have signed the CLA

[noti0na1]

The reason why there is a warning for the "unreachable wildcard except null" cases is: this kind of cases we normally don't write. If there is an extra wildcard case at the end, it much match with null, and it is better to explicitly write it down.

For example, if val a: A,

• with explicit nulls, a is expected to not carry a null value, so a case _: A => should cover all possible values, and any additional cases should be considered "unreachable";
• without explicit nulls, a is expected to carry a non-nullable value in most of cases, and a case _: A => is normally good enough to cover all possible values. If there is an additional case _ =>, it must match with null, so we give a "unreachable except null" warning.

For val a: FlexibleType(A) in explicit nulls, we want to treat it as A without explicit nulls in terms of space analyse. Hence, a match case _: A => should not produce any warning. If there is an additional case _ =>, we give a "unreachable except null" warning similarly.

[noti0na1]

The indent need to be fixed.

[noti0na1]

I guess f and f2 are testing the same thing? Both selectors are flexible types.

[noti0na1]

I expect no warning here.

[HarrisL2]

Isn't this case only reachable by Null? In which case we should emit a warning that it's unreachable except for null. Or is it allowed because we explicitly have Null in the selector type?

[noti0na1]

Well, with explcit nullls, we can really consider String and Null as two independent types, similar to String and Int, so the semantic is clear the second case matches null value. What do you think? cc @olhotak

[olhotak]

Without explicit nulls, this warns. I think we can keep the warning. People can still write case null => for this, which is more explicit. I think it would get tricky trying to distinguish between String | Null and a flexible String type for the scrutinee. If we start getting complaints about the warning, we can revisit it in the future.

[noti0na1]

If the type is String | Null with explcit nullls, I just find it confusing to call the wildcard case "unreachable", because the types clearly indicate what values are covered or not.

[olhotak]

So is the intention for this warning to issue it only when the scrutinee has a flexible type?

How would we best test for that?

[noti0na1]

I think the isNullable is the problem. There are some other things I don't like about this part of code. I will try to do
a refactor to this part.

[noti0na1]

Please considering adding more tests:

def f(s: String) = s match
  case _: String =>
  case _ =>
  
def f(s: String) = s match
  case _: String =>
  case null =>
 
def f(s: String | Null) = s match
  case _: String => 
  
def f(s: String | Null) = s match
  case _: String => 
  case null =>
  
def f(s: String | Int | Null) = s match
  case _: String =>
  case null =>
  
// etc...

[noti0na1]

Another issue which can be improved:

class A
class B

def f(s: A | B) = s match
  case _: A => 0
  case null => 1
  case _: B => 2
  case _ => 3

If we compile without explicit nulls, it says the last wildcard case is "Unreachable case except for null". If we change to case null =>, it will say it is Unreachable.

It should be unreachable from the begining.

[noti0na1]

I did some refactoring to checkReachability, can you test if this version works?

  def checkReachability(m: Match)(using Context): Unit = trace(i"checkReachability($m)"):
    val selTyp = toUnderlying(m.selector.tpe).dealias
    val isNullable = selTyp.isInstanceOf[FlexibleType] || selTyp.classSymbol.isNullableClass
    val targetSpace = trace(i"targetSpace($selTyp)"):
      if isNullable && !ctx.mode.is(Mode.SafeNulls)
      then project(OrType(selTyp, ConstantType(Constant(null)), soft = false))
      else project(selTyp)

    @tailrec def recur(cases: List[CaseDef], prevs: List[Space], deferred: List[Tree]): Unit =
      cases match
        case Nil =>
        case CaseDef(pat, guard, _) :: rest =>
          val curr = trace(i"project($pat)")(project(pat))
          val covered = trace("covered")(simplify(intersect(curr, targetSpace)))
          val prev = trace("prev")(simplify(Or(prevs)))

          if prev == Empty && covered == Empty then // defer until a case is reachable
            recur(rest, prevs, pat :: deferred)
          else
            for pat <- deferred.reverseIterator
            do report.warning(MatchCaseUnreachable(), pat.srcPos)

            if pat != EmptyTree // rethrow case of catch uses EmptyTree
                && !pat.symbol.isAllOf(SyntheticCase, butNot=Method) // ExpandSAMs default cases use SyntheticCase
                && isSubspace(covered, prev)
            then
              val nullOnly = isNullable && rest.isEmpty && isWildcardArg(pat)
              val msg = if nullOnly then MatchCaseOnlyNullWarning() else MatchCaseUnreachable()
              report.warning(msg, pat.srcPos)

            val newPrev = if guard.isEmpty then covered :: prevs else prevs
            recur(rest, newPrev, Nil)

    recur(m.cases, Nil, Nil)
  end checkReachability

I think you also need to add a case to decompose:

case tp: FlexibleType                            => List(tp.underlying, ConstantType(Constant(null)))

and a case to toUnderlying:

case tp: FlexibleType                           => tp.derivedFlexibleType(toUnderlying(tp.underlying))

A flexible type is considered "nullable" as it may contain null value, and it is key to keep the flexible type in the space.

I don't think FlexibleType has been handled properly before in this file. Could you also test some examples such that more complicated types are inside the FlexibleType?