Skip to content

Commit a3786a5

Browse files
noti0na1noti0na1
authored
Improve warning for wildcard matching only null under the explicit nulls flag (scala#21577) (#21623)
Improve warning for wildcard matching only null under the explicit nulls. Fix #21577
2 parents dd37f07 + cb9fcd8 commit a3786a5

File tree

8 files changed

+130
-44
lines changed

8 files changed

+130
-44
lines changed

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

Lines changed: 34 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import typer.*, Applications.*, Inferencing.*, ProtoTypes.*
1414
import util.*
1515

1616
import scala.annotation.internal.sharable
17+
import scala.annotation.tailrec
1718
import scala.collection.mutable
1819

1920
import SpaceEngine.*
@@ -696,7 +697,7 @@ object SpaceEngine {
696697
else NoType
697698
}.filter(_.exists)
698699
parts
699-
700+
case tp: FlexibleType => List(tp.underlying, ConstantType(Constant(null)))
700701
case _ => ListOfNoType
701702
end rec
702703

@@ -876,6 +877,7 @@ object SpaceEngine {
876877
case tp: SingletonType => toUnderlying(tp.underlying)
877878
case tp: ExprType => toUnderlying(tp.resultType)
878879
case AnnotatedType(tp, annot) => AnnotatedType(toUnderlying(tp), annot)
880+
case tp: FlexibleType => tp.derivedFlexibleType(toUnderlying(tp.underlying))
879881
case _ => tp
880882
})
881883

@@ -910,52 +912,41 @@ object SpaceEngine {
910912
&& !sel.tpe.widen.isRef(defn.QuotedExprClass)
911913
&& !sel.tpe.widen.isRef(defn.QuotedTypeClass)
912914

913-
def checkReachability(m: Match)(using Context): Unit = trace(i"checkReachability($m)") {
914-
val cases = m.cases.toIndexedSeq
915-
915+
def checkReachability(m: Match)(using Context): Unit = trace(i"checkReachability($m)"):
916916
val selTyp = toUnderlying(m.selector.tpe).dealias
917-
918-
val isNullable = selTyp.classSymbol.isNullableClass
919-
val targetSpace = trace(i"targetSpace($selTyp)")(if isNullable
917+
val isNullable = selTyp.isInstanceOf[FlexibleType] || selTyp.classSymbol.isNullableClass
918+
val targetSpace = trace(i"targetSpace($selTyp)"):
919+
if isNullable && !ctx.mode.is(Mode.SafeNulls)
920920
then project(OrType(selTyp, ConstantType(Constant(null)), soft = false))
921921
else project(selTyp)
922-
)
923-
924-
var i = 0
925-
val len = cases.length
926-
var prevs = List.empty[Space]
927-
var deferred = List.empty[Tree]
928-
929-
while (i < len) {
930-
val CaseDef(pat, guard, _) = cases(i)
931-
932-
val curr = trace(i"project($pat)")(project(pat))
933-
934-
val covered = trace("covered")(simplify(intersect(curr, targetSpace)))
935-
936-
val prev = trace("prev")(simplify(Or(prevs)))
937922

938-
if prev == Empty && covered == Empty then // defer until a case is reachable
939-
deferred ::= pat
940-
else {
941-
for (pat <- deferred.reverseIterator)
942-
report.warning(MatchCaseUnreachable(), pat.srcPos)
943-
if pat != EmptyTree // rethrow case of catch uses EmptyTree
944-
&& !pat.symbol.isAllOf(SyntheticCase, butNot=Method) // ExpandSAMs default cases use SyntheticCase
945-
&& isSubspace(covered, prev)
946-
then {
947-
val nullOnly = isNullable && i == len - 1 && isWildcardArg(pat)
948-
val msg = if nullOnly then MatchCaseOnlyNullWarning() else MatchCaseUnreachable()
949-
report.warning(msg, pat.srcPos)
950-
}
951-
deferred = Nil
952-
}
953-
954-
// in redundancy check, take guard as false in order to soundly approximate
955-
prevs ::= (if guard.isEmpty then covered else Empty)
956-
i += 1
957-
}
958-
}
923+
@tailrec def recur(cases: List[CaseDef], prevs: List[Space], deferred: List[Tree]): Unit =
924+
cases match
925+
case Nil =>
926+
case CaseDef(pat, guard, _) :: rest =>
927+
val curr = trace(i"project($pat)")(project(pat))
928+
val covered = trace("covered")(simplify(intersect(curr, targetSpace)))
929+
val prev = trace("prev")(simplify(Or(prevs)))
930+
if prev == Empty && covered == Empty then // defer until a case is reachable
931+
recur(rest, prevs, pat :: deferred)
932+
else
933+
for pat <- deferred.reverseIterator
934+
do report.warning(MatchCaseUnreachable(), pat.srcPos)
935+
936+
if pat != EmptyTree // rethrow case of catch uses EmptyTree
937+
&& !pat.symbol.isAllOf(SyntheticCase, butNot=Method) // ExpandSAMs default cases use SyntheticCase
938+
&& isSubspace(covered, prev)
939+
then
940+
val nullOnly = isNullable && rest.isEmpty && isWildcardArg(pat)
941+
val msg = if nullOnly then MatchCaseOnlyNullWarning() else MatchCaseUnreachable()
942+
report.warning(msg, pat.srcPos)
943+
944+
// in redundancy check, take guard as false in order to soundly approximate
945+
val newPrev = if guard.isEmpty then covered :: prevs else prevs
946+
recur(rest, newPrev, Nil)
947+
948+
recur(m.cases, Nil, Nil)
949+
end checkReachability
959950

960951
def checkMatch(m: Match)(using Context): Unit =
961952
if exhaustivityCheckable(m.selector) then checkExhaustivity(m)

‎compiler/test/dotty/tools/dotc/CompilationTests.scala‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,11 @@ class CompilationTests {
213213
)
214214
}.checkCompile()
215215

