From ab1c90af40eef228cd6ffd3071a9a57a19f7e4b4 Mon Sep 17 00:00:00 2001
From: Guillaume Martres <smarter@ubuntu.com>
Date: Tue, 26 May 2020 17:06:55 +0200
Subject: [PATCH] Simplify merging in lub/glb, avoid unnecessary constraints

In `mergeIfSuper`, to simplify `tp1 | tp2`, we first check if `tp2` can
be made a subtype of `tp1`. If so we could just return `tp1` but this
isn't what we did before this commit: at that point we checked if `tp1`
could be made a subtype of `tp2` and in that case prefered returning
`tp2` to `tp1`. I haven't been able to find a reason for this (the
comment says "keep existing type if possible" which I don't understand).

On the other hand, I've found cases where this logic broke type
inference because the second subtype check inferred extraneous
constraints (see added testcase). So this commit simply removes this
logic, it does the same for `mergeIfSub` which contains similar logic to
simplify `tp1 & tp2`, though this one is less problematic since it
always runs with frozen constraints.
---
 compiler/src/dotty/tools/dotc/core/TypeComparer.scala |  6 ++----
 tests/neg/i4564.scala                                 |  3 +--
 tests/pos/merge-constraint.scala                      | 11 +++++++++++
 3 files changed, 14 insertions(+), 6 deletions(-)
 create mode 100644 tests/pos/merge-constraint.scala

diff --git a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala
index 7ae72bf4317f..a689e2353737 100644
--- a/compiler/src/dotty/tools/dotc/core/TypeComparer.scala
+++ b/compiler/src/dotty/tools/dotc/core/TypeComparer.scala
@@ -1983,8 +1983,7 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w
   /** Merge `t1` into `tp2` if t1 is a subtype of some &-summand of tp2.
    */
   private def mergeIfSub(tp1: Type, tp2: Type): Type =
-    if (isSubTypeWhenFrozen(tp1, tp2))
-      if (isSubTypeWhenFrozen(tp2, tp1)) tp2 else tp1 // keep existing type if possible
+    if (isSubTypeWhenFrozen(tp1, tp2)) tp1
     else tp2 match {
       case tp2 @ AndType(tp21, tp22) =>
         val lower1 = mergeIfSub(tp1, tp21)
@@ -2004,8 +2003,7 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w
    *  @param canConstrain  If true, new constraints might be added to make the merge possible.
    */
   private def mergeIfSuper(tp1: Type, tp2: Type, canConstrain: Boolean): Type =
-    if (isSubType(tp2, tp1, whenFrozen = !canConstrain))
-      if (isSubType(tp1, tp2, whenFrozen = !canConstrain)) tp2 else tp1 // keep existing type if possible
+    if (isSubType(tp2, tp1, whenFrozen = !canConstrain)) tp1
     else tp2 match {
       case tp2 @ OrType(tp21, tp22) =>
         val higher1 = mergeIfSuper(tp1, tp21, canConstrain)
diff --git a/tests/neg/i4564.scala b/tests/neg/i4564.scala
index 327baf0d35d8..cbe57b1ad03f 100644
--- a/tests/neg/i4564.scala
+++ b/tests/neg/i4564.scala
@@ -42,5 +42,4 @@ class BaseCNSP[T] {
 }
 
 object ClashNoSigPoly extends BaseCNSP[Int]
-// TODO: improve error message
-case class ClashNoSigPoly private(x: Int)  // error: found: ClashNoSigPoly required: Nothing
\ No newline at end of file
+case class ClashNoSigPoly private(x: Int)
diff --git a/tests/pos/merge-constraint.scala b/tests/pos/merge-constraint.scala
new file mode 100644
index 000000000000..7434be81e53f
--- /dev/null
+++ b/tests/pos/merge-constraint.scala
@@ -0,0 +1,11 @@
+class Hi
+class Lo extends Hi
+
+object Test {
+  def foo[T, U <: T](t: T, f: T => U): U = ???
+
+  def test(hi: Hi, lo: Lo): Unit = {
+    val ret = foo(hi, x => lo) // This used to infer U := Hi
+    val y: Lo = ret
+  }
+}