Skip to content

Fix #12530: Wildcard of value types don't cover null #12533

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

Merged
merged 4 commits into from
May 23, 2021
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
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ import transform.SymUtils._

class MatchCaseOnlyNullWarning()(using Context)
extends PatternMatchMsg(MatchCaseOnlyNullWarningID) {
def msg = em"""Only ${hl("null")} is matched. Consider using ${hl("case null =>")} instead."""
def msg = em"""Unreachable case except for ${hl("null")} (if this is intentional, consider writing ${hl("case null =>")} instead)."""
def explain = ""
}

Expand Down
90 changes: 43 additions & 47 deletions compiler/src/dotty/tools/dotc/transform/patmat/Space.scala
Original file line number Diff line number Diff line change
Expand Up @@ -320,21 +320,13 @@ class SpaceEngine(using Context) extends SpaceLogic {
private val scalaConsType = defn.ConsClass.typeRef

private val constantNullType = ConstantType(Constant(null))
private val constantNullSpace = Typ(constantNullType)

/** Does the given tree stand for the literal `null`? */
def isNullLit(tree: Tree): Boolean = tree match {
case Literal(Constant(null)) => true
case _ => false
}

/** Does the given space contain just the value `null`? */
def isNullSpace(space: Space): Boolean = space match {
case Typ(tpe, _) => tpe.dealias == constantNullType || tpe.isNullType
case Or(spaces) => spaces.forall(isNullSpace)
case _ => false
}

override def intersectUnrelatedAtomicTypes(tp1: Type, tp2: Type): Space = trace(s"atomic intersection: ${AndType(tp1, tp2).show}", debug) {
// Precondition: !isSubType(tp1, tp2) && !isSubType(tp2, tp1).
if (!ctx.explicitNulls && (tp1.isNullType || tp2.isNullType)) {
Expand All @@ -355,18 +347,18 @@ class SpaceEngine(using Context) extends SpaceLogic {
def project(pat: Tree): Space = pat match {
case Literal(c) =>
if (c.value.isInstanceOf[Symbol])
Typ(c.value.asInstanceOf[Symbol].termRef, false)
Typ(c.value.asInstanceOf[Symbol].termRef, decomposed = false)
else
Typ(ConstantType(c), false)
Typ(ConstantType(c), decomposed = false)

case pat: Ident if isBackquoted(pat) =>
Typ(pat.tpe, false)
Typ(pat.tpe, decomposed = false)

case Ident(nme.WILDCARD) =>
Or(Typ(erase(pat.tpe.stripAnnots), false) :: constantNullSpace :: Nil)
Typ(erase(pat.tpe.stripAnnots, isValue = true), decomposed = false)

case Ident(_) | Select(_, _) =>
Typ(erase(pat.tpe.stripAnnots), false)
Typ(erase(pat.tpe.stripAnnots, isValue = true), decomposed = false)

case Alternative(trees) =>
Or(trees.map(project(_)))
Expand All @@ -386,31 +378,31 @@ class SpaceEngine(using Context) extends SpaceLogic {
else {
val (arity, elemTp, resultTp) = unapplySeqInfo(fun.tpe.widen.finalResultType, fun.srcPos)
if (elemTp.exists)
Prod(erase(pat.tpe.stripAnnots), funRef, projectSeq(pats) :: Nil)
Prod(erase(pat.tpe.stripAnnots, isValue = false), funRef, projectSeq(pats) :: Nil)
else
Prod(erase(pat.tpe.stripAnnots), funRef, pats.take(arity - 1).map(project) :+ projectSeq(pats.drop(arity - 1)))
Prod(erase(pat.tpe.stripAnnots, isValue = false), funRef, pats.take(arity - 1).map(project) :+ projectSeq(pats.drop(arity - 1)))
}
else
Prod(erase(pat.tpe.stripAnnots), funRef, pats.map(project))
Prod(erase(pat.tpe.stripAnnots, isValue = false), funRef, pats.map(project))

case Typed(pat @ UnApply(_, _, _), _) =>
project(pat)

case Typed(_, tpt) =>
Typ(erase(tpt.tpe.stripAnnots), true)
Typ(erase(tpt.tpe.stripAnnots, isValue = true), decomposed = false)

case This(_) =>
Typ(pat.tpe.stripAnnots, false)
Typ(pat.tpe.stripAnnots, decomposed = false)

case EmptyTree => // default rethrow clause of try/catch, check tests/patmat/try2.scala
Typ(WildcardType, false)
Typ(WildcardType, decomposed = false)

case Block(Nil, expr) =>
project(expr)

case _ =>
// Pattern is an arbitrary expression; assume a skolem (i.e. an unknown value) of the pattern type
Typ(pat.tpe.narrow, false)
Typ(pat.tpe.narrow, decomposed = false)
}

private def project(tp: Type): Space = tp match {
Expand Down Expand Up @@ -458,29 +450,37 @@ class SpaceEngine(using Context) extends SpaceLogic {
* case (IntExpr(_), IntExpr(_)) =>
* case (BooleanExpr(_), BooleanExpr(_)) =>
* }
*
* @param inArray whether `tp` is a type argument to `Array`
* @param isValue whether `tp` is the type which match against values
*
* If `isValue` is true, then pattern-bound symbols are erased to its upper bound.
* This is needed to avoid spurious unreachable warnings. See tests/patmat/i6197.scala.
*/
private def erase(tp: Type, inArray: Boolean = false): Type = trace(i"$tp erased to", debug) {
private def erase(tp: Type, inArray: Boolean = false, isValue: Boolean = false): Type = trace(i"$tp erased to", debug) {

tp match {
case tp @ AppliedType(tycon, args) =>
if tycon.typeSymbol.isPatternBound then return WildcardType

val args2 =
if (tycon.isRef(defn.ArrayClass)) args.map(arg => erase(arg, inArray = true))
else args.map(arg => erase(arg, inArray = false))
tp.derivedAppliedType(erase(tycon, inArray), args2)
if (tycon.isRef(defn.ArrayClass)) args.map(arg => erase(arg, inArray = true, isValue = false))
else args.map(arg => erase(arg, inArray = false, isValue = false))
tp.derivedAppliedType(erase(tycon, inArray, isValue = false), args2)

case tp @ OrType(tp1, tp2) =>
OrType(erase(tp1, inArray), erase(tp2, inArray), tp.isSoft)
OrType(erase(tp1, inArray, isValue), erase(tp2, inArray, isValue), tp.isSoft)

case AndType(tp1, tp2) =>
AndType(erase(tp1, inArray), erase(tp2, inArray))
AndType(erase(tp1, inArray, isValue), erase(tp2, inArray, isValue))

case tp @ RefinedType(parent, _, _) =>
erase(parent)
erase(parent, inArray, isValue)

case tref: TypeRef if tref.symbol.isPatternBound =>
if (inArray) tref.underlying else WildcardType
if inArray then tref.underlying
else if isValue then tref.superType
else WildcardType

case _ => tp
}
Expand Down Expand Up @@ -864,25 +864,28 @@ class SpaceEngine(using Context) extends SpaceLogic {
&& !sel.tpe.widen.isRef(defn.QuotedTypeClass)

def checkRedundancy(_match: Match): Unit = {
debug.println(s"---------------checking redundant patterns ${_match.show}")

val Match(sel, cases) = _match
val selTyp = sel.tpe.widen.dealias

if (!redundancyCheckable(sel)) return

val targetSpace =
if (ctx.explicitNulls || selTyp.classSymbol.isPrimitiveValueClass)
if !selTyp.classSymbol.isNullableClass then
project(selTyp)
else
project(OrType(selTyp, constantNullType, soft = false))

// in redundancy check, take guard as false in order to soundly approximate
def projectPrevCases(cases: List[CaseDef]): List[Space] =
cases.map { x =>
val spaces = cases.map { x =>
val res =
if (x.guard.isEmpty) project(x.pat)
else Empty
}

val spaces = projectPrevCases(cases)
debug.println(s"${x.pat.show} ====> ${res}")
res
}

(1 until cases.length).foreach { i =>
val pat = cases(i).pat
Expand All @@ -902,20 +905,13 @@ class SpaceEngine(using Context) extends SpaceLogic {
if (covered == Empty && !isNullLit(pat)) covered = curr

if (isSubspace(covered, prevs)) {
report.warning(MatchCaseUnreachable(), pat.srcPos)
}

// if last case is `_` and only matches `null`, produce a warning
// If explicit nulls are enabled, this check isn't needed because most of the cases
// that would trigger it would also trigger unreachability warnings.
if (!ctx.explicitNulls && i == cases.length - 1 && !isNullLit(pat) ) {
val spaces = flatten(simplify(minus(covered, prevs)))
if spaces.lengthCompare(10) < 0 then
dedup(spaces).toList match
case Typ(`constantNullType`, _) :: Nil =>
report.warning(MatchCaseOnlyNullWarning(), pat.srcPos)
case s =>
debug.println("`_` matches = " + s)
if i == cases.length - 1
&& isWildcardArg(pat)
&& pat.tpe.classSymbol.isNullableClass
then
report.warning(MatchCaseOnlyNullWarning(), pat.srcPos)
else
report.warning(MatchCaseUnreachable(), pat.srcPos)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1218,13 +1218,6 @@ object SourceCode {
this += "]"
printType(tpe.resType)

case tpe: TypeLambda =>
this += "["
printList(tpe.paramNames.zip(tpe.paramBounds), ", ",
(x: (String, TypeBounds)) => (this += x._1 += " ").printType(x._2))
this += "] => "
printType(tpe.resType)

case tpe@TypeBounds(lo, hi) =>
this += "_ >: "
printType(lo)
Expand Down
2 changes: 2 additions & 0 deletions tests/patmat/i12530.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
6: Match case Unreachable
14: Match case Unreachable
16 changes: 16 additions & 0 deletions tests/patmat/i12530.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
object Test {
def foo(a: Boolean, b: Boolean): Unit =
(a, b) match {
case (false, _) =>
case (true, _) =>
case (_, false) => // error: unreachable
}

def bar(a: Option[Boolean], b: Boolean): Unit =
(a, b) match {
case (Some(false), _) =>
case (Some(true), _) =>
case (None, _) =>
case (_, false) => // reachable: null
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now issue a warning here (see the check file).

}
}
3 changes: 1 addition & 2 deletions tests/patmat/null.check
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
6: Match case Unreachable
6: Pattern Match
13: Pattern Match
18: Match case Unreachable
20: Pattern Match