Skip to content

Commit

Permalink
Improve warning for wildcard matching only null under the explicit nu…
Browse files Browse the repository at this point in the history
…lls flag (scala#21577) (#21623)

Improve warning for wildcard matching only null under the explicit nulls. Fix #21577
  • Loading branch information
noti0na1 authored Oct 28, 2024
2 parents dd37f07 + cb9fcd8 commit a3786a5
Show file tree
Hide file tree
Showing 8 changed files with 130 additions and 44 deletions.
77 changes: 34 additions & 43 deletions compiler/src/dotty/tools/dotc/transform/patmat/Space.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import typer.*, Applications.*, Inferencing.*, ProtoTypes.*
import util.*

import scala.annotation.internal.sharable
import scala.annotation.tailrec
import scala.collection.mutable

import SpaceEngine.*
Expand Down Expand Up @@ -696,7 +697,7 @@ object SpaceEngine {
else NoType
}.filter(_.exists)
parts

case tp: FlexibleType => List(tp.underlying, ConstantType(Constant(null)))
case _ => ListOfNoType
end rec

Expand Down Expand Up @@ -876,6 +877,7 @@ object SpaceEngine {
case tp: SingletonType => toUnderlying(tp.underlying)
case tp: ExprType => toUnderlying(tp.resultType)
case AnnotatedType(tp, annot) => AnnotatedType(toUnderlying(tp), annot)
case tp: FlexibleType => tp.derivedFlexibleType(toUnderlying(tp.underlying))
case _ => tp
})

Expand Down Expand Up @@ -910,52 +912,41 @@ object SpaceEngine {
&& !sel.tpe.widen.isRef(defn.QuotedExprClass)
&& !sel.tpe.widen.isRef(defn.QuotedTypeClass)

def checkReachability(m: Match)(using Context): Unit = trace(i"checkReachability($m)") {
val cases = m.cases.toIndexedSeq

def checkReachability(m: Match)(using Context): Unit = trace(i"checkReachability($m)"):
val selTyp = toUnderlying(m.selector.tpe).dealias

val isNullable = selTyp.classSymbol.isNullableClass
val targetSpace = trace(i"targetSpace($selTyp)")(if isNullable
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)
)

var i = 0
val len = cases.length
var prevs = List.empty[Space]
var deferred = List.empty[Tree]

while (i < len) {
val CaseDef(pat, guard, _) = cases(i)

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
deferred ::= pat
else {
for (pat <- deferred.reverseIterator)
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 && i == len - 1 && isWildcardArg(pat)
val msg = if nullOnly then MatchCaseOnlyNullWarning() else MatchCaseUnreachable()
report.warning(msg, pat.srcPos)
}
deferred = Nil
}

// in redundancy check, take guard as false in order to soundly approximate
prevs ::= (if guard.isEmpty then covered else Empty)
i += 1
}
}
@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)

// in redundancy check, take guard as false in order to soundly approximate
val newPrev = if guard.isEmpty then covered :: prevs else prevs
recur(rest, newPrev, Nil)

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

def checkMatch(m: Match)(using Context): Unit =
if exhaustivityCheckable(m.selector) then checkExhaustivity(m)
Expand Down
5 changes: 5 additions & 0 deletions compiler/test/dotty/tools/dotc/CompilationTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,11 @@ class CompilationTests {
)
}.checkCompile()

@Test def explicitNullsWarn: Unit = {
implicit val testGroup: TestGroup = TestGroup("explicitNullsWarn")
compileFilesInDir("tests/explicit-nulls/warn", explicitNullsOptions)
}.checkWarnings()

@Test def explicitNullsRun: Unit = {
implicit val testGroup: TestGroup = TestGroup("explicitNullsRun")
compileFilesInDir("tests/explicit-nulls/run", explicitNullsOptions)
Expand Down
2 changes: 1 addition & 1 deletion tests/explicit-nulls/pos/interop-constructor.scala
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Test that constructors have a non-nullab.e return type.
// Test that constructors have a non-nullable return type.

class Foo {
val x: java.lang.String = new java.lang.String()
Expand Down
28 changes: 28 additions & 0 deletions tests/explicit-nulls/warn/i21577.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/i21577.scala:5:9 --------------------------------------------
5 | case _ => // warn
| ^
| 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
| ^
| 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
| ^
| Unreachable case
-- [E029] Pattern Match Exhaustivity Warning: tests/explicit-nulls/warn/i21577.scala:29:27 -----------------------------
29 |def f7(s: String | Null) = s match // warn
| ^
| 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
| ^
| match may not be exhaustive.
|
| It would fail on pattern case: _: Int
|
| longer explanation available when compiling with `-explain`
38 changes: 38 additions & 0 deletions tests/explicit-nulls/warn/i21577.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
def f(s: String) =
val s2 = s.trim()
s2 match
case s3: String =>
case _ => // warn


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

def f3(s: String | Null) = s match
case s2: String =>
case _ =>

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

def f6(s: String) = s.trim() match
case _: String =>
case null =>

def f61(s: String) = s.trim() match
case _: String =>

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

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

def f9(s: String | Int | Null) = s match // warn
case _: String =>
case null =>
8 changes: 8 additions & 0 deletions tests/explicit-nulls/warn/interop.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/interop/S.scala:8:11 ----------------------------------------
8 | case _ => // warn
| ^
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/interop/S.scala:9:9 -----------------------------------------
9 | case _ => println(2) // warn
| ^
| Unreachable case except for null (if this is intentional, consider writing case null => instead).
6 changes: 6 additions & 0 deletions tests/explicit-nulls/warn/interop/J.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import java.util.ArrayList;

class J {
ArrayList<ArrayList<String>> foo(String x) { return null; }
static String fooStatic(String x) { return null; }
}
10 changes: 10 additions & 0 deletions tests/explicit-nulls/warn/interop/S.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import java.util.ArrayList
def f() =
val j = new J()
val s2 = j.foo(null)
s2 match
case s3: ArrayList[ArrayList[String]] => s3.get(0) match
case _: ArrayList[_] =>
case _ => // warn
case _ => println(2) // warn

0 comments on commit a3786a5

Please sign in to comment.