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

Improve warning for wildcard matching only null under the explicit nulls flag (scala#21577) #21623

Merged
merged 3 commits into from
Oct 28, 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
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
HarrisL2 marked this conversation as resolved.
Show resolved Hide resolved
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

Loading