Modify pattern matcher to warn when | Null is used in patterns

compiler/src/dotty/tools/dotc/core/NullOpsDecorator.scala

@@ -8,13 +8,14 @@ import Types.*
 object NullOpsDecorator:
 
   extension (self: Type)
-    /** Syntactically strips the nullability from this type.
+    /** If explicit-nulls is enabled, syntactically strips the nullability from this type.
+     *  If explicit-nulls is not enabled and forceStrip is enabled, removes all Null in unions.
      *  If the type is `T1 | ... | Tn`, and `Ti` references to `Null`,
      *  then return `T1 | ... | Ti-1 | Ti+1 | ... | Tn`.
      *  If this type isn't (syntactically) nullable, then returns the type unchanged.
-     *  The type will not be changed if explicit-nulls is not enabled.
+     *  The type will not be changed if explicit-nulls is not enabled and forceStrip is false.
      */
-    def stripNull(stripFlexibleTypes: Boolean = true)(using Context): Type = {
+    def stripNull(stripFlexibleTypes: Boolean = true, forceStrip: Boolean = false)(using Context): Type = {
       def strip(tp: Type): Type =
         val tpWiden = tp.widenDealias
         val tpStripped = tpWiden match {
@@ -42,12 +43,12 @@ object NullOpsDecorator:
         }
         if tpStripped ne tpWiden then tpStripped else tp
 
-      if ctx.explicitNulls then strip(self) else self
+      if ctx.explicitNulls || forceStrip then strip(self) else self
     }
 
     /** Is self (after widening and dealiasing) a type of the form `T | Null`? */
-    def isNullableUnion(using Context): Boolean = {
-      val stripped = self.stripNull()
+    def isNullableUnion(stripFlexibleTypes: Boolean = true, forceStrip: Boolean = false)(using Context): Boolean = {
+      val stripped = self.stripNull(stripFlexibleTypes, forceStrip)
       stripped ne self
     }
   end extension

compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala

@@ -235,6 +235,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
   case CannotInstantiateQuotedTypeVarID // errorNumber: 219
   case DefaultShadowsGivenID // errorNumber: 220
   case RecurseWithDefaultID // errorNumber: 221
+  case MatchCaseUnnecessaryOrNullID // errorNumber: 222
 
   def errorNumber = ordinal - 1
 

compiler/src/dotty/tools/dotc/reporting/messages.scala

@@ -962,6 +962,12 @@ extends PatternMatchMsg(MatchCaseOnlyNullWarningID) {
   def explain(using Context) = ""
 }
 
+class MatchCaseUnnecessaryNullable(tp: Type)(using Context)
+extends PatternMatchMsg(MatchCaseUnnecessaryOrNullID) {
+  def msg(using Context) = i"""$tp is nullable, but will not be matched by ${hl("null")}."""
+  def explain(using Context) = ""
+}
+
 class MatchableWarning(tp: Type, pattern: Boolean)(using Context)
 extends TypeMsg(MatchableWarningID) {
   def msg(using Context) =

compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala

@@ -16,6 +16,7 @@ import reporting.*
 import config.Printers.{ transforms => debug }
 
 import patmat.Typ
+import dotty.tools.dotc.core.NullOpsDecorator.stripNull
 import dotty.tools.dotc.util.SrcPos
 
 /** This transform normalizes type tests and type casts,
@@ -323,7 +324,7 @@ object TypeTestsCasts {
          *  The transform happens before erasure of `testType`, thus cannot be merged
          *  with `transformIsInstanceOf`, which depends on erased type of `testType`.
          */
-        def transformTypeTest(expr: Tree, testType: Type, flagUnrelated: Boolean): Tree = testType.dealias match {
+        def transformTypeTest(expr: Tree, testType: Type, flagUnrelated: Boolean): Tree = testType.dealias.stripNull(stripFlexibleTypes = false, forceStrip = true) match {
           case tref: TermRef if tref.symbol == defn.EmptyTupleModule =>
             ref(defn.RuntimeTuples_isInstanceOfEmptyTuple).appliedTo(expr)
           case _: SingletonType =>

compiler/src/dotty/tools/dotc/transform/patmat/Space.scala

@@ -926,8 +926,38 @@ object SpaceEngine {
       cases match
         case Nil =>
         case (c @ CaseDef(pat, _, _)) :: rest =>
+          def checkForUnnecessaryNullable(p: Tree): Unit = p match {
+            case Bind(_, body) => checkForUnnecessaryNullable(body)
+            case Typed(expr, tpt) =>
+              if tpt.tpe.isNullableUnion(stripFlexibleTypes = false, forceStrip = true) then
+                report.warning(MatchCaseUnnecessaryNullable(tpt.tpe), p.srcPos)
+              else
+                checkForUnnecessaryNullable(expr)
+            case UnApply(_, _, patterns) =>
+              patterns.map(checkForUnnecessaryNullable)
+            case Alternative(patterns) => patterns.map(checkForUnnecessaryNullable)
+            case _ =>
+          }
+
+          def handlesNull(p: Tree): Boolean = p match {
+            case Literal(Constant(null)) => true
+            case Typed(expr, tpt) => tpt.tpe.isSingleton || handlesNull(expr) // assume all SingletonType's handle null (see redundant-null.scala)
+            case Bind(_, body) => handlesNull(body)
+            case Alternative(patterns) => patterns.exists(handlesNull)
+            case _ => false
+          }
           val curr = trace(i"project($pat)")(projectPat(pat))
-          val covered = trace("covered")(simplify(intersect(curr, targetSpace)))
+          var covered = trace("covered")(simplify(intersect(curr, targetSpace)))
+
+          checkForUnnecessaryNullable(pat)
+
+          if !handlesNull(pat) && !isWildcardArg(pat) then
+            // Remove nullSpace from covered only if:
+            // 1. No pattern is the null constant (e.g., `case null =>` or `case ... | null | ... =>`) or
+            // has a SingletonType (e.g., `case _: n.type =>` where `val n = null`, see redundant-null.scala) in one of its pattern(s)
+            // 2. The pattern is a wildcard pattern.
+            covered = minus(covered, nullSpace)
+
           val prev = trace("prev")(simplify(Or(prevs)))
           if prev == Empty && covered == Empty then // defer until a case is reachable
             recur(rest, prevs, pat :: deferred)

compiler/src/dotty/tools/dotc/typer/Nullables.scala

@@ -433,10 +433,10 @@ object Nullables:
     def computeAssignNullable()(using Context): tree.type =
       var nnInfo = tree.rhs.notNullInfo
       tree.lhs match
-        case TrackedRef(ref) if ctx.explicitNulls && ref.isNullableUnion =>
+        case TrackedRef(ref) if ctx.explicitNulls && ref.isNullableUnion() =>
           nnInfo = nnInfo.seq:
             val rhstp = tree.rhs.typeOpt
-            if rhstp.isNullType || rhstp.isNullableUnion then
+            if rhstp.isNullType || rhstp.isNullableUnion() then
               // If the type of rhs is nullable (`T|Null` or `Null`), then the nullability of the
               // lhs variable is no longer trackable. We don't need to check whether the type `T`
               // is correct here, as typer will check it.

tests/explicit-nulls/warn/i23243.scala

@@ -0,0 +1,18 @@
+object Extractor:
+  def unapply(s: String|Null): Boolean = true
+
+class A
+
+def main =
+  ("foo": (A|String)) match
+    case Extractor() => println("foo matched") // warn: String | Null is nullable
+    case _ => println("foo didn't match")
+  val s: Some[Any] = Some(5)
+
+  s match {
+    case Some(s: (String | Null)) => // warn: String | Null is nullable
+    case Some(Some(s: (String | Null))) => // warn: String | Null is nullable
+    case _: s.type =>
+    case Some(s: Int) =>
+    case _ =>
+  }

tests/neg/i23243a.scala

@@ -0,0 +1,8 @@
+def main = {
+  ("foo": String) match {
+    case a: (String | Nothing) => // error
+      println("bar")
+    case _ =>
+      println("foo")
+  }
+}

tests/neg/i4004.scala

@@ -1,13 +1,8 @@
 @main def Test =
-  "a".isInstanceOf[Null] // error
-  null.isInstanceOf[Null] // error
-  "a".isInstanceOf[Nothing] // error
-  "a".isInstanceOf[Singleton] // error
 
   "a" match
   case _: Null => () // error
   case _: Nothing => () // error
-  case _: Singleton => () // error
   case _ => ()
 
   null match

tests/neg/i4004a.scala

@@ -0,0 +1,10 @@
+@main def Test =
+   "a".isInstanceOf[Null] // error
+   null.isInstanceOf[Null] // error
+   "a".isInstanceOf[Nothing] // error
+   "a".isInstanceOf[Singleton] // error
+
+   "a" match {
+    case _: Singleton => () // error
+    case _ => ()
+   }

tests/warn/i23243.check

@@ -0,0 +1,20 @@
+-- [E222] Pattern Match Warning: tests/warn/i23243.scala:8:9 -----------------------------------------------------------
+8 |    case Extractor() => println("foo matched") // warn: String | Null is nullable
+  |         ^^^^^^^^^^^
+  |         String | Null is nullable, but will not be matched by null.
+-- [E222] Pattern Match Warning: tests/warn/i23243.scala:13:17 ---------------------------------------------------------
+13 |    case Some(s: (String | Null)) => // warn: String | Null is nullable
+   |                 ^^^^^^^^^^^^^^^
+   |                 String | Null is nullable, but will not be matched by null.
+-- [E222] Pattern Match Warning: tests/warn/i23243.scala:14:22 ---------------------------------------------------------
+14 |    case Some(Some(s: (String | Null))) => // warn: String | Null is nullable
+   |                      ^^^^^^^^^^^^^^^
+   |                      String | Null is nullable, but will not be matched by null.
+-- [E222] Pattern Match Warning: tests/warn/i23243.scala:17:22 ---------------------------------------------------------
+17 |    case (null | Some(_: (Int | Null)))| Some(_: (String | Null)) => // warn: Int | Null is nullable // warn: String | Null is nullable
+   |                      ^^^^^^^^^^^^^^^
+   |                      Int | Null is nullable, but will not be matched by null.
+-- [E222] Pattern Match Warning: tests/warn/i23243.scala:17:46 ---------------------------------------------------------
+17 |    case (null | Some(_: (Int | Null)))| Some(_: (String | Null)) => // warn: Int | Null is nullable // warn: String | Null is nullable
+   |                                              ^^^^^^^^^^^^^^^^^^
+   |                                              String | Null is nullable, but will not be matched by null.

tests/warn/i23243.scala

@@ -0,0 +1,19 @@
+object Extractor:
+  def unapply(s: String|Null): Boolean = true
+
+class A
+
+def main =
+  ("foo": (A|String)) match
+    case Extractor() => println("foo matched") // warn: String | Null is nullable
+    case _ => println("foo didn't match")
+  val s: Any = 5
+
+  s match {
+    case Some(s: (String | Null)) => // warn: String | Null is nullable
+    case Some(Some(s: (String | Null))) => // warn: String | Null is nullable
+    case Some(null) =>
+    case _: s.type =>
+    case (null | Some(_: (Int | Null)))| Some(_: (String | Null)) => // warn: Int | Null is nullable // warn: String | Null is nullable
+    case _ =>
+  }