Merge pull request #4529 from lrytz/inlineAccessibility

Fix illegal inlining of instructions accessing protected members

2 files changed, 74 insertions, 18 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala
index 1b9fd5e298..fffb9286b8 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala
@@ -213,6 +213,35 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
     assert(!primitiveTypeMap.contains(sym) || isCompilingPrimitive, sym)
   }
 
+  /**
+   * Reconstruct the classfile flags from a Java defined class symbol.
+   *
+   * The implementation of this method is slightly different that [[javaFlags]]. The javaFlags
+   * method is primarily used to map Scala symbol flags to sensible classfile flags that are used
+   * in the generated classfiles. For example, all classes emitted by the Scala compiler have
+   * ACC_PUBLIC.
+   *
+   * When building a [[ClassBType]] from a Java class symbol, the flags in the type's `info` have
+   * to correspond exactly to the flags in the classfile. For example, if the class is package
+   * protected (i.e., it doesn't have the ACC_PUBLIC flag), this needs to be reflected in the
+   * ClassBType. For example, the inliner needs the correct flags for access checks.
+   *
+   * Class flags are listed here:
+   *   https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.1-200-E.1
+   */
+  private def javaClassfileFlags(classSym: Symbol): Int = {
+    assert(classSym.isJava, s"Expected Java class symbol, got ${classSym.fullName}")
+    import asm.Opcodes._
+    GenBCode.mkFlags(
+      if (classSym.isPublic)        ACC_PUBLIC    else 0,
+      if (classSym.isFinal)         ACC_FINAL     else 0,
+      if (classSym.isInterface)     ACC_INTERFACE else ACC_SUPER, // see the link above. javac does the same: ACC_SUPER for all classes, but not interfaces.
+      if (classSym.hasAbstractFlag) ACC_ABSTRACT  else 0,
+      if (classSym.isArtifact)      ACC_SYNTHETIC else 0,
+      if (classSym.hasEnumFlag)     ACC_ENUM      else 0
+    )
+  }
+
   private def setClassInfo(classSym: Symbol, classBType: ClassBType): ClassBType = {
     val superClassSym = if (classSym.isImplClass) ObjectClass else classSym.superClass
     assert(
@@ -230,7 +259,10 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
 
     val interfaces = implementedInterfaces(classSym).map(classBTypeFromSymbol)
 
-    val flags = javaFlags(classSym)
+    val flags = {
+      if (classSym.isJava) javaClassfileFlags(classSym) // see comment on javaClassfileFlags
+      else javaFlags(classSym)
+    }
 
     /* The InnerClass table of a class C must contain all nested classes of C, even if they are only
      * declared but not otherwise referenced in C (from the bytecode or a method / field signature).
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala
index 814c78b69c..b4f091b37f 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala
@@ -562,38 +562,62 @@ class Inliner[BT <: BTypes](val btypes: BT) {
      * @param memberDeclClass The class in which the member is declared (A)
      * @param memberRefClass  The class used in the member reference (B)
      *
+     * (B0) JVMS 5.4.3.2 / 5.4.3.3: when resolving a member of class C in D, the class C is resolved
+     * first. According to 5.4.3.1, this requires C to be accessible in D.
+     *
      * JVMS 5.4.4 summary: A field or method R is accessible to a class D (destinationClass) iff
      *  (B1) R is public
      *  (B2) R is protected, declared in C (memberDeclClass) and D is a subclass of C.
      *       If R is not static, R must contain a symbolic reference to a class T (memberRefClass),
      *       such that T is either a subclass of D, a superclass of D, or D itself.
+     *       Also (P) needs to be satisfied.
      *  (B3) R is either protected or has default access and declared by a class in the same
      *       run-time package as D.
+     *       If R is protected, also (P) needs to be satisfied.
      *  (B4) R is private and is declared in D.
+     *
+     *  (P) When accessing a protected instance member, the target object on the stack (the receiver)
+     *      has to be a subtype of D (destinationClass). This is enforced by classfile verification
+     *      (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.1.8).
+     *
+     * TODO: we cannot currently implement (P) because we don't have the necessary information
+     * available. Once we have a type propagation analysis implemented, we can extract the receiver
+     * type from there (https://github.com/scala-opt/scala/issues/13).
      */
     def memberIsAccessible(memberFlags: Int, memberDeclClass: ClassBType, memberRefClass: ClassBType): Either[OptimizerWarning, Boolean] = {
       // TODO: B3 requires "same run-time package", which seems to be package + classloader (JMVS 5.3.). is the below ok?
       def samePackageAsDestination = memberDeclClass.packageInternalName == destinationClass.packageInternalName
-
-      val key = (ACC_PUBLIC | ACC_PROTECTED | ACC_PRIVATE) & memberFlags
-      key match {
-        case ACC_PUBLIC     =>       // B1
-          Right(true)
-
-        case ACC_PROTECTED  =>       // B2
-          tryEither {
-            val condB2 = destinationClass.isSubtypeOf(memberDeclClass).orThrow && {
-              val isStatic = (ACC_STATIC & memberFlags) != 0
-              isStatic || memberRefClass.isSubtypeOf(destinationClass).orThrow || destinationClass.isSubtypeOf(memberRefClass).orThrow
+      def targetObjectConformsToDestinationClass = false // needs type propagation analysis, see above
+
+      def memberIsAccessibleImpl = {
+        val key = (ACC_PUBLIC | ACC_PROTECTED | ACC_PRIVATE) & memberFlags
+        key match {
+          case ACC_PUBLIC => // B1
+            Right(true)
+
+          case ACC_PROTECTED => // B2
+            val isStatic = (ACC_STATIC & memberFlags) != 0
+            tryEither {
+              val condB2 = destinationClass.isSubtypeOf(memberDeclClass).orThrow && {
+                isStatic || memberRefClass.isSubtypeOf(destinationClass).orThrow || destinationClass.isSubtypeOf(memberRefClass).orThrow
+              }
+              Right(
+                (condB2 || samePackageAsDestination /* B3 (protected) */) &&
+                (isStatic || targetObjectConformsToDestinationClass) // (P)
+              )
             }
-            Right(condB2 || samePackageAsDestination) // B3 (protected)
-          }
 
-        case 0 =>                    // B3 (default access)
-          Right(samePackageAsDestination)
+          case 0 => // B3 (default access)
+            Right(samePackageAsDestination)
+
+          case ACC_PRIVATE => // B4
+            Right(memberDeclClass == destinationClass)
+        }
+      }
 
-        case ACC_PRIVATE    =>       // B4
-          Right(memberDeclClass == destinationClass)
+      classIsAccessible(memberDeclClass) match { // B0
+        case Right(true) => memberIsAccessibleImpl
+        case r => r
       }
     }