Merge pull request scala#3781 from retronym/topic/8611

SI-8611 Avoid accidental patmat unification with refinement types

Author: retronym

src/compiler/scala/tools/nsc/transform/patmat/Logic.scala

@@ -505,7 +505,7 @@ trait ScalaLogic extends Interface with Logic with TreeAndTypeAnalysis {
     }
 
 
-    import global.{ConstantType, Constant, SingletonType, Literal, Ident, singleType}
+    import global.{ConstantType, Constant, EmptyScope, SingletonType, Literal, Ident, refinedType, singleType, TypeBounds, NoSymbol}
     import global.definitions._
 
 
@@ -538,23 +538,30 @@ trait ScalaLogic extends Interface with Logic with TreeAndTypeAnalysis {
       private val trees = mutable.HashSet.empty[Tree]
 
       // hashconsing trees (modulo value-equality)
-      private[TreesAndTypesDomain] def uniqueTpForTree(t: Tree): Type =
-        // a new type for every unstable symbol -- only stable value are uniqued
-        // technically, an unreachable value may change between cases
-        // thus, the failure of a case that matches on a mutable value does not exclude the next case succeeding
-        // (and thuuuuus, the latter case must be considered reachable)
-        if (!t.symbol.isStable) t.tpe.narrow
+      private[TreesAndTypesDomain] def uniqueTpForTree(t: Tree): Type = {
+        def freshExistentialSubtype(tp: Type): Type = {
+          // SI-8611 tp.narrow is tempting, but unsuitable. See `testRefinedTypeSI8611` for an explanation.
+          NoSymbol.freshExistential("").setInfo(TypeBounds.upper(tp)).tpe
+        }
+
+        if (!t.symbol.isStable) {
+          // Create a fresh type for each unstable value, since we can never correlate it to another value.
+          // For example `case X => case X =>` should not complaing about the second case being unreachable,
+          // if X is mutable.
+          freshExistentialSubtype(t.tpe)
+        }
         else trees find (a => a.correspondsStructure(t)(sameValue)) match {
           case Some(orig) =>
-            debug.patmat("unique tp for tree: "+ ((orig, orig.tpe)))
+            debug.patmat("unique tp for tree: " + ((orig, orig.tpe)))
             orig.tpe
           case _ =>
             // duplicate, don't mutate old tree (TODO: use a map tree -> type instead?)
-            val treeWithNarrowedType = t.duplicate setType t.tpe.narrow
+            val treeWithNarrowedType = t.duplicate setType freshExistentialSubtype(t.tpe)
             debug.patmat("uniqued: "+ ((t, t.tpe, treeWithNarrowedType.tpe)))
             trees += treeWithNarrowedType
             treeWithNarrowedType.tpe
         }
+      }
     }
 
     sealed abstract class Const {

test/files/run/t8611a.flags

@@ -0,0 +1 @@
+-Xfatal-warnings

test/files/run/t8611a.scala

@@ -0,0 +1,16 @@
+trait K
+trait L
+
+object O {
+  type LK = K with L
+  val A: LK = new K with L
+  val B: LK = new K with L
+}
+
+object Test extends App {
+  val scrut: O.LK = O.B
+  scrut match {
+    case O.A => ???
+    case O.B => // spurious unreachable
+  }
+}

test/files/run/t8611b.flags

@@ -0,0 +1 @@
+-Xfatal-warnings

test/files/run/t8611b.scala

@@ -0,0 +1,54 @@
+sealed trait KrafsDescription
+
+abstract class NotWorkingEnum extends Enumeration {
+
+  type ExtendedValue = Value with KrafsDescription
+
+  def Enum(inDescription: String): ExtendedValue = {
+    new Val(nextId) with KrafsDescription {
+    }
+  }
+}
+
+abstract class WorkingEnum extends Enumeration {
+
+  type ExtendedValue = Value
+
+  def Enum(inDescription: String): ExtendedValue = {
+    new Val(nextId) {
+    }
+  }
+}
+
+object NotWorkingTab extends NotWorkingEnum {
+  val a = Enum("A")
+  val b = Enum("B")
+}
+
+object WorkingTab extends WorkingEnum {
+  val a = Enum("A")
+  val b = Enum("B")
+}
+
+object Test extends App {
+  testGris()
+  testWorking()
+
+  def testGris() {
+    val pipp = NotWorkingTab.b
+    pipp match {
+      case NotWorkingTab.a => ???
+      case NotWorkingTab.b =>
+      case _ => ???
+    }
+  }
+
+  def testWorking() {
+    val stuff = WorkingTab.a
+    stuff match {
+      case WorkingTab.a =>
+      case WorkingTab.b => ???
+      case _ => ???
+    }
+  }
+}

test/files/run/t8611c.flags

@@ -0,0 +1 @@
+-Xfatal-warnings

test/files/run/t8611c.scala

@@ -0,0 +1,21 @@
+trait K
+trait L
+
+object O {
+  type LK = K with L
+}
+
+object Test extends App {
+  local
+
+  def local = {
+    val A: O.LK = new K with L
+    val B: O.LK = new K with L
+    val scrut: O.LK = A
+    scrut match {
+      case B if "".isEmpty => ???
+      case A =>
+      case B => ???
+    }
+  }
+}

test/junit/scala/reflect/internal/TypesTest.scala

@@ -0,0 +1,35 @@
+package scala.reflect.internal
+
+import org.junit.Assert._
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+import scala.tools.nsc.symtab.SymbolTableForUnitTesting
+
+@RunWith(classOf[JUnit4])
+class TypesTest {
+
+  object symbolTable extends SymbolTableForUnitTesting
+  import symbolTable._, definitions._
+
+  @Test
+  def testRefinedTypeSI8611(): Unit = {
+    def stringNarrowed = StringTpe.narrow
+    assert(stringNarrowed != stringNarrowed)
+    assert(!(stringNarrowed =:= stringNarrowed))
+
+    def boolWithString = refinedType(BooleanTpe :: StringTpe :: Nil, NoSymbol)
+    assert(boolWithString != boolWithString)
+    assert(boolWithString =:= boolWithString)
+
+    val boolWithString1 = boolWithString
+    val boolWithString1narrow1 = boolWithString1.narrow
+    val boolWithString1narrow2 = boolWithString1.narrow
+    // Two narrowings of the same refinement end up =:=. This was the root
+    // cause of SI-8611. See `narrowUniquely` in `Logic` for the workaround.
+    assert(boolWithString1narrow1 =:= boolWithString1narrow2)
+    val uniquelyNarrowed1 = refinedType(boolWithString1narrow1 :: Nil, NoSymbol)
+    val uniquelyNarrowed2 = refinedType(boolWithString1narrow2 :: Nil, NoSymbol)
+    assert(uniquelyNarrowed1 =:= uniquelyNarrowed2)
+  }
+}