From 0cf41d07341a1dc27f8fe4a71b91641612375f90 Mon Sep 17 00:00:00 2001 From: HarrisL2 Date: Fri, 20 Sep 2024 23:23:39 -0400 Subject: [PATCH 1/3] Improve warning for wildcard matching only null under explicit nulls (scala#21577) Adds a more detailed warning message when a wildcard case is only reachable by null under explict nulls flag. Fixes scala#21577 --- .../tools/dotc/transform/patmat/Space.scala | 16 +++++++++---- .../dotty/tools/dotc/CompilationTests.scala | 5 ++++ tests/explicit-nulls/warn/i21577.check | 12 ++++++++++ tests/explicit-nulls/warn/i21577.scala | 24 +++++++++++++++++++ 4 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 tests/explicit-nulls/warn/i21577.check create mode 100644 tests/explicit-nulls/warn/i21577.scala diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index 9fb3c00c67c4..deaf285c79cc 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -920,7 +920,7 @@ object SpaceEngine { then project(OrType(selTyp, ConstantType(Constant(null)), soft = false)) else project(selTyp) ) - + var i = 0 val len = cases.length var prevs = List.empty[Space] @@ -942,11 +942,17 @@ object SpaceEngine { 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) + && isSubspace(covered, Or(List(prev, Typ(defn.NullType)))) // for when Null is not subtype of AnyRef under explicit nulls then { - val nullOnly = isNullable && i == len - 1 && isWildcardArg(pat) - val msg = if nullOnly then MatchCaseOnlyNullWarning() else MatchCaseUnreachable() - report.warning(msg, pat.srcPos) + val nullOnly = + (isNullable || (defn.NullType <:< selTyp)) + && i == len - 1 + && isWildcardArg(pat) + if nullOnly then { + report.warning(MatchCaseOnlyNullWarning(), pat.srcPos) + } else if isSubspace(covered, prev) then { + report.warning(MatchCaseUnreachable(), pat.srcPos) + } } deferred = Nil } diff --git a/compiler/test/dotty/tools/dotc/CompilationTests.scala b/compiler/test/dotty/tools/dotc/CompilationTests.scala index d7ef7f6f6085..de00faa86406 100644 --- a/compiler/test/dotty/tools/dotc/CompilationTests.scala +++ b/compiler/test/dotty/tools/dotc/CompilationTests.scala @@ -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) diff --git a/tests/explicit-nulls/warn/i21577.check b/tests/explicit-nulls/warn/i21577.check new file mode 100644 index 000000000000..944b2ccab3c9 --- /dev/null +++ b/tests/explicit-nulls/warn/i21577.check @@ -0,0 +1,12 @@ +-- [E121] Pattern Match Warning: tests/explicit-nulls/warn/i21577.scala:5:9 -------------------------------------------- +5 | case _ => println(2) // 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:11 ------------------------------------------ +12 | case _ => println(2) // 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:18:11 ------------------------------------------ +18 | case _ => println(2) // warn + | ^ + | Unreachable case except for null (if this is intentional, consider writing case null => instead). diff --git a/tests/explicit-nulls/warn/i21577.scala b/tests/explicit-nulls/warn/i21577.scala new file mode 100644 index 000000000000..d5da1403deea --- /dev/null +++ b/tests/explicit-nulls/warn/i21577.scala @@ -0,0 +1,24 @@ +def f(s: String) = + val s2 = s.trim() + s2 match + case s3: String => println(1) + case _ => println(2) // warn + + +def f2(s: String | Null) = + val s2 = s.nn.trim() + s2 match + case s3: String => println(1) + case _ => println(2) // warn + +def f3(s: String | Null) = + val s2 = s + s2 match + case s3: String => println(1) + case _ => println(2) // warn + +def f4(s: String | Int) = + val s2 = s + s2 match + case s3: String => println(1) + case _ => println(2) \ No newline at end of file From 4f775fab0f16a27d6be276dde0834cd286d8b506 Mon Sep 17 00:00:00 2001 From: HarrisL2 Date: Thu, 17 Oct 2024 15:00:04 -0400 Subject: [PATCH 2/3] Implement change and add test cases --- .../tools/dotc/transform/patmat/Space.scala | 82 ++++++++----------- .../pos/interop-constructor.scala | 2 +- tests/explicit-nulls/warn/i21577.check | 34 ++++++-- tests/explicit-nulls/warn/i21577.scala | 48 +++++++---- tests/explicit-nulls/warn/interop.check | 8 ++ tests/explicit-nulls/warn/interop/J.java | 6 ++ tests/explicit-nulls/warn/interop/S.scala | 10 +++ 7 files changed, 114 insertions(+), 76 deletions(-) create mode 100644 tests/explicit-nulls/warn/interop.check create mode 100644 tests/explicit-nulls/warn/interop/J.java create mode 100644 tests/explicit-nulls/warn/interop/S.scala diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index deaf285c79cc..c7b304e6caf6 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -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.* @@ -696,7 +697,7 @@ object SpaceEngine { else NoType }.filter(_.exists) parts - + case tp: FlexibleType => List(tp.underlying, ConstantType(Constant(null))) case _ => ListOfNoType end rec @@ -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 }) @@ -910,58 +912,40 @@ 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, Or(List(prev, Typ(defn.NullType)))) // for when Null is not subtype of AnyRef under explicit nulls - then { - val nullOnly = - (isNullable || (defn.NullType <:< selTyp)) - && i == len - 1 - && isWildcardArg(pat) - if nullOnly then { - report.warning(MatchCaseOnlyNullWarning(), pat.srcPos) - } else if isSubspace(covered, prev) then { - report.warning(MatchCaseUnreachable(), 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) + + 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) diff --git a/tests/explicit-nulls/pos/interop-constructor.scala b/tests/explicit-nulls/pos/interop-constructor.scala index f222d24b0919..4ebfaa752b3a 100644 --- a/tests/explicit-nulls/pos/interop-constructor.scala +++ b/tests/explicit-nulls/pos/interop-constructor.scala @@ -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() diff --git a/tests/explicit-nulls/warn/i21577.check b/tests/explicit-nulls/warn/i21577.check index 944b2ccab3c9..acedd7a9c713 100644 --- a/tests/explicit-nulls/warn/i21577.check +++ b/tests/explicit-nulls/warn/i21577.check @@ -1,12 +1,28 @@ -- [E121] Pattern Match Warning: tests/explicit-nulls/warn/i21577.scala:5:9 -------------------------------------------- -5 | case _ => println(2) // warn +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:11 ------------------------------------------ -12 | case _ => println(2) // 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:18:11 ------------------------------------------ -18 | case _ => println(2) // 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` diff --git a/tests/explicit-nulls/warn/i21577.scala b/tests/explicit-nulls/warn/i21577.scala index d5da1403deea..67da6068f22c 100644 --- a/tests/explicit-nulls/warn/i21577.scala +++ b/tests/explicit-nulls/warn/i21577.scala @@ -1,24 +1,38 @@ def f(s: String) = val s2 = s.trim() s2 match - case s3: String => println(1) - case _ => println(2) // warn + case s3: String => + case _ => // warn def f2(s: String | Null) = val s2 = s.nn.trim() - s2 match - case s3: String => println(1) - case _ => println(2) // warn - -def f3(s: String | Null) = - val s2 = s - s2 match - case s3: String => println(1) - case _ => println(2) // warn - -def f4(s: String | Int) = - val s2 = s - s2 match - case s3: String => println(1) - case _ => println(2) \ No newline at end of file + 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 => \ No newline at end of file diff --git a/tests/explicit-nulls/warn/interop.check b/tests/explicit-nulls/warn/interop.check new file mode 100644 index 000000000000..0afc1dc0a3cb --- /dev/null +++ b/tests/explicit-nulls/warn/interop.check @@ -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). diff --git a/tests/explicit-nulls/warn/interop/J.java b/tests/explicit-nulls/warn/interop/J.java new file mode 100644 index 000000000000..f81cf685b9a9 --- /dev/null +++ b/tests/explicit-nulls/warn/interop/J.java @@ -0,0 +1,6 @@ +import java.util.ArrayList; + +class J { + ArrayList> foo(String x) { return null; } + static String fooStatic(String x) { return null; } +} diff --git a/tests/explicit-nulls/warn/interop/S.scala b/tests/explicit-nulls/warn/interop/S.scala new file mode 100644 index 000000000000..57beebe4eb76 --- /dev/null +++ b/tests/explicit-nulls/warn/interop/S.scala @@ -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 + From cb9fcd88ac63eed06a16fe9f85deac2fc29b17eb Mon Sep 17 00:00:00 2001 From: HarrisL2 Date: Mon, 28 Oct 2024 11:31:55 -0400 Subject: [PATCH 3/3] Add comment --- compiler/src/dotty/tools/dotc/transform/patmat/Space.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala index c7b304e6caf6..c7f7a236fcf3 100644 --- a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala +++ b/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala @@ -941,6 +941,7 @@ object SpaceEngine { 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)