Fix access to protected members in package-protected classes

A super access to a protected member in a package-protected class is allowed through
an intermediate public class. The fix for scala/bug#5162 actually introduced an error
for this case, because the scala compiler would generate an INVOKESPECIAL with the
package-protected class as receiver, which is illegal. However, we can use the public
intermediate class in the invocation signature.

Fixes scala/bug#7936

This is very similar to scala/bug#4283

src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala

@@ -557,7 +557,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
 
           generatedType = genTypeApply()
 
-        case Apply(fun @ Select(Super(qual, _), _), args) =>
+        case Apply(fun @ Select(sup @ Super(superQual, _), _), args) =>
           def initModule() {
             // we initialize the MODULE$ field immediately after the super ctor
             if (!isModuleInitialized &&
@@ -576,9 +576,9 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
           }
 
           // scala/bug#10290: qual can be `this.$outer()` (not just `this`), so we call genLoad (not jsut ALOAD_0)
-          genLoad(qual)
+          genLoad(superQual)
           genLoadArguments(args, paramTKs(app))
-          generatedType = genCallMethod(fun.symbol, InvokeStyle.Super, app.pos)
+          generatedType = genCallMethod(fun.symbol, InvokeStyle.Super, app.pos, sup.tpe.typeSymbol)
           initModule()
 
         // 'new' constructor call: Note: since constructors are
@@ -1024,14 +1024,14 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
     /**
      * Generate a method invocation. If `specificReceiver != null`, it is used as receiver in the
      * invocation instruction, otherwise `method.owner`. A specific receiver class is needed to
-     * prevent an IllegalAccessError, (aladdin bug 455).
+     * prevent an IllegalAccessError, (aladdin bug 455). Same for super calls, scala/bug#7936.
      */
     def genCallMethod(method: Symbol, style: InvokeStyle, pos: Position, specificReceiver: Symbol = null): BType = {
       val methodOwner = method.owner
       // the class used in the invocation's method descriptor in the classfile
       val receiverClass = {
         if (specificReceiver != null)
-          assert(style.isVirtual || specificReceiver == methodOwner, s"specificReceiver can only be specified for virtual calls. $method - $specificReceiver")
+          assert(style.isVirtual || style.isSuper || specificReceiver == methodOwner, s"specificReceiver can only be specified for virtual calls. $method - $specificReceiver")
 
         val useSpecificReceiver = specificReceiver != null && !specificReceiver.isBottomClass
         val receiver = if (useSpecificReceiver) specificReceiver else methodOwner
@@ -1070,8 +1070,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
       val isInterface = receiverBType.isInterface.get
       import InvokeStyle._
       if (style == Super) {
-        assert(receiverClass == methodOwner, s"for super call, expecting $receiverClass == $methodOwner")
-        if (receiverClass.isTrait && !receiverClass.isJavaDefined) {
+        if (receiverClass.isTrait && !method.isJavaDefined) {
           val staticDesc = MethodBType(typeToBType(method.owner.info) :: bmType.argumentTypes, bmType.returnType).descriptor
           val staticName = traitSuperAccessorName(method).toString
           bc.invokestatic(receiverName, staticName, staticDesc, isInterface, pos)

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

@@ -402,16 +402,14 @@ abstract class Erasure extends InfoTransform
       *  to tree, which is assumed to be the body of a constructor of class clazz.
       */
     private def addMixinConstructorCalls(tree: Tree, clazz: Symbol): Tree = {
-      def mixinConstructorCall(mc: Symbol): Tree = atPos(tree.pos) {
-        Apply(SuperSelect(clazz, mc.primaryConstructor), Nil)
-      }
-      val mixinConstructorCalls: List[Tree] = {
-        for (mc <- clazz.mixinClasses.reverse
-             if mc.isTrait && mc.primaryConstructor != NoSymbol)
-          yield mixinConstructorCall(mc)
+      def mixinConstructorCalls: List[Tree] = {
+        for (mc <- clazz.mixinClasses.reverse if mc.isTrait && mc.primaryConstructor != NoSymbol)
+          yield atPos(tree.pos) {
+            Apply(SuperSelect(clazz, mc.primaryConstructor), Nil)
+          }
       }
-      tree match {
 
+      tree match {
         case Block(Nil, expr) =>
           // AnyVal constructor - have to provide a real body so the
           // jvm doesn't throw a VerifyError. But we can't add the
@@ -726,8 +724,16 @@ abstract class Erasure extends InfoTransform
               assert(qual1.symbol.isStable, qual1.symbol)
               adaptMember(selectFrom(applyMethodWithEmptyParams(qual1)))
             } else if (!(qual1.isInstanceOf[Super] || (qual1.tpe.typeSymbol isSubClass tree.symbol.owner))) {
-              assert(tree.symbol.owner != ArrayClass)
-              selectFrom(cast(qual1, tree.symbol.owner.tpe.resultType))
+              // For example in `(foo: Option[String]).get.trim`, the qualifier has type `Object`.
+              // A `QualTypeSymAttachment` is present if the selected member's owner is not an
+              // accessible (java-defined) class, see `preErase`. Selections from `super` are not
+              // handled here because inserting a cast would not be legal. Instead there's a
+              // special case in `typedSelectInternal`.
+              val qualTpe = tree.getAndRemoveAttachment[QualTypeSymAttachment] match {
+                case Some(a) => a.sym.tpe
+                case None => tree.symbol.owner.tpe.resultType
+              }
+              selectFrom(cast(qual1, qualTpe))
             } else {
               selectFrom(qual1)
             }
@@ -938,9 +944,6 @@ abstract class Erasure extends InfoTransform
      *   - Add bridge definitions to a template.
      *   - Replace all types in type nodes and the EmptyTree object by their erasure.
      *     Type nodes of type Unit representing result types of methods are left alone.
-     *   - Given a selection q.s, where the owner of `s` is not accessible but the
-     *     type symbol of q's type qT is accessible, insert a cast (q.asInstanceOf[qT]).s
-     *     This prevents illegal access errors (see #4283).
      *   - Remove all instance creations new C(arg) where C is an inlined class.
      *   - Reset all other type attributes to null, thus enforcing a retyping.
      */
@@ -1153,24 +1156,47 @@ abstract class Erasure extends InfoTransform
             }
           }
 
-          def isJvmAccessible(sym: Symbol) = (sym.isClass && !sym.isJavaDefined) || localTyper.context.isAccessible(sym, sym.owner.thisType)
-          if (!isJvmAccessible(owner) && qual.tpe != null) {
-            qual match {
-              case Super(_, _) =>
-                // Insert a cast here at your peril -- see scala/bug#5162.
-                reporter.error(tree.pos, s"Unable to access ${tree.symbol.fullLocationString} with a super reference.")
-                tree
-              case _ =>
-                // Todo: Figure out how qual.tpe could be null in the check above (it does appear in build where SwingWorker.this
-                // has a null type).
-                val qualSym = qual.tpe.widen.typeSymbol
-                if (isJvmAccessible(qualSym) && !qualSym.isPackageClass && !qualSym.isPackageObjectClass) {
-                  // insert cast to prevent illegal access error (see #4283)
-                  // util.trace("insert erasure cast ") (*/
-                  treeCopy.Select(tree, gen.mkAttributedCast(qual, qual.tpe.widen), name) //)
-                } else tree
+          // This code may add an QualTypeSymAttachment to the Select tree. The referenced class is
+          // then used in erasure type checking as the type of the Select's qualifier. This fixes
+          // two situations where erasure type checking cannot assign a precise enough type.
+          //
+          //  - In a `super.m` selection, erasure typing assigns the type of the superclass to the
+          //    Super tree. This is wrong if `m` is a member of a trait (not the superclass). A
+          //    special-case in `typedSelectInternal` assigns m's owner in this case.
+          //  - In a non-super selection, the qualifier may erase to a type that doesn't hold the
+          //    selected member, for example `(q: Option[String]).get.trim` erases to Object, not
+          //    String. Erasure's `adaptMember` then introduces a cast to the member's owner.
+          //
+          // In both cases, using the member's owner is not legal if the member is defined in
+          // Java and the owner class is not accessible (scala/bug#7936, scala/bug#4283). In this
+          // situation we store a valid class type of the qualifier in the attachment.
+          //   - For `super.m`, we store a direct parent of the current class
+          //   - For a non-super selection, we store the non-erased class type of the qualifier
+          //
+          // In addition, for `super.m` selections, we also store a direct parent of the current
+          // class if `m` is defined in Java. This avoids the need for having the Java class as
+          // a direct parent (scala-dev#143).
+          if (qual.isInstanceOf[Super]) {
+            val qualSym = accessibleOwnerOrParentDefiningMember(sym, qual.tpe.typeSymbol.parentSymbols, localTyper.context) match {
+              case Some(p) => p
+              case None =>
+                // There is no test for this warning, I have been unable to come up with an example that would trigger it.
+                // In a selection `a.m`, there must be a direct parent from which `m` can be selected.
+                reporter.error(tree.pos, s"Unable to emit super reference to ${sym.fullLocationString}, $owner is not accessible in ${localTyper.context.enclClass.owner}")
+                owner
             }
-          } else tree
+            if (qualSym != owner)
+              tree.updateAttachment(new QualTypeSymAttachment(qualSym))
+          } else if (!isJvmAccessible(owner, localTyper.context)) {
+            val qualSym = qual.tpe.typeSymbol
+            if (qualSym != owner && isJvmAccessible(qualSym, localTyper.context) && definesMemberAfterErasure(qualSym, sym))
+              tree.updateAttachment(new QualTypeSymAttachment(qualSym))
+            else
+              reporter.error(tree.pos, s"Unable to emit reference to ${sym.fullLocationString}, $owner is not accessible in ${localTyper.context.enclClass.owner}")
+          }
+
+          tree
+
         case Template(parents, self, body) =>
           //Console.println("checking no dble defs " + tree)//DEBUG
           checkNoDoubleDefs(tree.symbol.owner)
@@ -1294,5 +1320,39 @@ abstract class Erasure extends InfoTransform
     ok(tpSym) && tpSym.ancestors.forall(sym => (sym eq AnyClass) || (sym eq ObjectClass) || ok(sym))
   }
 
+  final def isJvmAccessible(cls: Symbol, context: global.analyzer.Context): Boolean =
+    !cls.isJavaDefined || context.isAccessible(cls, cls.owner.thisType)
+
+  /**
+   * Check if a class defines a member after erasure. The phase travel is important for
+   * `trait T extends AClass`: after erasure (and in bytecode), `T` has supertype `Object`, not
+   * `AClass`.
+   */
+  final def definesMemberAfterErasure(cls: Symbol, member: Symbol): Boolean =
+    exitingErasure(cls.tpe.member(member.name).alternatives.contains(member))
+
+  /**
+   * The goal of this method is to find a class that is accessible (in bytecode) and can be used
+   * to select `member`.
+   * - For constructors, it returns the `member.owner`. We can assume the class is accessible: if
+   *   it wasn't, the typer would have rejected the program, as the class is referenced in source.
+   * - For Scala-defined members it also returns `member.owner`, all Scala-defined classes are
+   *   public in bytecode.
+   * - For Java-defined members we prefer a direct parent over of the owner, even if the owner is
+   *   accessible. This way the owner doesn't need to be added as a direct parent, see scala-dev#143.
+   */
+  final def accessibleOwnerOrParentDefiningMember(member: Symbol, parents: List[Symbol], context: global.analyzer.Context): Option[Symbol] = {
+    def eraseAny(cls: Symbol) = if (cls == AnyClass || cls == AnyValClass) ObjectClass else cls
+
+    if (member.isConstructor || !member.isJavaDefined) Some(eraseAny(member.owner))
+    else parents.find { p =>
+      val e = eraseAny(p)
+      isJvmAccessible(e, context) && definesMemberAfterErasure(e, member)
+    } orElse {
+      val e = eraseAny(member.owner)
+      if (isJvmAccessible(e, context)) Some(e) else None
+    }
+  }
+
   private class TypeRefAttachment(val tpe: TypeRef)
 }

src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala

@@ -141,14 +141,17 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT
             !(member.isAbstractOverride && member.isIncompleteIn(clazz)))
           reporter.error(sel.pos, ""+sym.fullLocationString+" is accessed from super. It may not be abstract "+
                                "unless it is overridden by a member declared `abstract' and `override'")
-      } else if (mix == tpnme.EMPTY && !sym.owner.isTrait){
-        // scala/bug#4989 Check if an intermediate class between `clazz` and `sym.owner` redeclares the method as abstract.
-        val intermediateClasses = clazz.info.baseClasses.tail.takeWhile(_ != sym.owner)
-        intermediateClasses.map(sym.overridingSymbol).find(s => s.isDeferred && !s.isAbstractOverride && !s.owner.isTrait).foreach {
-          absSym =>
-            reporter.error(sel.pos, s"${sym.fullLocationString} cannot be directly accessed from $clazz because ${absSym.owner} redeclares it as abstract")
-        }
       } else {
+        val owner = sym.owner
+        if (mix == tpnme.EMPTY && !owner.isTrait) {
+          // scala/bug#4989 Check if an intermediate class between `clazz` and `owner` redeclares the method as abstract.
+          val intermediateClasses = clazz.info.baseClasses.tail.takeWhile(_ != owner)
+          intermediateClasses.map(sym.overridingSymbol).find(s => s.isDeferred && !s.isAbstractOverride && !s.owner.isTrait).foreach {
+            absSym =>
+              reporter.error(sel.pos, s"${sym.fullLocationString} cannot be directly accessed from $clazz because ${absSym.owner} redeclares it as abstract")
+          }
+        }
+
         // SD-143: a call super[T].m that resolves to A.m cannot be translated to correct bytecode if
         //   - A is a class (not a trait / interface), but not the direct superclass. Invokespecial
         //     would select an overriding method in the direct superclass, rather than A.m.
@@ -161,13 +164,17 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT
           else if (member.overridingSymbol(subclass) != NoSymbol) true
           else hasClassOverride(member, subclass.superClass)
         }
-        val owner = sym.owner
         if (mix != tpnme.EMPTY && !owner.isTrait && owner != clazz.superClass && hasClassOverride(sym, clazz.superClass)) {
           reporter.error(sel.pos,
             s"cannot emit super call: the selected $sym is declared in $owner, which is not the direct superclass of $clazz.\n" +
             s"An unqualified super call (super.${sym.name}) would be allowed.")
-        } else if (owner.isInterface && owner.isJavaDefined && !clazz.parentSymbols.contains(owner)) {
-          reporter.error(sel.pos, s"unable to emit super call unless interface ${owner.name} (which declares $sym) is directly extended by $clazz.")
+        } else if (owner.isInterface && owner.isJavaDefined) {
+          // There is no test left for this warning, as I have been unable to come up with an example that would trigger it.
+          // For a `super.m` selection, there must be a direct parent from which `m` can be selected. This parent will be used
+          // as receiver in the invokespecial call.
+          val receiverInBytecode = erasure.accessibleOwnerOrParentDefiningMember(sym, sup.tpe.typeSymbol.parentSymbols, localTyper.context).getOrElse(sym.owner)
+          if (!clazz.parentSymbols.contains(receiverInBytecode))
+            reporter.error(sel.pos, s"unable to emit super call unless interface ${owner.name} (which declares $sym) is directly extended by $clazz.")
         }
       }
 

src/compiler/scala/tools/nsc/typechecker/Typers.scala

@@ -4886,8 +4886,21 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
             return typed(treeCopy.Select(tree, qual1, name), mode, pt)
         }
 
-        if (phase.erasedTypes && qual.isInstanceOf[Super] && tree.symbol != NoSymbol)
-          qual setType tree.symbol.owner.tpe
+        // This special-case complements the logic in `adaptMember` in erasure, it handles selections
+        // from `Super`. In `adaptMember`, if the erased type of a qualifier doesn't conform to the
+        // owner of the selected member, a cast is inserted, e.g., (foo: Option[String]).get.trim).
+        // Similarly, for `super.m`, typing `super` during erasure assigns the superclass. If `m`
+        // is defined in a trait, this is incorrect, we need to assign a type to `super` that conforms
+        // to the owner of `m`. Adding a cast (as in `adaptMember`) would not work, `super.asInstanceOf`
+        // is not a valid tree.
+        if (phase.erasedTypes && qual.isInstanceOf[Super]) {
+          //  See the comment in `preErase` why we use the attachment (scala/bug#7936)
+          val qualSym = tree.getAndRemoveAttachment[QualTypeSymAttachment] match {
+            case Some(a) => a.sym
+            case None => sym.owner
+          }
+          qual.setType(qualSym.tpe)
+        }
 
         if (!reallyExists(sym)) {
           def handleMissing: Tree = {

src/reflect/scala/reflect/internal/StdAttachments.scala

@@ -14,6 +14,11 @@ trait StdAttachments {
     def setAttachments(attachments: scala.reflect.macros.Attachments { type Pos = Position }): this.type = { rawatt = attachments; this }
     def updateAttachment[T: ClassTag](attachment: T): this.type = { rawatt = rawatt.update(attachment); this }
     def removeAttachment[T: ClassTag]: this.type = { rawatt = rawatt.remove[T]; this }
+    def getAndRemoveAttachment[T: ClassTag]: Option[T] = {
+      val r = attachments.get[T]
+      if (r.nonEmpty) removeAttachment[T]
+      r
+    }
     def hasAttachment[T: ClassTag]: Boolean = rawatt.contains[T]
 
     // cannot be final due to SynchronizedSymbols
@@ -93,4 +98,6 @@ trait StdAttachments {
     * error to indicate that the earlier observation was incomplete.
     */
   case object KnownDirectSubclassesCalled extends PlainAttachment
+
+  class QualTypeSymAttachment(val sym: Symbol)
 }

src/reflect/scala/reflect/internal/Trees.scala

@@ -1161,9 +1161,16 @@ trait Trees extends api.Trees {
   def Super(sym: Symbol, mix: TypeName): Tree =
     Super(This(sym), mix)
 
-  /** Selection of a method in an arbitrary ancestor */
-  def SuperSelect(clazz: Symbol, sym: Symbol): Tree =
-    Select(Super(clazz, tpnme.EMPTY), sym)
+  /**
+   * Creates a tree that selects a specific member `sym` without having to qualify the `super`.
+   * For example, given traits `B <:< A`, a class `C <:< B` needs to invoke `A.$init$`. If `A` is
+   * not a direct parent, a tree `super[A].$init$` would not type check ("does not name a parent").
+   * So we generate `super.$init$` and pre-assign the correct symbol. A special-case in
+   * `typedSelectInternal` assigns the correct type `A` to the `super` qualifier.
+   */
+  def SuperSelect(clazz: Symbol, sym: Symbol): Tree = {
+    Select(Super(clazz, tpnme.EMPTY), sym).updateAttachment(new QualTypeSymAttachment(sym.owner))
+  }
 
   def This(sym: Symbol): Tree =
     This(sym.name.toTypeName) setSymbol sym

test/files/neg/t10249.check

@@ -0,0 +1,4 @@
+Test_1.scala:11: error: Unable to emit reference to method m in class A, class A is not accessible in object Test
+    w.m()
+      ^
+one error found

test/files/neg/t10249/A.java

@@ -0,0 +1,5 @@
+package a;
+
+class A {
+    public final int m() { return 1; }
+}

test/files/neg/t10249/Test_1.scala

@@ -0,0 +1,13 @@
+package a {
+  // A is a class, so W does not conform to A in bytecode. an access (w: W).m() requires a cast to A.
+  // If `A` is not accessible, there's no solution.
+  trait W extends A
+  class C extends W
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    val w: a.W = new a.C
+    w.m()
+  }
+}