Skip to content

Commit 558d67f

Browse files
SuperCl4shSuperCl4sh
committed
Add warning for having | Null's in patterns in pattern matching
1 parent 3e25a19 commit 558d67f

File tree

10 files changed

+104
-19
lines changed

10 files changed

+104
-19
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ object NullOpsDecorator:
99

1010
extension (self: Type)
1111
/** If explicit-nulls is enabled, syntactically strips the nullability from this type.
12-
* If explicit-nulls is not enabled, removes all Null in unions.
12+
* If explicit-nulls is not enabled and forceStrip is enabled, removes all Null in unions.
1313
* If the type is `T1 | ... | Tn`, and `Ti` references to `Null`,
1414
* then return `T1 | ... | Ti-1 | Ti+1 | ... | Tn`.
1515
* If this type isn't (syntactically) nullable, then returns the type unchanged.
@@ -47,8 +47,8 @@ object NullOpsDecorator:
4747
}
4848

4949
/** Is self (after widening and dealiasing) a type of the form `T | Null`? */
50-
def isNullableUnion(using Context): Boolean = {
51-
val stripped = self.stripNull()
50+
def isNullableUnion(stripFlexibleTypes: Boolean = true, forceStrip: Boolean = false)(using Context): Boolean = {
51+
val stripped = self.stripNull(stripFlexibleTypes, forceStrip)
5252
stripped ne self
5353
}
5454
end extension

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
235235
case CannotInstantiateQuotedTypeVarID // errorNumber: 219
236236
case DefaultShadowsGivenID // errorNumber: 220
237237
case RecurseWithDefaultID // errorNumber: 221
238+
case MatchCaseUnnecessaryOrNullID // errorNumber: 222
238239

239240
def errorNumber = ordinal - 1
240241

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -962,6 +962,12 @@ extends PatternMatchMsg(MatchCaseOnlyNullWarningID) {
962962
def explain(using Context) = ""
963963
}
964964

965+
class MatchCaseUnnecessaryNullable(tp: Type)(using Context)
966+
extends PatternMatchMsg(MatchCaseUnnecessaryOrNullID) {
967+
def msg(using Context) = i"""$tp is nullable, but will not be matched by ${hl("null")}."""
968+
def explain(using Context) = ""
969+
}
970+
965971
class MatchableWarning(tp: Type, pattern: Boolean)(using Context)
966972
extends TypeMsg(MatchableWarningID) {
967973
def msg(using Context) =

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -324,20 +324,20 @@ object TypeTestsCasts {
324324
* The transform happens before erasure of `testType`, thus cannot be merged
325325
* with `transformIsInstanceOf`, which depends on erased type of `testType`.
326326
*/
327-
def transformTypeTest(expr: Tree, testType: Type, flagUnrelated: Boolean): Tree = testType.dealias.stripNull(false, true) match {
327+
def transformTypeTest(expr: Tree, testType: Type, flagUnrelated: Boolean): Tree = testType.dealias.stripNull(stripFlexibleTypes = false, forceStrip = true) match {
328328
case tref: TermRef if tref.symbol == defn.EmptyTupleModule =>
329329
ref(defn.RuntimeTuples_isInstanceOfEmptyTuple).appliedTo(expr)
330330
case _: SingletonType =>
331331
expr.isInstance(testType).withSpan(tree.span)
332-
case t @ OrType(tp1, tp2) =>
332+
case OrType(tp1, tp2) =>
333333
evalOnce(expr) { e =>
334334
transformTypeTest(e, tp1, flagUnrelated = false)
335335
.or(transformTypeTest(e, tp2, flagUnrelated = false))
336336
}
337337
case AndType(tp1, tp2) =>
338338
evalOnce(expr) { e =>
339339
transformTypeTest(e, tp1, flagUnrelated)
340-
.and(transformTypeTest(e, tp2, flagUnrelated))
340+
.and(transformTypeTest(e, tp2, flagUnrelated))
341341
}
342342
case defn.MultiArrayOf(elem, ndims) if isGenericArrayElement(elem, isScala2 = false) =>
343343
def isArrayTest(arg: Tree) =

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

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -925,9 +925,39 @@ object SpaceEngine {
925925
@tailrec def recur(cases: List[CaseDef], prevs: List[Space], deferred: List[Tree]): Unit =
926926
cases match
927927
case Nil =>
928-
case CaseDef(pat, guard, _) :: rest =>
928+
case (c @ CaseDef(pat, guard, _)) :: rest =>
929+
def checkForUnnecessaryNullable(p: Tree): Unit = p match {
930+
case Bind(_, body) => checkForUnnecessaryNullable(body)
931+
case Typed(expr, tpt) =>
932+
if tpt.tpe.isNullableUnion(stripFlexibleTypes = false, forceStrip = true) then
933+
report.warning(MatchCaseUnnecessaryNullable(tpt.tpe), p.srcPos)
934+
else
935+
checkForUnnecessaryNullable(expr)
936+
case UnApply(_, _, patterns) =>
937+
patterns.map(checkForUnnecessaryNullable)
938+
case Alternative(patterns) => patterns.map(checkForUnnecessaryNullable)
939+
case _ =>
940+
}
941+
942+
def handlesNull(p: Tree): Boolean = p match {
943+
case Literal(Constant(null)) => true
944+
case Typed(expr, tpt) => tpt.tpe.isSingleton || handlesNull(expr) // assume all SingletonType's handle null (see redundant-null.scala)
945+
case Bind(_, body) => handlesNull(body)
946+
case Alternative(patterns) => patterns.exists(handlesNull)
947+
case _ => false
948+
}
929949
val curr = trace(i"project($pat)")(projectPat(pat))
930-
val covered = trace("covered")(simplify(intersect(curr, targetSpace)))
950+
var covered = trace("covered")(simplify(intersect(curr, targetSpace)))
951+
952+
checkForUnnecessaryNullable(pat)
953+
954+
if !handlesNull(pat) && !isWildcardArg(pat) then
955+
// Remove nullSpace from covered only if:
956+
// 1. No pattern is the null constant (e.g., `case null =>` or `case ... | null | ... =>`) or
957+
// has a SingletonType (e.g., `case _: n.type =>` where `val n = null`, see redundant-null.scala) in one of its pattern(s)
958+
// 2. The pattern is a wildcard pattern.
959+
covered = minus(covered, nullSpace)
960+
931961
val prev = trace("prev")(simplify(Or(prevs)))
932962
if prev == Empty && covered == Empty then // defer until a case is reachable
933963
recur(rest, prevs, pat :: deferred)

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -433,10 +433,10 @@ object Nullables:
433433
def computeAssignNullable()(using Context): tree.type =
434434
var nnInfo = tree.rhs.notNullInfo
435435
tree.lhs match
436-
case TrackedRef(ref) if ctx.explicitNulls && ref.isNullableUnion =>
436+
case TrackedRef(ref) if ctx.explicitNulls && ref.isNullableUnion() =>
437437
nnInfo = nnInfo.seq:
438438
val rhstp = tree.rhs.typeOpt
439-
if rhstp.isNullType || rhstp.isNullableUnion then
439+
if rhstp.isNullType || rhstp.isNullableUnion() then
440440
// If the type of rhs is nullable (`T|Null` or `Null`), then the nullability of the
441441
// lhs variable is no longer trackable. We don't need to check whether the type `T`
442442
// is correct here, as typer will check it.

‎tests/explicit-nulls/warn/i23243.scala‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
object Extractor:
2+
def unapply(s: String|Null): Boolean = true
3+
4+
class A
5+
6+
def main =
7+
("foo": (A|String)) match
8+
case Extractor() => println("foo matched") // warn: String | Null is nullable
9+
case _ => println("foo didn't match")
10+
val s: Some[Any] = Some(5)
11+
12+
s match {
13+
case Some(s: (String | Null)) => // warn: String | Null is nullable
14+
case Some(Some(s: (String | Null))) => // warn: String | Null is nullable
15+
case _: s.type =>
16+
case Some(s: Int) =>
17+
case _ =>
18+
}

‎tests/pos/i23243.scala‎

Lines changed: 0 additions & 9 deletions
This file was deleted.

‎tests/warn/i23243.check‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
-- [E222] Pattern Match Warning: tests/warn/i23243.scala:8:9 -----------------------------------------------------------
2+
8 | case Extractor() => println("foo matched") // warn: String | Null is nullable
3+
| ^^^^^^^^^^^
4+
| String | Null is nullable, but will not be matched by null.
5+
-- [E222] Pattern Match Warning: tests/warn/i23243.scala:13:17 ---------------------------------------------------------
6+
13 | case Some(s: (String | Null)) => // warn: String | Null is nullable
7+
| ^^^^^^^^^^^^^^^
8+
| String | Null is nullable, but will not be matched by null.
9+
-- [E222] Pattern Match Warning: tests/warn/i23243.scala:14:22 ---------------------------------------------------------
10+
14 | case Some(Some(s: (String | Null))) => // warn: String | Null is nullable
11+
| ^^^^^^^^^^^^^^^
12+
| String | Null is nullable, but will not be matched by null.
13+
-- [E222] Pattern Match Warning: tests/warn/i23243.scala:17:22 ---------------------------------------------------------
14+
17 | case (null | Some(_: (Int | Null)))| Some(_: (String | Null)) => // warn: Int | Null is nullable // warn: String | Null is nullable
15+
| ^^^^^^^^^^^^^^^
16+
| Int | Null is nullable, but will not be matched by null.
17+
-- [E222] Pattern Match Warning: tests/warn/i23243.scala:17:46 ---------------------------------------------------------
18+
17 | case (null | Some(_: (Int | Null)))| Some(_: (String | Null)) => // warn: Int | Null is nullable // warn: String | Null is nullable
19+
| ^^^^^^^^^^^^^^^^^^
20+
| String | Null is nullable, but will not be matched by null.

‎tests/warn/i23243.scala‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
object Extractor:
2+
def unapply(s: String|Null): Boolean = true
3+
4+
class A
5+
6+
def main =
7+
("foo": (A|String)) match
8+
case Extractor() => println("foo matched") // warn: String | Null is nullable
9+
case _ => println("foo didn't match")
10+
val s: Any = 5
11+
12+
s match {
13+
case Some(s: (String | Null)) => // warn: String | Null is nullable
14+
case Some(Some(s: (String | Null))) => // warn: String | Null is nullable
15+
case Some(null) =>
16+
case _: s.type =>
17+
case (null | Some(_: (Int | Null)))| Some(_: (String | Null)) => // warn: Int | Null is nullable // warn: String | Null is nullable
18+
case _ =>
19+
}

0 commit comments

Comments
 (0)