Skip to content

Fix #5826: specialize union types to avoid constraint cut #5858

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 2 commits into from
Feb 7, 2019
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
24 changes: 17 additions & 7 deletions compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,17 @@ object TypeTestsCasts {
val tvars = constrained(typeLambda, untpd.EmptyTree, alwaysAddTypeVars = true)._2.map(_.tpe)
val P1 = tycon.appliedTo(tvars)

debug.println("P : " + P)
debug.println("P1 : " + P1)
debug.println("X : " + X)
debug.println("P : " + P.show)
debug.println("P1 : " + P1.show)
debug.println("X : " + X.show)

P1 <:< X // constraint P1

// use fromScala2x to avoid generating pattern bound symbols
maximizeType(P1, span, fromScala2x = true)

val res = P1 <:< P
debug.println("P1 : " + P1)
debug.println("P1 : " + P1.show)
debug.println("P1 <:< P = " + res)

res
Expand All @@ -122,9 +122,19 @@ object TypeTestsCasts {
case _ => recur(defn.AnyType, tpT)
}
case tpe: AppliedType =>
// first try withou striping type parameters for performance
isClassDetermined(X, tpe)(ctx.fresh.setNewTyperState()) ||
isClassDetermined(stripTypeParam(X), tpe)(ctx.fresh.setNewTyperState())
X.widen match {
case OrType(tp1, tp2) =>
// This case is required to retrofit type inference,
// which cut constraints in the following two cases:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm familiar with TypeComparer#either but I'm not sure I get what you are saying...

This case is required to match type inference, which simplifies constraints in the following two cases:

But if that's the reason, what about T1 & T2 <:< T3? Do we need to handle that somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

T1 & T2 <:< T3 is handled in the top-level match on P

// - T1 <:< T2 | T3
// - T1 & T2 <:< T3
// See TypeComparer#either
recur(tp1, P) && recur(tp2, P)
case _ =>
// first try withou striping type parameters for performance
isClassDetermined(X, tpe)(ctx.fresh.setNewTyperState()) ||
isClassDetermined(stripTypeParam(X), tpe)(ctx.fresh.setNewTyperState())
}
case AndType(tp1, tp2) => recur(X, tp1) && recur(X, tp2)
case OrType(tp1, tp2) => recur(X, tp1) && recur(X, tp2)
case AnnotatedType(t, _) => recur(X, t)
Expand Down
3 changes: 2 additions & 1 deletion tests/neg-custom-args/isInstanceOf/3324f.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class D[T]

class Test {
def foo[T](x: C[T]) = x match {
case _: D[T] => // error
case _: D[T] => // error
case _: C[Int] => // error
}
}
20 changes: 20 additions & 0 deletions tests/neg-custom-args/isInstanceOf/i5826.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
class Foo {
def test[A]: List[Int] | A => Int = {
case ls: List[Int] => ls.head // error
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also make sure to test that GADT matching doesn't infer Int:

case ls: List[_] => ls.head       // error

and that List[_] is otherwise still allowed:

case ls: List[_] => ls.length // no error here!

case _ => 0
}

def test2: List[Int] | List[String] => Int = {
case ls: List[Int] => ls.head // error
case _ => 0
}

trait A[T]
trait B[T]

// suppose: class C extends A[Int] with B[String]
def test3[X]: A[X] | B[X] => Int = {
case ls: A[X] => 4 // error
case _ => 0
}
}
11 changes: 11 additions & 0 deletions tests/neg-custom-args/isInstanceOf/i5826b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class Foo {
def test1[A]: List[Int] | A => Int = {
case ls: List[_] => ls.head // error
case _ => 0
}

def test2[A]: List[Int] | A => Int = {
case ls: List[_] => ls.size
case _ => 0
}
}