Merge pull request #4974 from szeiger/wip/patmat-outertest

More conservative optimization for unnecessary outer ref checks

Author: adriaanm

spec/08-pattern-matching.md

@@ -328,10 +328,12 @@ A type pattern $T$ is of one of the following  forms:
 
 * A reference to a class $C$, $p.C$, or `$T$#$C$`.  This
   type pattern matches any non-null instance of the given class.
-  Note that the prefix of the class, if it is given, is relevant for determining
+  Note that the prefix of the class, if it exists, is relevant for determining
   class instances. For instance, the pattern $p.C$ matches only
   instances of classes $C$ which were created with the path $p$ as
-  prefix.
+  prefix. This also applies to prefixes which are not given syntactically.
+  For example, if $C$ refers to a class defined in the nearest enclosing
+  class and is thus equivalent to $this.C$, it is considered to have a prefix.
 
   The bottom types `scala.Nothing` and `scala.Null` cannot
   be used as type patterns, because they would match nothing in any case.

src/compiler/scala/tools/nsc/transform/ExplicitOuter.scala

@@ -486,7 +486,8 @@ abstract class ExplicitOuter extends InfoTransform
               // since we can't fix SI-4440 properly (we must drop the outer accessors of final classes when there's no immediate reference to them in sight)
               // at least don't crash... this duplicates maybeOmittable from constructors
               (acc.owner.isEffectivelyFinal && !acc.isOverridingSymbol)) {
-            currentRun.reporting.uncheckedWarning(tree.pos, "The outer reference in this type test cannot be checked at run time.")
+            if (!base.tpe.hasAnnotation(UncheckedClass))
+              currentRun.reporting.uncheckedWarning(tree.pos, "The outer reference in this type test cannot be checked at run time.")
             transform(TRUE) // urgh... drop condition if there's no accessor (or if it may disappear after constructors)
           } else {
             // println("(base, acc)= "+(base, acc))

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

@@ -351,7 +351,7 @@ trait MatchApproximation extends TreeAndTypeAnalysis with ScalaLogic with MatchT
               object condStrategy extends TypeTestTreeMaker.TypeTestCondStrategy {
                 type Result                                           = Prop
                 def and(a: Result, b: Result)                         = And(a, b)
-                def outerTest(testedBinder: Symbol, expectedTp: Type) = True // TODO OuterEqProp(testedBinder, expectedType)
+                def withOuterTest(testedBinder: Symbol, expectedTp: Type) = True // TODO OuterEqProp(testedBinder, expectedType)
                 def typeTest(b: Symbol, pt: Type) = { // a type test implies the tested path is non-null (null.isInstanceOf[T] is false for all T)
                   val p = binderToUniqueTree(b);                        And(uniqueNonNullProp(p), uniqueTypeProp(p, uniqueTp(pt)))
                 }

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

@@ -316,7 +316,7 @@ trait MatchTreeMaking extends MatchCodeGen with Debugging {
       trait TypeTestCondStrategy {
         type Result
 
-        def outerTest(testedBinder: Symbol, expectedTp: Type): Result
+        def withOuterTest(orig: Result)(testedBinder: Symbol, expectedTp: Type): Result = orig
         // TODO: can probably always widen
         def typeTest(testedBinder: Symbol, expectedTp: Type): Result
         def nonNullTest(testedBinder: Symbol): Result
@@ -336,18 +336,34 @@ trait MatchTreeMaking extends MatchCodeGen with Debugging {
         def equalsTest(pat: Tree, testedBinder: Symbol)      = codegen._equals(pat, testedBinder)
         def eqTest(pat: Tree, testedBinder: Symbol)          = REF(testedBinder) OBJ_EQ pat
 
-        def outerTest(testedBinder: Symbol, expectedTp: Type): Tree = {
-          val expectedOuter = expectedTp.prefix match {
-            case ThisType(clazz) => This(clazz)
-            case NoType          => mkTRUE // fallback for SI-6183
-            case pre             => REF(pre.prefix, pre.termSymbol)
+        override def withOuterTest(orig: Tree)(testedBinder: Symbol, expectedTp: Type): Tree = {
+          val expectedPrefix = expectedTp.prefix
+          val testedPrefix = testedBinder.info.prefix
+
+          // Check if a type is defined in a static location. Unlike `tp.isStatic` before `flatten`,
+          // this also includes methods and (possibly nested) objects inside of methods.
+          def definedInStaticLocation(tp: Type): Boolean = {
+            def isStatic(tp: Type): Boolean =
+              if (tp == NoType || tp.typeSymbol.isPackageClass || tp == NoPrefix) true
+              else if (tp.typeSymbol.isModuleClass) isStatic(tp.prefix)
+              else false
+            tp.typeSymbol.owner == tp.prefix.typeSymbol && isStatic(tp.prefix)
           }
 
-          // ExplicitOuter replaces `Select(q, outerSym) OBJ_EQ expectedPrefix` by `Select(q, outerAccessor(outerSym.owner)) OBJ_EQ expectedPrefix`
-          // if there's an outer accessor, otherwise the condition becomes `true` -- TODO: can we improve needsOuterTest so there's always an outerAccessor?
-          val outer = expectedTp.typeSymbol.newMethod(vpmName.outer, newFlags = SYNTHETIC | ARTIFACT) setInfo expectedTp.prefix
-
-          (Select(codegen._asInstanceOf(testedBinder, expectedTp), outer)) OBJ_EQ expectedOuter
+          if ((expectedPrefix eq NoPrefix)
+            || definedInStaticLocation(expectedTp)
+            || testedPrefix =:= expectedPrefix) orig
+          else gen.mkAttributedQualifierIfPossible(expectedPrefix) match {
+            case None => orig
+            case Some(expectedOuterRef) =>
+              // ExplicitOuter replaces `Select(q, outerSym) OBJ_EQ expectedPrefix`
+              // by `Select(q, outerAccessor(outerSym.owner)) OBJ_EQ expectedPrefix`
+              // if there's an outer accessor, otherwise the condition becomes `true`
+              // TODO: centralize logic whether there's an outer accessor and use here?
+              val synthOuterGetter = expectedTp.typeSymbol.newMethod(vpmName.outer, newFlags = SYNTHETIC | ARTIFACT) setInfo expectedPrefix
+              val outerTest = (Select(codegen._asInstanceOf(testedBinder, expectedTp), synthOuterGetter)) OBJ_EQ expectedOuterRef
+              and(orig, outerTest)
+          }
         }
       }
 
@@ -356,7 +372,6 @@ trait MatchTreeMaking extends MatchCodeGen with Debugging {
 
         def typeTest(testedBinder: Symbol, expectedTp: Type): Result  = true
 
-        def outerTest(testedBinder: Symbol, expectedTp: Type): Result = false
         def nonNullTest(testedBinder: Symbol): Result                 = false
         def equalsTest(pat: Tree, testedBinder: Symbol): Result       = false
         def eqTest(pat: Tree, testedBinder: Symbol): Result           = false
@@ -368,7 +383,6 @@ trait MatchTreeMaking extends MatchCodeGen with Debugging {
         type Result = Boolean
 
         def typeTest(testedBinder: Symbol, expectedTp: Type): Result  = testedBinder eq binder
-        def outerTest(testedBinder: Symbol, expectedTp: Type): Result = false
         def nonNullTest(testedBinder: Symbol): Result                 = testedBinder eq binder
         def equalsTest(pat: Tree, testedBinder: Symbol): Result       = false // could in principle analyse pat and see if it's statically known to be non-null
         def eqTest(pat: Tree, testedBinder: Symbol): Result           = false // could in principle analyse pat and see if it's statically known to be non-null
@@ -405,12 +419,6 @@ trait MatchTreeMaking extends MatchCodeGen with Debugging {
       import TypeTestTreeMaker._
       debug.patmat("TTTM"+((prevBinder, extractorArgTypeTest, testedBinder, expectedTp, nextBinderTp)))
 
-      lazy val outerTestNeeded = (
-           (expectedTp.prefix ne NoPrefix)
-        && !expectedTp.prefix.typeSymbol.isPackageClass
-        && needsOuterTest(expectedTp, testedBinder.info, matchOwner)
-      )
-
       // the logic to generate the run-time test that follows from the fact that
       // a `prevBinder` is expected to have type `expectedTp`
       // the actual tree-generation logic is factored out, since the analyses generate Cond(ition)s rather than Trees
@@ -429,12 +437,11 @@ trait MatchTreeMaking extends MatchCodeGen with Debugging {
         def isExpectedPrimitiveType = isAsExpected && isPrimitiveValueType(expectedTp)
         def isExpectedReferenceType = isAsExpected && (expectedTp <:< AnyRefTpe)
         def mkNullTest              = nonNullTest(testedBinder)
-        def mkOuterTest             = outerTest(testedBinder, expectedTp)
         def mkTypeTest              = typeTest(testedBinder, expectedWide)
 
         def mkEqualsTest(lhs: Tree): cs.Result      = equalsTest(lhs, testedBinder)
         def mkEqTest(lhs: Tree): cs.Result          = eqTest(lhs, testedBinder)
-        def addOuterTest(res: cs.Result): cs.Result = if (outerTestNeeded) and(res, mkOuterTest) else res
+        def addOuterTest(res: cs.Result): cs.Result = withOuterTest(res)(testedBinder, expectedTp)
 
         // If we conform to expected primitive type:
         //   it cannot be null and cannot have an outer pointer. No further checking.

src/reflect/scala/reflect/internal/TreeGen.scala

@@ -117,6 +117,30 @@ abstract class TreeGen {
     case _                       => qual
   }
 
+
+
+  //          val selType = testedBinder.info
+  //
+  //          // See the test for SI-7214 for motivation for dealias. Later `treeCondStrategy#outerTest`
+  //          // generates an outer test based on `patType.prefix` with automatically dealises.
+  //          // Prefixes can have all kinds of shapes SI-9110
+  //          val patPre = expectedTp.dealiasWiden.prefix
+  //          val selPre = selType.dealiasWiden.prefix
+  //
+  //          // Optimization: which prefixes can we disqualify from the need for an outer reference check?
+  //          //   - classes in static owners do not get outer pointers
+  //          //   - if the prefixes are statically known to be equal, the type system ensures an outer test is redundant
+  //          !((patPre eq NoPrefix) || (selPre eq NoPrefix)
+  //            || patPre.typeSymbol.isPackageClass
+  //            || selPre =:= patPre)
+
+  def mkAttributedQualifierIfPossible(prefix: Type): Option[Tree] = prefix match {
+    case NoType | NoPrefix | ErrorType => None
+    case TypeRef(_, sym, _) if sym.isModule || sym.isClass || sym.isType => None
+    case pre => Some(mkAttributedQualifier(prefix))
+  }
+
+
   /** Builds a reference to given symbol with given stable prefix. */
   def mkAttributedRef(pre: Type, sym: Symbol): RefTree = {
     val qual = mkAttributedQualifier(pre)

src/reflect/scala/reflect/internal/Types.scala

@@ -3946,47 +3946,6 @@ trait Types
     check(tp1, tp2) && check(tp2, tp1)
   }
 
-  /** Does a pattern of type `patType` need an outer test when executed against
-   *  selector type `selType` in context defined by `currentOwner`?
-   */
-  def needsOuterTest(patType: Type, selType: Type, currentOwner: Symbol) = {
-    def createDummyClone(pre: Type): Type = {
-      val dummy = currentOwner.enclClass.newValue(nme.ANYname).setInfo(pre.widen)
-      singleType(ThisType(currentOwner.enclClass), dummy)
-    }
-    def maybeCreateDummyClone(pre: Type, sym: Symbol): Type = pre match {
-      case SingleType(pre1, sym1) =>
-        if (sym1.isModule && sym1.isStatic) {
-          if (sym.owner == sym1 || sym.isJavaDefined || sym.owner.sourceModule.isStaticModule) NoType
-          else pre
-        } else if (sym1.isModule && sym.owner == sym1.moduleClass) {
-          val pre2 = maybeCreateDummyClone(pre1, sym1)
-          if (pre2 eq NoType) pre2
-          else singleType(pre2, sym1)
-        } else {
-          createDummyClone(pre)
-        }
-      case ThisType(clazz) =>
-        if (clazz.isModuleClass)
-          maybeCreateDummyClone(clazz.typeOfThis, sym)
-        else if (sym.owner == clazz && (sym.hasFlag(PRIVATE) || sym.privateWithin == clazz))
-          NoType
-        else
-          createDummyClone(pre)
-      case _ =>
-        NoType
-    }
-    // See the test for SI-7214 for motivation for dealias. Later `treeCondStrategy#outerTest`
-    // generates an outer test based on `patType.prefix` with automatically dealises.
-    patType.dealias match {
-      case TypeRef(pre, sym, args) =>
-        val pre1 = maybeCreateDummyClone(pre, sym)
-        (pre1 ne NoType) && isPopulated(copyTypeRef(patType, pre1, sym, args), selType)
-      case _ =>
-        false
-    }
-  }
-
   def normalizePlus(tp: Type): Type = {
     if (isRawType(tp)) rawToExistential(tp)
     else tp.normalize match {

test/files/neg/outer-ref-checks.check

@@ -0,0 +1,24 @@
+outer-ref-checks.scala:5: warning: The outer reference in this type test cannot be checked at run time.
+  final case class Inner(val s: String) // unchecked warning
+                   ^
+outer-ref-checks.scala:8: warning: The outer reference in this type test cannot be checked at run time.
+    case Inner(s) => // unchecked warning
+              ^
+outer-ref-checks.scala:18: warning: The outer reference in this type test cannot be checked at run time.
+    case Inner(s) => // unchecked warning
+              ^
+outer-ref-checks.scala:19: warning: The outer reference in this type test cannot be checked at run time.
+    case O.Inner(s) => // unchecked warning
+                ^
+outer-ref-checks.scala:41: warning: The outer reference in this type test cannot be checked at run time.
+    case Inner(s) => // unchecked warning
+              ^
+outer-ref-checks.scala:46: warning: The outer reference in this type test cannot be checked at run time.
+    case _: Inner => // unchecked warning
+          ^
+outer-ref-checks.scala:56: warning: The outer reference in this type test cannot be checked at run time.
+    case _: (Inner @uncheckedVariance) => // unchecked warning
+          ^
+error: No warnings can be incurred under -Xfatal-warnings.
+7 warnings found
+one error found

test/files/neg/outer-ref-checks.flags

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

test/files/neg/outer-ref-checks.scala

@@ -0,0 +1,106 @@
+import scala.annotation.unchecked.uncheckedVariance
+
+class Outer {
+  // A final class gets no outer ref, so we expect to see warnings where an outer ref check should be performed
+  final case class Inner(val s: String) // unchecked warning
+
+  def belongs(a: Any): Unit = a match {
+    case Inner(s) => // unchecked warning
+    case _ =>
+  }
+
+  def belongsStaticSameOuter(a: Inner): Unit = a match {
+    case Inner(s) => // no need for outer check
+    // match is exhaustive, no default case needed
+  }
+
+  def belongsOtherOuter(a: Outer#Inner): Unit = a match {
+    case Inner(s) => // unchecked warning
+    case O.Inner(s) => // unchecked warning
+    case _ =>
+  }
+}
+
+object O extends Outer {
+  def belongsStaticSameOuter2(a: Inner): Unit = a match {
+    case Inner(s) => // no need for outer check
+    // match is exhaustive, no default case needed
+  }
+
+  def belongsStaticSameOuter3(a: Inner): Unit = a match {
+    case _: Inner => // no need for outer check
+    // match is exhaustive, no default case needed
+  }
+
+  def belongsStaticSameOuter4(a: Inner): Unit = a match {
+    case _: (Inner @uncheckedVariance) => // no need for outer check
+    // match is exhaustive, no default case needed
+  }
+
+  def belongsOtherOuter2(a: Outer#Inner): Unit = a match {
+    case Inner(s) => // unchecked warning
+    case _ =>
+  }
+
+  def belongsOtherOuter3(a: Outer#Inner): Unit = a match {
+    case _: Inner => // unchecked warning
+    case _ =>
+  }
+
+  def belongsOtherOuter4(a: Outer#Inner): Unit = a match {
+    case _: (Inner @unchecked) => // warning supressed
+    case _ =>
+  }
+
+  def belongsOtherOuter5(a: Outer#Inner): Unit = a match {
+    case _: (Inner @uncheckedVariance) => // unchecked warning
+    case _ =>
+  }
+
+  def nested: Unit = {
+    final case class I(s: String)
+
+    def check1(a: Any): Unit = a match {
+      case I(s) => // no need for outer check
+      case _ =>
+    }
+
+    def check2(a: I): Unit = a match {
+      case I(s) => // no need for outer check
+      // match is exhaustive, no default case needed
+    }
+  }
+}
+
+class O2 {
+  def nested: Unit = {
+    final case class I(s: String)
+
+    def check1(a: Any): Unit = a match {
+      case I(s) => // no need for outer check (is this correct?)
+      case _ =>
+    }
+
+    def check2(a: I): Unit = a match {
+      case I(s) => // no need for outer check (is this correct?)
+      // match is exhaustive, no default case needed
+    }
+  }
+}
+
+package p {
+  object T {
+    case class C(x: Int)
+  }
+}
+
+object U {
+  val T = p.T
+}
+
+class Test {
+  def m(a: Any) = a match {
+    case U.T.C(1) => 1  // used to warn
+    case _ => 1
+  }
+}

test/files/neg/t7171.check

@@ -1,6 +1,9 @@
 t7171.scala:2: warning: The outer reference in this type test cannot be checked at run time.
   final case class A()
                    ^
+t7171.scala:9: warning: The outer reference in this type test cannot be checked at run time.
+    case _: A => true; case _ => false
+          ^
 error: No warnings can be incurred under -Xfatal-warnings.
-one warning found
+two warnings found
 one error found

test/files/neg/t7171b.check

@@ -1,6 +1,12 @@
 t7171b.scala:2: warning: The outer reference in this type test cannot be checked at run time.
   final case class A()
                    ^
+t7171b.scala:8: warning: The outer reference in this type test cannot be checked at run time.
+    case _: A => true; case _ => false
+          ^
+t7171b.scala:13: warning: The outer reference in this type test cannot be checked at run time.
+    case _: A => true; case _ => false
+          ^
 error: No warnings can be incurred under -Xfatal-warnings.
-one warning found
+three warnings found
 one error found

test/files/run/t7171.check

@@ -1,3 +1,6 @@
 t7171.scala:2: warning: The outer reference in this type test cannot be checked at run time.
   final case class A()
                    ^
+t7171.scala:9: warning: The outer reference in this type test cannot be checked at run time.
+    case _: A => true; case _ => false
+          ^