Fix #1754: Don't narrow GADTs to lower bounds

neg/i1754.scala succeeded because a GADT bound for `A` in `A <: B`
was narrowed to the lower bound of `B` (which was nothing) instead of
`B` itself. Fixing this uncovered several other problems that were
hidden by the overly aggressive narrowing "feature".

Author: odersky

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

@@ -8,7 +8,7 @@ import StdNames.{nme, tpnme}
 import collection.mutable
 import util.{Stats, DotClass}
 import config.Config
-import config.Printers.{typr, constr, subtyping, noPrinter}
+import config.Printers.{typr, constr, subtyping, gadts, noPrinter}
 import TypeErasure.{erasedLub, erasedGlb}
 import TypeApplications._
 import scala.util.control.NonFatal
@@ -383,9 +383,9 @@ class TypeComparer(initctx: Context) extends DotClass with ConstraintHandling {
             narrowGADTBounds(tp2, tp1, isUpper = false)) &&
           GADTusage(tp2.symbol)
       }
-      ((frozenConstraint || !isCappable(tp1)) && isSubType(tp1, lo2) ||
-        compareGADT ||
-        fourthTry(tp1, tp2))
+      val tryLowerFirst = frozenConstraint || !isCappable(tp1)
+      if (tryLowerFirst) isSubType(tp1, lo2) || compareGADT || fourthTry(tp1, tp2)
+      else compareGADT || fourthTry(tp1, tp2) || isSubType(tp1, lo2)
 
     case _ =>
       val cls2 = tp2.symbol
@@ -1076,13 +1076,15 @@ class TypeComparer(initctx: Context) extends DotClass with ConstraintHandling {
     case _ => proto.isMatchedBy(tp)
   }
 
-  /** Can type `tp` be constrained from above by adding a constraint to
-   *  a typevar that it refers to? In that case we have to be careful not
-   *  to approximate with the lower bound of a type in `thirdTry`. Instead,
-   *  we should first unroll `tp1` until we hit the type variable and bind the
-   *  type variable with (the corresponding type in) `tp2` instead.
+  /** Can type `tp` be constrained from above, either by adding a constraint to
+   *  a typevar that it refers to, or by narrowing a GADT bound? In that case we have
+   *  to be careful not to approximate with the lower bound of a type in `thirdTry`.
+   *  Instead, we should first unroll `tp1` until we hit the type variable and bind the
+   *  type variable with (the corresponding type in) `tp2` instead. Or, in the
+   *  case of a GADT bounded typeref, we should narrow with `tp2` instead of its lower bound.
    */
   private def isCappable(tp: Type): Boolean = tp match {
+    case tp: TypeRef => ctx.gadt.bounds.contains(tp.symbol)
     case tp: TypeParamRef => constraint contains tp
     case tp: TypeProxy => isCappable(tp.underlying)
     case tp: AndOrType => isCappable(tp.tp1) || isCappable(tp.tp2)
@@ -1096,7 +1098,7 @@ class TypeComparer(initctx: Context) extends DotClass with ConstraintHandling {
   private def narrowGADTBounds(tr: NamedType, bound: Type, isUpper: Boolean): Boolean =
     ctx.mode.is(Mode.GADTflexible) && !frozenConstraint && {
       val tparam = tr.symbol
-      typr.println(i"narrow gadt bound of $tparam: ${tparam.info} from ${if (isUpper) "above" else "below"} to $bound ${bound.isRef(tparam)}")
+      gadts.println(i"narrow gadt bound of $tparam: ${tparam.info} from ${if (isUpper) "above" else "below"} to $bound ${bound.toString} ${bound.isRef(tparam)}")
       if (bound.isRef(tparam)) false
       else {
         val oldBounds = ctx.gadt.bounds(tparam)

compiler/test/dotty/tools/dotc/CompilationTests.scala

@@ -175,6 +175,7 @@ class CompilationTests extends ParallelTesting {
     compileFilesInDir("../tests/neg", defaultOptions) +
     compileFilesInDir("../tests/neg-tailcall", defaultOptions) +
     compileFilesInDir("../tests/neg-no-optimise", defaultOptions) +
+    compileFile("../tests/neg-custom-args/i1754.scala", allowDeepSubtypes) +
     compileFile("../tests/neg-custom-args/i3246.scala", scala2Mode) +
     compileFile("../tests/neg-custom-args/i3627.scala", allowDeepSubtypes) +
     compileFile("../tests/neg-custom-args/typers.scala", allowDoubleBindings) +

tests/neg-custom-args/i1754.scala

@@ -0,0 +1,9 @@
+case class One[T](fst: T)
+
+object Test {
+  def bad[T](e: One[T]) = e match {
+    case foo: One[a] =>
+      val t: T = e.fst
+      val nok: Nothing = t // error
+  }
+}

tests/neg/boundspropagation.scala

@@ -33,7 +33,7 @@ object test4 {
     class Tree[-S, -T >: Option[S]]
 
     def g(x: Any): Tree[_, _ <: Option[N]] = x match {
-      case y: Tree[_, _] => y                         // works now (because of capture conversion?)
+      case y: Tree[_, _] => y                         // error -- used to work (because of capture conversion?)
     }
   }
 }

tests/pos/boundspropagation.scala

@@ -17,12 +17,15 @@ object test1 {
     }
   }
 }
+
+/** Does not work:
 object test2 {
   class Tree[S, T <: S]
 
   class Base {
     def g(x: Any): Tree[_, _ <: Int] = x match {
-      case y: Tree[Int @unchecked, _] => y
+      case y: Tree[Int @unchecked, t] => y
     }
   }
 }
+*/

tests/pos/i1754.scala

@@ -0,0 +1,24 @@
+object Test {
+  import java.util.{ concurrent => juc }
+  import scala.collection.concurrent
+  import scala.collection.convert.Wrappers._
+
+  /**
+   * Implicitly converts a Java ConcurrentMap to a Scala mutable ConcurrentMap.
+   * The returned Scala ConcurrentMap is backed by the provided Java
+   * ConcurrentMap and any side-effects of using it via the Scala interface will
+   * be visible via the Java interface and vice versa.
+   *
+   * If the Java ConcurrentMap was previously obtained from an implicit or
+   * explicit call of `asConcurrentMap(scala.collection.mutable.ConcurrentMap)`
+   * then the original Scala ConcurrentMap will be returned.
+   *
+   * @param m The ConcurrentMap to be converted.
+   * @return A Scala mutable ConcurrentMap view of the argument.
+   */
+  implicit def mapAsScalaConcurrentMap[A, B](m: juc.ConcurrentMap[A, B]): concurrent.Map[A, B] = m match {
+    case null                             => null
+    case cmw: ConcurrentMapWrapper[_, _]  => cmw.underlying
+    case _                                => new JConcurrentMapWrapper(m)
+  }
+}