Skip to content

Fix #3144: emit warnings for unchecked type patterns #3156

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 9 commits into from
Sep 27, 2017
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
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ public enum ErrorMessageID {
MissingReturnTypeWithReturnStatementID,
NoReturnFromInlineID,
ReturnOutsideMethodDefinitionID,
UncheckedTypePatternID,
;

public int errorNumber() {
Expand Down
12 changes: 12 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,18 @@ object messages {
|"""
}

case class UncheckedTypePattern(msg: String)(implicit ctx: Context)
extends Message(UncheckedTypePatternID) {
val kind = "Pattern Match Exhaustivity"

val explanation =
hl"""|Type arguments and type refinements are erased during compile time, thus it's
|impossible to check them at run-time.
|
|You can either replace the type arguments by `_` or use `@unchecked`.
|"""
}

case class MatchCaseUnreachable()(implicit ctx: Context)
extends Message(MatchCaseUnreachableID) {
val kind = s"""Match ${hl"case"} Unreachable"""
Expand Down
36 changes: 25 additions & 11 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 Types._
import Contexts._
import Flags._
import ast.Trees._
import ast.{tpd, untpd}
import ast.tpd
import Decorators._
import Symbols._
import StdNames._
Expand Down Expand Up @@ -403,21 +403,37 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
else
Prod(pat.tpe.stripAnnots, fun.tpe.widen, fun.symbol, pats.map(project), irrefutable(fun))
case Typed(pat @ UnApply(_, _, _), _) => project(pat)
case Typed(expr, _) => Typ(erase(expr.tpe.stripAnnots), true)
case Typed(expr, tpt) =>
val unchecked = expr.tpe.hasAnnotation(ctx.definitions.UncheckedAnnot)
def warn(msg: String): Unit = if (!unchecked) ctx.warning(UncheckedTypePattern(msg), tpt.pos)
Typ(erase(expr.tpe.stripAnnots)(warn), true)
case _ =>
debug.println(s"unknown pattern: $pat")
Empty
}

/* Erase a type binding according to erasure semantics in pattern matching */
def erase(tp: Type): Type = tp match {
def erase(tp: Type)(implicit warn: String => Unit): Type = tp match {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing an implicit around, you could have an outer function which defines warn and an inner recursive function which uses it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarter thanks a lot, I've addressed other comments. For this one, doing the refactoring would require to pass an additional Position to erase, which will make the API a little ugly.

case tp @ AppliedType(tycon, args) =>
if (tycon.isRef(defn.ArrayClass)) tp.derivedAppliedType(tycon, args.map(erase))
else tp.derivedAppliedType(tycon, args.map(t => WildcardType))
else {
val ignoreWarning = args.forall { p =>
p.typeSymbol.is(BindDefinedType) ||
p.hasAnnotation(defn.UncheckedAnnot) ||
p.isInstanceOf[TypeBounds]
}
if (!ignoreWarning)
warn("type arguments are not checked since they are eliminated by erasure")

tp.derivedAppliedType(tycon, args.map(t => WildcardType))
}
case OrType(tp1, tp2) =>
OrType(erase(tp1), erase(tp2))
case AndType(tp1, tp2) =>
AndType(erase(tp1), erase(tp2))
case tp: RefinedType =>
warn("type refinement is not checked since it is eliminated by erasure")
tp.derivedRefinedType(erase(tp.parent), tp.refinedName, WildcardType)
case _ => tp
}

Expand Down Expand Up @@ -721,12 +737,10 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
}

def checkable(tree: Match): Boolean = {
def isCheckable(tp: Type): Boolean = tp match {
case AnnotatedType(tp, annot) =>
(ctx.definitions.UncheckedAnnot != annot.symbol) && isCheckable(tp)
case _ =>
// Possible to check everything, but be compatible with scalac by default
ctx.settings.YcheckAllPatmat.value ||
// Possible to check everything, but be compatible with scalac by default
def isCheckable(tp: Type): Boolean =
!tp.hasAnnotation(defn.UncheckedAnnot) && (
ctx.settings.YcheckAllPatmat.value ||
tp.typeSymbol.is(Sealed) ||
tp.isInstanceOf[OrType] ||
(tp.isInstanceOf[AndType] && {
Expand All @@ -737,7 +751,7 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
tp.typeSymbol.is(Enum) ||
canDecompose(tp) ||
(defn.isTupleType(tp) && tp.dealias.argInfos.exists(isCheckable(_)))
}
)

val Match(sel, cases) = tree
val res = isCheckable(sel.tpe.widen.dealiasKeepAnnots)
Expand Down
2 changes: 2 additions & 0 deletions tests/patmat/3144.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
2: Pattern Match Exhaustivity
7: Pattern Match Exhaustivity
9 changes: 9 additions & 0 deletions tests/patmat/3144.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
sealed trait Foo[T]
case class Bar[T](s: String)

object Test {
def shouldError[T](foo: Foo[T]): String =
foo match {
case bar: Bar[T] => bar.s
}
}
2 changes: 2 additions & 0 deletions tests/patmat/3144b.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
4: Pattern Match Exhaustivity
10: Pattern Match Exhaustivity
13 changes: 13 additions & 0 deletions tests/patmat/3144b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class Test {
def f(x: Any): Int = x match {
case xs: List[Int] @unchecked => xs.head
case xs: Array[List[Int]] => 3
case _ => 0
}

def g(x: Any): Int = x match {
case xs: List[Int @unchecked] => xs.head
case xs: Array[List[Int]] => 3
case _ => 0
}
}
1 change: 1 addition & 0 deletions tests/patmat/enum-HList.check
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
2: Pattern Match Exhaustivity
1 change: 1 addition & 0 deletions tests/patmat/enum-Tree.check
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
8: Pattern Match Exhaustivity
2 changes: 2 additions & 0 deletions tests/patmat/t10019.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
2: Pattern Match Exhaustivity: (List(_, _: _*), Nil), (List(_, _: _*), List(_, _, _: _*)), (Nil, List(_, _: _*)), (List(_, _, _: _*), List(_, _: _*))
11: Pattern Match Exhaustivity: (Foo(None), Foo(_))
14 changes: 14 additions & 0 deletions tests/patmat/t10019.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
object Bug {
def foo[T](t1: List[T], t2: List[T]) = (t1, t2) match {
case (Nil, Nil) => ()
case (List(_), List(_)) => ()
}
}

object Bug2 {
sealed case class Foo(e: Option[Int])

def loop(s: Foo, t: Foo): Nothing = (s,t) match {
case (Foo(Some(_)), _) => ???
}
}
1 change: 1 addition & 0 deletions tests/patmat/t3683.check
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7: Pattern Match Exhaustivity
1 change: 1 addition & 0 deletions tests/patmat/t3683a.check
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
8: Pattern Match Exhaustivity
14: Pattern Match Exhaustivity: XX()