216+
@Test def explicitNullsWarn: Unit = {
217+
implicit val testGroup: TestGroup = TestGroup("explicitNullsWarn")
218+
compileFilesInDir("tests/explicit-nulls/warn", explicitNullsOptions)
219+
}.checkWarnings()
220+
216221
@Test def explicitNullsRun: Unit = {
217222
implicit val testGroup: TestGroup = TestGroup("explicitNullsRun")
218223
compileFilesInDir("tests/explicit-nulls/run", explicitNullsOptions)

‎tests/explicit-nulls/pos/interop-constructor.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Test that constructors have a non-nullab.e return type.
1+
// Test that constructors have a non-nullable return type.
22

33
class Foo {
44
val x: java.lang.String = new java.lang.String()

‎tests/explicit-nulls/warn/i21577.check‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/i21577.scala:5:9 --------------------------------------------
2+
5 | case _ => // warn
3+
| ^
4+
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
5+
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/i21577.scala:12:9 -------------------------------------------
6+
12 | case _ => // warn
7+
| ^
8+
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
9+
-- [E030] Match case Unreachable Warning: tests/explicit-nulls/warn/i21577.scala:20:7 ----------------------------------
10+
20 | case _ => // warn
11+
| ^
12+
| Unreachable case
13+
-- [E029] Pattern Match Exhaustivity Warning: tests/explicit-nulls/warn/i21577.scala:29:27 -----------------------------
14+
29 |def f7(s: String | Null) = s match // warn
15+
| ^
16+
| match may not be exhaustive.
17+
|
18+
| It would fail on pattern case: _: Null
19+
|
20+
| longer explanation available when compiling with `-explain`
21+
-- [E029] Pattern Match Exhaustivity Warning: tests/explicit-nulls/warn/i21577.scala:36:33 -----------------------------
22+
36 |def f9(s: String | Int | Null) = s match // warn
23+
| ^
24+
| match may not be exhaustive.
25+
|
26+
| It would fail on pattern case: _: Int
27+
|
28+
| longer explanation available when compiling with `-explain`

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
def f(s: String) =
2+
val s2 = s.trim()
3+
s2 match
4+
case s3: String =>
5+
case _ => // warn
6+
7+
8+
def f2(s: String | Null) =
9+
val s2 = s.nn.trim()
10+
s2 match
11+
case s3: String =>
12+
case _ => // warn
13+
14+
def f3(s: String | Null) = s match
15+
case s2: String =>
16+
case _ =>
17+
18+
def f5(s: String) = s match
19+
case _: String =>
20+
case _ => // warn
21+
22+
def f6(s: String) = s.trim() match
23+
case _: String =>
24+
case null =>
25+
26+
def f61(s: String) = s.trim() match
27+
case _: String =>
28+
29+
def f7(s: String | Null) = s match // warn
30+
case _: String =>
31+
32+
def f8(s: String | Null) = s match
33+
case _: String =>
34+
case null =>
35+
36+
def f9(s: String | Int | Null) = s match // warn
37+
case _: String =>
38+
case null =>

‎tests/explicit-nulls/warn/interop.check‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/interop/S.scala:8:11 ----------------------------------------
2+
8 | case _ => // warn
3+
| ^
4+
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
5+
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/interop/S.scala:9:9 -----------------------------------------
6+
9 | case _ => println(2) // warn
7+
| ^
8+
| Unreachable case except for null (if this is intentional, consider writing case null => instead).

‎tests/explicit-nulls/warn/interop/J.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import java.util.ArrayList;
2+
3+
class J {
4+
ArrayList<ArrayList<String>> foo(String x) { return null; }
5+
static String fooStatic(String x) { return null; }
6+
}

‎tests/explicit-nulls/warn/interop/S.scala‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import java.util.ArrayList
2+
def f() =
3+
val j = new J()
4+
val s2 = j.foo(null)
5+
s2 match
6+
case s3: ArrayList[ArrayList[String]] => s3.get(0) match
7+
case _: ArrayList[_] =>
8+
case _ => // warn
9+
case _ => println(2) // warn
10+

0 commit comments

Comments
 (0)