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.

Sign up for GitHub

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

Fix #4314: revert changes in #4299 #4333

Merged
merged 5 commits into from
Apr 20, 2018
+78 −38
Merged

Fix #4314: revert changes in #4299 #4333

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
84cf261
Fix #4314: revert changes in #4299
liufengyun Apr 17, 2018
54a7df8
fix test case t2425.scala
liufengyun Apr 17, 2018
f9700b1
catch two more exhaustive issue in compiler
liufengyun Apr 17, 2018
d8139fc
address review
liufengyun Apr 19, 2018
0dc0502
fix CI
liufengyun Apr 19, 2018
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
89 changes: 55 additions & 34 deletions compiler/src/dotty/tools/dotc/transform/patmat/Space.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ trait SpaceLogic {
case Prod(tp, fun, sym, spaces, full) =>
val sp = Prod(tp, fun, sym, spaces.map(simplify(_)), full)
if (sp.params.contains(Empty)) Empty
else if (canDecompose(tp) && decompose(tp).isEmpty) Empty
else sp
case Or(spaces) =>
val set = spaces.map(simplify(_)).flatMap {
Expand Down Expand Up @@ -349,18 +350,22 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
Empty
}

/* Erase a type binding according to erasure semantics in pattern matching */
def erase(tp: Type): Type = tp match {
case tp @ AppliedType(tycon, args) =>
if (tycon.isRef(defn.ArrayClass)) tp.derivedAppliedType(tycon, args.map(erase))
else 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(parent, refinedName, _) if refinedName.isTermName => // see pos/dependent-extractors.scala
tp.derivedRefinedType(erase(parent), refinedName, WildcardType)
case _ => tp
/* Erase pattern bound types with WildcardType */
def erase(tp: Type) = {
def isPatternTypeSymbol(sym: Symbol) = !sym.isClass && sym.is(Case)

val map = new TypeMap {
def apply(tp: Type) = tp match {
case tp @ AppliedType(tycon, args) if tycon.isRef(defn.ArrayClass) =>
// walkaround `Array[_] <:< Array[Array[_]]`, see tests/patmat/t2425.scala
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this comment is true, the following code doesn't compile:

def foo(x: Array[_]): Array[Array[_]] = x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underscore above expands to TypeBounds Array[<: Any :> Nothing] instead of Wildcards.

Here is the subtyping log:

  ==> Array[(?)] <:< Array[Array[(?)]]
    ==> scala.type <:< scala.type
    <== scala.type <:< scala.type   = true
    ==> Array[(?)] <:< (?)
      ==> Any <:< Any LoApprox
      <== Any <:< Any LoApprox  = true
    <== Array[(?)] <:< (?)   = true
    ==> (?) <:< Array[(?)]
    <== (?) <:< Array[(?)]   = true
  <== Array[(?)] <:< Array[Array[(?)]]   = true

And the behavior can be confirmed from the code in TypeComparer.

But thanks for raising the question, as by using TypeBounds instead WildcardType, we avoid the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, WildcardType(lo, hi) <:< X is equivalent to lo <:< X, see TypeComparer

Copy link
Contributor

Choose a reason for hiding this comment

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

s/walkaround/work around/

tp.derivedAppliedType(tycon, args.map(mapOver))
case tref: TypeRef if isPatternTypeSymbol(tref.typeSymbol) =>
WildcardType(tref.underlying.bounds)
case _ => mapOver(tp)
}
}

map(tp)
}

/** Space of the pattern: unapplySeq(a, b, c: _*)
Expand All @@ -384,7 +389,7 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
/** Is `tp1` a subtype of `tp2`? */
def isSubType(tp1: Type, tp2: Type): Boolean = {
val res = (tp1 != nullType || tp2 == nullType) && tp1 <:< tp2
debug.println(s"${tp1.show} <:< ${tp2.show} = $res")
debug.println(s"${tp1} <:< ${tp2} = $res")
res
}

Expand Down Expand Up @@ -576,8 +581,8 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
noClassConflict &&
(!isSingleton(tp1) || tp1 <:< tp2) &&
(!isSingleton(tp2) || tp2 <:< tp1) &&
(!bases1.exists(_ is Final) || tp1 <:< tp2) &&
(!bases2.exists(_ is Final) || tp2 <:< tp1)
(!bases1.exists(_ is Final) || tp1 <:< maxTypeMap.apply(tp2)) &&
(!bases2.exists(_ is Final) || tp2 <:< maxTypeMap.apply(tp1))
}
case OrType(tp1, tp2) =>
recur(tp1) || recur(tp2)
Expand All @@ -596,6 +601,41 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
res
}

