Make IsInstanceOfEvaluator more robust

odersky · odersky · commit 6fd397116012 · 2017-07-09T19:06:18.000+02:00
IsInstanceOfEvaluator as prone to mispredict whether
the test was generated by a pattern match or not. We fix
this by having a special type test symbol that represents
tests generated by a pattern match.
diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala
@@ -255,7 +255,7 @@ class Definitions {
       // generated by pattern matcher, eliminated by erasure
 
     def AnyMethods = List(Any_==, Any_!=, Any_equals, Any_hashCode,
-      Any_toString, Any_##, Any_getClass, Any_isInstanceOf, Any_asInstanceOf)
+      Any_toString, Any_##, Any_getClass, Any_isInstanceOf, Any_asInstanceOf, Any_typeTest)
 
   lazy val ObjectClass: ClassSymbol = {
     val cls = ctx.requiredClass("java.lang.Object")
diff --git a/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala b/compiler/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala
@@ -20,7 +20,7 @@ import TypeUtils._, TypeErasure._, Flags._
  *
  *  Steps taken:
  *
- *  1. `evalTypeApply` will establish the matrix and choose the appropriate
+ *  1. `evalTypeTest` will establish the matrix and choose the appropriate
  *     handling for the case:
  *      - Sel/sc is a value class or scrutinee is `Any`
  *      - `handleStaticallyKnown`
@@ -45,128 +45,128 @@ class IsInstanceOfEvaluator extends MiniPhaseTransform { thisTransformer =>
      *  the correct warnings, or an error if statically known to be false in
      *  match
      */
-    def handleStaticallyKnown(select: Select, scrutinee: Type, selector: Type, inMatch: Boolean, pos: Position): Tree = {
+    def handleStaticallyKnown(qualifier: Tree, scrutinee: Type, selector: Type, inMatch: Boolean, pos: Position): Tree = {
       val scrutineeSubSelector = scrutinee <:< selector
       if (!scrutineeSubSelector && inMatch) {
         ctx.error(
           s"this case is unreachable due to `${selector.show}` not being a subclass of `${scrutinee.show}`",
           Position(pos.start - 5, pos.end - 5)
         )
-        rewrite(select, to = false)
+        rewrite(qualifier, to = false)
       } else if (!scrutineeSubSelector && !inMatch) {
         ctx.warning(
           s"this will always yield false since `${scrutinee.show}` is not a subclass of `${selector.show}` (will be optimized away)",
           pos
         )
-        rewrite(select, to = false)
+        rewrite(qualifier, to = false)
       } else if (scrutineeSubSelector && !inMatch) {
         ctx.warning(
           s"this will always yield true if the scrutinee is non-null, since `${scrutinee.show}` is a subclass of `${selector.show}` (will be optimized away)",
           pos
         )
-        rewrite(select, to = true)
-      } else /* if (scrutineeSubSelector && inMatch) */ rewrite(select, to = true)
+        rewrite(qualifier, to = true)
+      } else /* if (scrutineeSubSelector && inMatch) */ rewrite(qualifier, to = true)
     }
 
     /** Rewrites cases with unrelated types */
-    def handleFalseUnrelated(select: Select, scrutinee: Type, selector: Type, inMatch: Boolean) =
+    def handleFalseUnrelated(qualifier: Tree, scrutinee: Type, selector: Type, inMatch: Boolean) =
       if (inMatch) {
         ctx.error(
           s"will never match since `${selector.show}` is not a subclass of `${scrutinee.show}`",
-          Position(select.pos.start - 5, select.pos.end - 5)
+          Position(qualifier.pos.start - 5, qualifier.pos.end - 5) // WHY 5?
         )
-        rewrite(select, to = false)
+        rewrite(qualifier, to = false)
       } else {
         ctx.warning(
           s"will always yield false since `${scrutinee.show}` is not a subclass of `${selector.show}`",
-          select.pos
+          tree.pos
         )
-        rewrite(select, to = false)
+        rewrite(qualifier, to = false)
       }
 
-    /** Rewrites the select to a boolean if `to` is false or if the qualifier
+    /** Rewrites the qualifier of a type test to a boolean if `to` is false or if the qualifier
      *  is a value class.
      *
      *  If `to` is set to true and the qualifier is not a primitive, the
      *  instanceOf is replaced by a null check, since:
      *
      *  `scrutinee.isInstanceOf[Selector]` if `scrutinee eq null`
      */
-    def rewrite(tree: Select, to: Boolean): Tree =
-      if (!to || !tree.qualifier.tpe.widen.derivesFrom(defn.AnyRefAlias)) {
+    def rewrite(qualifier: Tree, to: Boolean): Tree =
+      if (!to || !qualifier.tpe.widen.derivesFrom(defn.AnyRefAlias)) {
         val literal = Literal(Constant(to))
-        if (!isPureExpr(tree.qualifier)) Block(List(tree.qualifier), literal)
+        if (!isPureExpr(qualifier)) Block(List(qualifier), literal)
         else literal
       } else
-        Apply(tree.qualifier.select(defn.Object_ne), List(Literal(Constant(null))))
+        Apply(qualifier.select(defn.Object_ne), List(Literal(Constant(null))))
 
-    /** Attempts to rewrite TypeApply to either `scrutinee ne null` or a
-     *  constant
+    /** Attempts to rewrite type test to either `scrutinee ne null` or a
+     *  constant. Any_typeTest nodes have been rewritten to Any_isInstanceOf at this point.
+     *  @param   tree       the whole type test <qualifier>.asInstanceOf[T]
+     *  @param   qualifier  the <qualifier> part
+     *  @param   inMatch    tree was a type test generated by a pattern match.
      */
-    def evalTypeApply(tree: TypeApply): Tree =
-      if (tree.symbol != defn.Any_isInstanceOf) tree
-      else tree.fun match {
-        case s: Select => {
-          val scrutinee = erasure(s.qualifier.tpe.widen)
-          val selector  = erasure(tree.args.head.tpe.widen)
-
-          val scTrait = scrutinee.typeSymbol is Trait
-          val scClass =
-            scrutinee.typeSymbol.isClass &&
-            !(scrutinee.typeSymbol is Trait) &&
-            !(scrutinee.typeSymbol is Module)
-
-          val scClassNonFinal = scClass && !(scrutinee.typeSymbol is Final)
-          val scFinalClass    = scClass && (scrutinee.typeSymbol is Final)
-
-          val selTrait = selector.typeSymbol is Trait
-          val selClass =
-            selector.typeSymbol.isClass &&
-            !(selector.typeSymbol is Trait) &&
-            !(selector.typeSymbol is Module)
-
-          val selClassNonFinal = selClass && !(selector.typeSymbol is Final)
-          val selFinalClass    = selClass && (selector.typeSymbol is Final)
-
-          // Cases ---------------------------------
-          val valueClassesOrAny =
-            ValueClasses.isDerivedValueClass(scrutinee.typeSymbol) ||
-            ValueClasses.isDerivedValueClass(selector.typeSymbol)  ||
-            scrutinee == defn.ObjectType
-
-          val knownStatically = scFinalClass
-
-          val falseIfUnrelated =
-            (scClassNonFinal && selClassNonFinal) ||
-            (scClassNonFinal && selFinalClass)    ||
-            (scTrait && selFinalClass)
-
-          val happens =
-            (scClassNonFinal && selClassNonFinal) ||
-            (scTrait && selClassNonFinal)         ||
-            (scTrait && selTrait)
-
-          val inMatch = s.qualifier.symbol is Case
-            // FIXME: This will misclassify case objects! We need to find another way to characterize
-            // isInstanceOfs generated by matches.
-            // Probably the most robust way is to use another symbol for the isInstanceOf method.
-
-          if (valueClassesOrAny) tree
-          else if (knownStatically)
-            handleStaticallyKnown(s, scrutinee, selector, inMatch, tree.pos)
-          else if (falseIfUnrelated && scrutinee <:< selector)
-            // scrutinee is a subtype of the selector, safe to rewrite
-            rewrite(s, to = true)
-          else if (falseIfUnrelated && !(selector <:< scrutinee))
-            // selector and scrutinee are unrelated
-            handleFalseUnrelated(s, scrutinee, selector, inMatch)
-          else if (happens) tree
-          else tree
-        }
-
-        case _ => tree
-      }
+    def evalTypeTest(tree: TypeApply, qualifier: Tree, inMatch: Boolean) = {
+      val scrutinee = erasure(qualifier.tpe.widen)
+      val selector  = erasure(tree.args.head.tpe.widen)
+
+      val scTrait = scrutinee.typeSymbol is Trait
+      val scClass =
+        scrutinee.typeSymbol.isClass &&
+        !(scrutinee.typeSymbol is Trait) &&
+        !(scrutinee.typeSymbol is Module)
+
+      val scClassNonFinal = scClass && !(scrutinee.typeSymbol is Final)
+      val scFinalClass    = scClass && (scrutinee.typeSymbol is Final)
+
+      val selTrait = selector.typeSymbol is Trait
+      val selClass =
+        selector.typeSymbol.isClass &&
+        !(selector.typeSymbol is Trait) &&
+        !(selector.typeSymbol is Module)
+
+      val selClassNonFinal = selClass && !(selector.typeSymbol is Final)
+      val selFinalClass    = selClass && (selector.typeSymbol is Final)
+
+      // Cases ---------------------------------
+      val valueClassesOrAny =
+        ValueClasses.isDerivedValueClass(scrutinee.typeSymbol) ||
+        ValueClasses.isDerivedValueClass(selector.typeSymbol)  ||
+        scrutinee == defn.ObjectType
+
+      val knownStatically = scFinalClass
+
+      val falseIfUnrelated =
+        (scClassNonFinal && selClassNonFinal) ||
+        (scClassNonFinal && selFinalClass)    ||
+        (scTrait && selFinalClass)
+
+      val happens =
+        (scClassNonFinal && selClassNonFinal) ||
+        (scTrait && selClassNonFinal)         ||
+        (scTrait && selTrait)
+
+      if (valueClassesOrAny) tree
+      else if (knownStatically)
+        handleStaticallyKnown(qualifier, scrutinee, selector, inMatch, tree.pos)
+      else if (falseIfUnrelated && scrutinee <:< selector)
+        // scrutinee is a subtype of the selector, safe to rewrite
+        rewrite(qualifier, to = true)
+      else if (falseIfUnrelated && !(selector <:< scrutinee))
+        // selector and scrutinee are unrelated
+        handleFalseUnrelated(qualifier, scrutinee, selector, inMatch)
+      else if (happens) tree
+      else tree
+    }
 
-    evalTypeApply(tree)
+    tree.fun match {
+      case fn: Select if fn.symbol == defn.Any_typeTest =>
+        evalTypeTest(
+          cpy.TypeApply(tree)(fn.qualifier.select(defn.Any_isInstanceOf), tree.args),
+          fn.qualifier, inMatch = true)
+      case fn: Select if fn.symbol == defn.Any_isInstanceOf =>
+        evalTypeTest(tree, fn.qualifier, inMatch = false)
+      case _ => tree
+    }
   }
 }
diff --git a/compiler/src/dotty/tools/dotc/transform/PatMat.scala b/compiler/src/dotty/tools/dotc/transform/PatMat.scala
@@ -100,7 +100,7 @@ object PatMat {
 
     class TypeTest(scrut: Symbol, tpt: Tree, ons: Node, onf: Node) extends Test(scrut, ons, onf) {
       def pos = tpt.pos
-      def condition = scrutinee.isInstance(tpt.tpe)
+      def condition = scrutinee.select(defn.Any_typeTest).appliedToType(tpt.tpe)
       override def toString = i"TypeTest($scrutinee: $tpt)"
     }
 
@@ -232,16 +232,16 @@ object PatMat {
               letAbstract(get) { getResult =>
                 if (isUnapplySeq)
                   translateUnApplySeq(getResult, args)
-                 else {
-                    val selectors =
-                      if (args.tail.isEmpty) ref(getResult) :: Nil
-                      else productSelectors(get.tpe).map(ref(getResult).select(_))
-                    new UnApplyTest(unappResult, translateArgs(selectors, args, onSuccess), onFailure)
-                  }
+                else {
+                  val selectors =
+                    if (args.tail.isEmpty) ref(getResult) :: Nil
+                    else productSelectors(get.tpe).map(ref(getResult).select(_))
+                  new UnApplyTest(unappResult, translateArgs(selectors, args, onSuccess), onFailure)
                 }
               }
             }
           }
+        }
       }
 
       swapBind(tree) match {
@@ -371,7 +371,7 @@ object PatMat {
       transform(node)
     }
 
-    val seen = mutable.Set[Int]()
+    val emitted = mutable.Set[Int]()
 
     def emit(node: Node): Tree = {
       if (selfCheck) {
diff --git a/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala b/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala
@@ -12,7 +12,7 @@ import SymDenotations._, Symbols._, StdNames._, Annotations._, Trees._, Scopes._
 import util.Positions._
 import Decorators._
 import NameKinds.{ProtectedAccessorName, ProtectedSetterName, OuterSelectName, SuperAccessorName}
-import Symbols._, TypeUtils._
+import Symbols._, TypeUtils._, SymUtils._
 
 /** This class performs the following functions:
  *
@@ -137,15 +137,11 @@ class SuperAccessors(thisTransformer: DenotTransformer) {
     /** Disallow some super.XX calls targeting Any methods which would
      *  otherwise lead to either a compiler crash or runtime failure.
      */
-    private def isDisallowed(sym: Symbol)(implicit ctx: Context) = {
-      val d = defn
-      import d._
-      (sym eq Any_isInstanceOf) ||
-      (sym eq Any_asInstanceOf) ||
-      (sym eq Any_==) ||
-      (sym eq Any_!=) ||
-      (sym eq Any_##)
-    }
+    private def isDisallowed(sym: Symbol)(implicit ctx: Context) =
+      sym.isTypeTestOrCast ||
+      (sym eq defn.Any_==) ||
+      (sym eq defn.Any_!=) ||
+      (sym eq defn.Any_##)
 
     /** Replace `sel` (or `sel[targs]` if `targs` is nonempty) with a protected accessor
      *  call, if necessary.
diff --git a/compiler/src/dotty/tools/dotc/transform/SymUtils.scala b/compiler/src/dotty/tools/dotc/transform/SymUtils.scala
@@ -44,8 +44,11 @@ class SymUtils(val self: Symbol) extends AnyVal {
     else directlyInheritedTraits
   }
 
+  def isTypeTest(implicit ctx: Context): Boolean =
+    self == defn.Any_isInstanceOf || self == defn.Any_typeTest
+
   def isTypeTestOrCast(implicit ctx: Context): Boolean =
-    self == defn.Any_asInstanceOf || self == defn.Any_isInstanceOf
+    self == defn.Any_asInstanceOf || isTypeTest
 
   def isVolatile(implicit ctx: Context) = self.hasAnnotation(defn.VolatileAnnot)
 

Original file line number	Diff line number	Diff line change
`@@ -100,7 +100,7 @@ object PatMat {`
`100`	`100`
`101`	`101`	`class TypeTest(scrut: Symbol, tpt: Tree, ons: Node, onf: Node) extends Test(scrut, ons, onf) {`
`102`	`102`	`def pos = tpt.pos`
`103`		`- def condition = scrutinee.isInstance(tpt.tpe)`
	`103`	`+ def condition = scrutinee.select(defn.Any_typeTest).appliedToType(tpt.tpe)`
`104`	`104`	`override def toString = i"TypeTest($scrutinee: $tpt)"`
`105`	`105`	`}`
`106`	`106`
`@@ -232,16 +232,16 @@ object PatMat {`
`232`	`232`	`letAbstract(get) { getResult =>`
`233`	`233`	`if (isUnapplySeq)`
`234`	`234`	`translateUnApplySeq(getResult, args)`
`235`		`- else {`
`236`		`- val selectors =`
`237`		`- if (args.tail.isEmpty) ref(getResult) :: Nil`
`238`		`- else productSelectors(get.tpe).map(ref(getResult).select(_))`
`239`		`- new UnApplyTest(unappResult, translateArgs(selectors, args, onSuccess), onFailure)`
`240`		`- }`
	`235`	`+ else {`
	`236`	`+ val selectors =`
	`237`	`+ if (args.tail.isEmpty) ref(getResult) :: Nil`
	`238`	`+ else productSelectors(get.tpe).map(ref(getResult).select(_))`
	`239`	`+ new UnApplyTest(unappResult, translateArgs(selectors, args, onSuccess), onFailure)`
`241`	`240`	`}`
`242`	`241`	`}`
`243`	`242`	`}`
`244`	`243`	`}`
	`244`	`+ }`
`245`	`245`	`}`
`246`	`246`
`247`	`247`	`swapBind(tree) match {`
`@@ -371,7 +371,7 @@ object PatMat {`
`371`	`371`	`transform(node)`
`372`	`372`	`}`
`373`	`373`
`374`		`- val seen = mutable.Set[Int]()`
	`374`	`+ val emitted = mutable.Set[Int]()`
`375`	`375`
`376`	`376`	`def emit(node: Node): Tree = {`
`377`	`377`	`if (selfCheck) {`