Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warning message for matching on redundant nulls #21850

Merged
merged 6 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/SymbolLoaders.scala
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ abstract class SymbolLoader extends LazyType { self =>
val sym = root.symbol
def associatedFile = root.symbol.associatedFile match
case file: AbstractFile => file
case _ => NoAbstractFile
case null => NoAbstractFile
ctx.profiler.onCompletion(sym, associatedFile)(body)
}

Expand Down
28 changes: 22 additions & 6 deletions compiler/src/dotty/tools/dotc/transform/patmat/Space.scala
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ object SpaceEngine {
def isSubspace(a: Space, b: Space)(using Context): Boolean = a.isSubspace(b)
def canDecompose(typ: Typ)(using Context): Boolean = typ.canDecompose
def decompose(typ: Typ)(using Context): List[Typ] = typ.decompose
def nullSpace(using Context): Space = Typ(ConstantType(Constant(null)), decomposed = false)

/** Simplify space such that a space equal to `Empty` becomes `Empty` */
def computeSimplify(space: Space)(using Context): Space = trace(i"simplify($space)")(space match {
Expand Down Expand Up @@ -336,6 +337,13 @@ object SpaceEngine {
case pat: Ident if isBackquoted(pat) =>
Typ(pat.tpe, decomposed = false)

case Ident(nme.WILDCARD) =>
val tp = pat.tpe.stripAnnots.widenSkolem
val isNullable = tp.isInstanceOf[FlexibleType] || tp.classSymbol.isNullableClass
val tpSpace = Typ(erase(tp, isValue = true), decomposed = false)
if isNullable then Or(tpSpace :: nullSpace :: Nil)
else tpSpace

case Ident(_) | Select(_, _) =>
Typ(erase(pat.tpe.stripAnnots.widenSkolem, isValue = true), decomposed = false)

Expand Down Expand Up @@ -667,7 +675,7 @@ object SpaceEngine {
case tp => (tp, Nil)
val (tp, typeArgs) = getAppliedClass(tpOriginal)
// This function is needed to get the arguments of the types that will be applied to the class.
// This is necessary because if the arguments of the types contain Nothing,
// This is necessary because if the arguments of the types contain Nothing,
// then this can affect whether the class will be taken into account during the exhaustiveness check
def getTypeArgs(parent: Symbol, child: Symbol, typeArgs: List[Type]): List[Type] =
val superType = child.typeRef.superType
Expand Down Expand Up @@ -930,7 +938,7 @@ object SpaceEngine {
if isNullable && !ctx.mode.is(Mode.SafeNulls)
then project(OrType(selTyp, ConstantType(Constant(null)), soft = false))
else project(selTyp)

var hadNullOnly = false
@tailrec def recur(cases: List[CaseDef], prevs: List[Space], deferred: List[Tree]): Unit =
cases match
case Nil =>
Expand All @@ -946,11 +954,19 @@ object SpaceEngine {

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)
if isSubspace(covered, prev) then
report.warning(MatchCaseUnreachable(), pat.srcPos)
else if isNullable && !hadNullOnly && isWildcardArg(pat)
&& isSubspace(covered, Or(prev :: nullSpace :: Nil)) then
// Issue OnlyNull warning only if:
// 1. The target space is nullable;
// 2. OnlyNull warning has not been issued before;
// 3. The pattern is a wildcard pattern;
// 4. The pattern is not covered by the previous cases,
// but covered by the previous cases with null.
hadNullOnly = true
report.warning(MatchCaseOnlyNullWarning(), pat.srcPos)

// in redundancy check, take guard as false in order to soundly approximate
val newPrev = if guard.isEmpty then covered :: prevs else prevs
Expand Down
14 changes: 9 additions & 5 deletions tests/explicit-nulls/warn/i21577.check
Original file line number Diff line number Diff line change
@@ -1,25 +1,29 @@
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/i21577.scala:5:9 --------------------------------------------
5 | case _ => // warn
5 | case _ => // warn: null only
| ^
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/i21577.scala:12:9 -------------------------------------------
12 | case _ => // warn
12 | case _ => // warn: null only
| ^
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/i21577.scala:16:7 -------------------------------------------
16 | case _ => // warn: null only
| ^
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
-- [E030] Match case Unreachable Warning: tests/explicit-nulls/warn/i21577.scala:20:7 ----------------------------------
20 | case _ => // warn
20 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E029] Pattern Match Exhaustivity Warning: tests/explicit-nulls/warn/i21577.scala:29:27 -----------------------------
29 |def f7(s: String | Null) = s match // warn
29 |def f7(s: String | Null) = s match // warn: not exhuastive
| ^
| match may not be exhaustive.
|
| It would fail on pattern case: _: Null
|
| longer explanation available when compiling with `-explain`
-- [E029] Pattern Match Exhaustivity Warning: tests/explicit-nulls/warn/i21577.scala:36:33 -----------------------------
36 |def f9(s: String | Int | Null) = s match // warn
36 |def f9(s: String | Int | Null) = s match // warn: not exhuastive
| ^
| match may not be exhaustive.
|
Expand Down
12 changes: 6 additions & 6 deletions tests/explicit-nulls/warn/i21577.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,22 @@ def f(s: String) =
val s2 = s.trim()
s2 match
case s3: String =>
case _ => // warn
case _ => // warn: null only


def f2(s: String | Null) =
val s2 = s.nn.trim()
s2 match
case s3: String =>
case _ => // warn
case _ => // warn: null only

def f3(s: String | Null) = s match
case s2: String =>
case _ =>
case _ => // warn: null only

def f5(s: String) = s match
case _: String =>
case _ => // warn
case _ => // warn: unreachable

def f6(s: String) = s.trim() match
case _: String =>
Expand All @@ -26,13 +26,13 @@ def f6(s: String) = s.trim() match
def f61(s: String) = s.trim() match
case _: String =>

def f7(s: String | Null) = s match // warn
def f7(s: String | Null) = s match // warn: not exhuastive
case _: String =>

def f8(s: String | Null) = s match
case _: String =>
case null =>

def f9(s: String | Int | Null) = s match // warn
def f9(s: String | Int | Null) = s match // warn: not exhuastive
case _: String =>
case null =>
1 change: 0 additions & 1 deletion tests/patmat/i12530.check
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
6: Match case Unreachable
14: Match case Unreachable
3 changes: 2 additions & 1 deletion tests/patmat/null.check
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
6: Pattern Match
6: Match case Unreachable
13: Pattern Match
18: Match case Unreachable
20: Pattern Match
4 changes: 2 additions & 2 deletions tests/warn/i20121.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ case class CC_B[A](a: A) extends T_A[A, X]

val v_a: T_A[X, X] = CC_B(null)
val v_b = v_a match
case CC_B(_) => 0 // warn: unreachable
case _ => 1
case CC_B(_) => 0
case _ => 1 // warn: null only
// for CC_B[A] to match T_A[X, X]
// A := X
// so require X, aka T_A[Byte, Byte]
Expand Down
2 changes: 1 addition & 1 deletion tests/warn/i20122.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ case class CC_E(a: CC_C[Char, Byte])

val v_a: T_B[Int, CC_A] = CC_B(CC_E(CC_C(null)))
val v_b = v_a match
case CC_B(CC_E(CC_C(_))) => 0 // warn: unreachable
case CC_B(CC_E(CC_C(_))) => 0
case _ => 1
// for CC_B[A, C] to match T_B[C, CC_A]
// C <: Int, ok
Expand Down
2 changes: 1 addition & 1 deletion tests/warn/i20123.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ case class CC_G[A, C](c: C) extends T_A[A, C]
val v_a: T_A[Boolean, T_B[Boolean]] = CC_G(null)
val v_b = v_a match {
case CC_D() => 0
case CC_G(_) => 1 // warn: unreachable
case CC_G(_) => 1
// for CC_G[A, C] to match T_A[Boolean, T_B[Boolean]]
// A := Boolean, which is ok
// C := T_B[Boolean],
Expand Down
56 changes: 56 additions & 0 deletions tests/warn/redundant-null.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:10:7 -----------------------------------------
10 | case _: n.type => // warn: unreachable
| ^^^^^^^^^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:12:7 -----------------------------------------
12 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:13:7 -----------------------------------------
13 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:18:7 -----------------------------------------
18 | case _ => 3 // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:23:7 -----------------------------------------
23 | case _: B => // warn: unreachable
| ^^^^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:24:7 -----------------------------------------
24 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:25:7 -----------------------------------------
25 | case null => // warn: unreachable
| ^^^^
| Unreachable case
-- [E121] Pattern Match Warning: tests/warn/redundant-null.scala:30:7 --------------------------------------------------
30 | case _ => // warn: null only
| ^
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:31:7 -----------------------------------------
31 | case null => // warn: unreachable
| ^^^^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:32:7 -----------------------------------------
32 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:33:7 -----------------------------------------
33 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:37:7 -----------------------------------------
37 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:41:7 -----------------------------------------
41 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E121] Pattern Match Warning: tests/warn/redundant-null.scala:45:7 --------------------------------------------------
45 | case _ => // warn: null only
| ^
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
45 changes: 45 additions & 0 deletions tests/warn/redundant-null.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
class A
class B
class C

val n = null

def f(s: A) = s match
case _: n.type =>
case _: A =>
case _: n.type => // warn: unreachable
case null =>
case _ => // warn: unreachable
case _ => // warn: unreachable

def f2(s: A | B | C) = s match
case _: A => 0
case _: C | null | _: B => 1
case _ => 3 // warn: unreachable

def f3(s: A | B) = s match
case _: A =>
case _ =>
case _: B => // warn: unreachable
case _ => // warn: unreachable
case null => // warn: unreachable

def f4(s: String | Int) = s match
case _: Int =>
case _: String =>
case _ => // warn: null only
case null => // warn: unreachable
case _ => // warn: unreachable
case _ => // warn: unreachable

def f5(x: String) = x match
case x =>
case _ => // warn: unreachable

def test(s: String | Null) = s match
case ss =>
case _ => // warn: unreachable

def test2(s: String | Null) = s match
case ss: String =>
case _ => // warn: null only
Loading