/** expose abstract type references to their bounds or tvars according to variance */
private class AbstractTypeMap(maximize: Boolean)(implicit ctx: Context) extends TypeMap {
def expose(lo: Type, hi: Type): Type =
if (variance == 0)
newTypeVar(TypeBounds(lo, hi))
else if (variance == 1)
if (maximize) hi else lo
else
if (maximize) lo else hi

def apply(tp: Type): Type = tp match {
case tp: TypeRef if tp.underlying.isInstanceOf[TypeBounds] =>
val lo = this(tp.info.loBound)
val hi = this(tp.info.hiBound)
// See tests/patmat/gadt.scala tests/patmat/exhausting.scala tests/patmat/t9657.scala
val exposed = expose(lo, hi)
debug.println(s"$tp exposed to =====> $exposed")
exposed

case AppliedType(tycon: TypeRef, args) if tycon.underlying.isInstanceOf[TypeBounds] =>
val args2 = args.map(this)
val lo = this(tycon.info.loBound).applyIfParameterized(args2)
val hi = this(tycon.info.hiBound).applyIfParameterized(args2)
val exposed = expose(lo, hi)
debug.println(s"$tp exposed to =====> $exposed")
exposed

case _ =>
mapOver(tp)
}
}

private def minTypeMap(implicit ctx: Context) = new AbstractTypeMap(maximize = false)
private def maxTypeMap(implicit ctx: Context) = new AbstractTypeMap(maximize = true)

/** Instantiate type `tp1` to be a subtype of `tp2`
*
* Return the instantiated type if type parameters and this type
Expand All @@ -605,25 +645,6 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
*
*/
def instantiate(tp1: NamedType, tp2: Type)(implicit ctx: Context): Type = {
// expose type param references to their bounds according to variance
class AbstractTypeMap(maximize: Boolean)(implicit ctx: Context) extends ApproximatingTypeMap {
variance = if (maximize) 1 else -1

def apply(tp: Type): Type = tp match {
case tp: TypeRef if tp.underlying.isInstanceOf[TypeBounds] =>
val lo = this(tp.info.loBound)
val hi = this(tp.info.hiBound)
// See tests/patmat/gadt.scala tests/patmat/exhausting.scala tests/patmat/t9657.scala
range(lo, hi)

case _ =>
mapOver(tp)
}
}

def minTypeMap(implicit ctx: Context) = new AbstractTypeMap(maximize = false)
def maxTypeMap(implicit ctx: Context) = new AbstractTypeMap(maximize = true)

// Fix subtype checking for child instantiation,
// such that `Foo(Test.this.foo) <:< Foo(Foo.this)`
// See tests/patmat/i3938.scala
Expand Down
10 changes: 10 additions & 0 deletions tests/patmat/i4314.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
sealed trait Foo[A]
case class One[A]() extends Foo[A]
sealed abstract case class Bar[A]() extends Foo[A]
class Two() extends Bar[String]

object Test {
def test(x: Foo[Int]) = x match {
case One() =>
}
}
1 change: 1 addition & 0 deletions tests/patmat/i4314b.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
9: Match case Unreachable
11 changes: 11 additions & 0 deletions tests/patmat/i4314b.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
sealed trait Foo[A]
case class One[A]() extends Foo[A]
sealed abstract case class Bar[A]() extends Foo[A]
class Two() extends Bar[String]

object Test {
def test(x: Foo[Int]) = x match {
case One() =>
case Bar() =>
}
}