Skip to content

Commit

Permalink
Add suggested changes
Browse files Browse the repository at this point in the history
  • Loading branch information
HarrisL2 committed Nov 4, 2024
1 parent 992f231 commit d53a9ce
Show file tree
Hide file tree
Showing 9 changed files with 100 additions and 71 deletions.
73 changes: 45 additions & 28 deletions compiler/src/dotty/tools/dotc/transform/patmat/Space.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import core.*
import Constants.*, Contexts.*, Decorators.*, Flags.*, NullOpsDecorator.*, Symbols.*, Types.*
import Names.*, NameOps.*, StdNames.*
import ast.*, tpd.*
import config.Printers.*
import config.Printers.exhaustivity
import printing.{ Printer, * }, Texts.*
import reporting.*
import typer.*, Applications.*, Inferencing.*, ProtoTypes.*
Expand Down 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 @@ -525,14 +533,25 @@ object SpaceEngine {
val mt: MethodType = unapp.widen match {
case mt: MethodType => mt
case pt: PolyType =>
scrutineeTp match
case AppliedType(tycon, targs)
if unappSym.is(Synthetic)
&& (pt.resultType.asInstanceOf[MethodType].paramInfos.head.typeConstructor eq tycon) =>
// Special case synthetic unapply/unapplySeq's
// Provided the shapes of the types match:
// the scrutinee type being unapplied and
// the unapply parameter type
pt.instantiate(targs).asInstanceOf[MethodType]
case _ =>
val locked = ctx.typerState.ownedVars
val tvars = constrained(pt)
val mt = pt.instantiate(tvars).asInstanceOf[MethodType]
scrutineeTp <:< mt.paramInfos(0)
val unapplyArgType = mt.paramInfos.head
scrutineeTp <:< unapplyArgType
// force type inference to infer a narrower type: could be singleton
// see tests/patmat/i4227.scala
mt.paramInfos(0) <:< scrutineeTp
maximizeType(mt.paramInfos(0), Spans.NoSpan)
unapplyArgType <:< scrutineeTp
maximizeType(unapplyArgType, Spans.NoSpan)
if !(ctx.typerState.ownedVars -- locked).isEmpty then
// constraining can create type vars out of wildcard types
// (in legalBound, by using a LevelAvoidMap)
Expand All @@ -544,7 +563,7 @@ object SpaceEngine {
// but I'd rather have an unassigned new-new type var, than an infinite loop.
// After all, there's nothing strictly "wrong" with unassigned type vars,
// it just fails TreeChecker's linting.
maximizeType(mt.paramInfos(0), Spans.NoSpan)
maximizeType(unapplyArgType, Spans.NoSpan)
mt
}

Expand Down Expand Up @@ -656,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 @@ -912,50 +931,48 @@ object SpaceEngine {
&& !sel.tpe.widen.isRef(defn.QuotedExprClass)
&& !sel.tpe.widen.isRef(defn.QuotedTypeClass)

def mayCoverNull(tp: Space)(using Context): Boolean = tp match
case Empty => false
case Prod(_, _, _) => false
case Typ(tp, decomposed) => tp == ConstantType(Constant(null))
case Or(ss) => ss.exists(mayCoverNull)

def checkReachability(m: Match)(using Context): Unit = trace(i"checkReachability($m)"):
val selTyp = toUnderlying(m.selector.tpe).dealias
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)

@tailrec def recur(cases: List[CaseDef], prevs: List[Space], deferred: List[Tree], nullCovered: Boolean): Unit =
var hadNullOnly = false
@tailrec def recur(cases: List[CaseDef], prevs: List[Space], deferred: List[Tree]): Unit =
cases match
case Nil =>
case (c @ CaseDef(pat, guard, _)) :: rest =>
val patNullable = Nullables.matchesNull(c)
val curr = trace(i"project($pat)")(
if patNullable
then Or(List(project(pat), Typ(ConstantType(Constant(null)))))
else project(pat))
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, nullCovered)
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, Or(List(prev, Typ(ConstantType(Constant(null))))))
then
val nullOnly = isNullable && isWildcardArg(pat) && !nullCovered && !isSubspace(covered, prev) && (!ctx.explicitNulls || selTyp.isInstanceOf[FlexibleType])
if nullOnly then report.warning(MatchCaseOnlyNullWarning() , pat.srcPos)
else if (isSubspace(covered, prev)) then report.warning(MatchCaseUnreachable(), 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
recur(rest, newPrev, Nil, nullCovered || (guard.isEmpty && patNullable))
val newPrev = if guard.isEmpty then covered :: prevs else prevs
recur(rest, newPrev, Nil)

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

def checkMatch(m: Match)(using Context): Unit =
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 =>
2 changes: 1 addition & 1 deletion tests/patmat/null.check
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
6: Match case Unreachable
6: Pattern Match
13: Pattern Match
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
30 changes: 17 additions & 13 deletions tests/warn/redundant-null.check
Original file line number Diff line number Diff line change
@@ -1,52 +1,56 @@
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:10:7 -----------------------------------------
10 | case _: n.type => // warn
10 | case _: n.type => // warn: unreachable
| ^^^^^^^^^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:12:7 -----------------------------------------
12 | case _ => // warn
12 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:13:7 -----------------------------------------
13 | case _ => // warn
13 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:18:7 -----------------------------------------
18 | case _ => 3 // warn
18 | case _ => 3 // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:23:7 -----------------------------------------
23 | case _: B => // warn
23 | case _: B => // warn: unreachable
| ^^^^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:24:7 -----------------------------------------
24 | case _ => // warn
24 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:25:7 -----------------------------------------
25 | case null => // warn
25 | case null => // warn: unreachable
| ^^^^
| Unreachable case
-- [E121] Pattern Match Warning: tests/warn/redundant-null.scala:30:7 --------------------------------------------------
30 | case _ => // warn
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
31 | case null => // warn: unreachable
| ^^^^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:32:7 -----------------------------------------
32 | case _ => // warn
32 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:33:7 -----------------------------------------
33 | case _ => // warn
33 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:37:7 -----------------------------------------
37 | case _ => println("unreachable") // warn
37 | case _ => // warn: unreachable
| ^
| Unreachable case
-- [E030] Match case Unreachable Warning: tests/warn/redundant-null.scala:41:7 -----------------------------------------
41 | case _ => // warn
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).
32 changes: 18 additions & 14 deletions tests/warn/redundant-null.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,35 +7,39 @@ val n = null
def f(s: A) = s match
case _: n.type =>
case _: A =>
case _: n.type => // warn
case _: n.type => // warn: unreachable
case null =>
case _ => // warn
case _ => // warn
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
case _ => 3 // warn: unreachable

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

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

def f5(x: String) = x match
case x => println("catch all")
case _ => println("unreachable") // warn
case x =>
case _ => // warn: unreachable

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

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

0 comments on commit d53a9ce

Please sign in to comment.