Check protected accessors after pickling

New phase to check protected accessors.

We now create accessors also for Scala symbols. Previously this was not done
as it was assumed that such symbols will always be mapped to public.

We create the accessors anyway in order to better test the functionality, so
that we can at some point avoid the access widening from protected to public.

Author: odersky

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -60,6 +60,7 @@ class Compiler {
   protected def transformPhases: List[List[Phase]] =
     List(new FirstTransform,         // Some transformations to put trees into a canonical form
          new CheckReentrant,         // Internal use only: Check that compiled program has no data races involving global vars
+         new ProtectedAccessors,     // Add accessors for protected members
          new ElimPackagePrefixes) :: // Eliminate references to package prefixes in Select nodes
     List(new CheckStatic,            // Check restrictions that apply to @static members
          new ElimRepeated,           // Rewrite vararg parameters and arguments

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -711,6 +711,11 @@ object SymDenotations {
             i"""
                | Access to protected $this not permitted because enclosing ${ctx.owner.enclosingClass.showLocated}
                | is not a subclass of ${owner.showLocated} where target is defined""")
+        else if (cls.is(Trait) && !this.owner.is(Trait))
+          fail(
+            i"""
+               | Access to protected $this not permitted from $cls,
+               | since traits cannot access protected members of superclasses""")
         else if (
           !(  isType // allow accesses to types from arbitrary subclasses fixes #4737
            || pre.derivesFrom(cls)

compiler/src/dotty/tools/dotc/transform/AccessProxies.scala

@@ -14,6 +14,7 @@ import NameKinds.ClassifiedNameKind
 import ast.Trees._
 import util.Property
 import util.Positions.Position
+import config.Printers.transforms
 
 /** A utility class for generating access proxies. Currently used for
  *  inline accessors and protected accessors.
@@ -56,9 +57,11 @@ abstract class AccessProxies {
     def needsAccessor(sym: Symbol)(implicit ctx: Context): Boolean
 
     /** A fresh accessor symbol */
-    def newAccessorSymbol(accessed: Symbol, name: TermName, info: Type)(implicit ctx: Context): TermSymbol =
-      ctx.newSymbol(accessed.owner.enclosingSubClass, name, Synthetic | Method,
-                    info, coord = accessed.pos).entered
+    def newAccessorSymbol(owner: Symbol, name: TermName, info: Type, pos: Position)(implicit ctx: Context): TermSymbol = {
+      val sym = ctx.newSymbol(owner, name, Synthetic | Method, info, coord = pos).entered
+      if (sym.allOverriddenSymbols.exists(!_.is(Deferred))) sym.setFlag(Override)
+      sym
+    }
 
     /** Create an accessor unless one exists already, and replace the original
       *  access with a reference to the accessor.
@@ -73,14 +76,30 @@ abstract class AccessProxies {
 
       def refersToAccessed(sym: Symbol) = accessedBy.get(sym) == Some(accessed)
 
-      val accessorInfo =
+      val accessorClass = {
+        def owningClass(start: Symbol) =
+          start.ownersIterator.findSymbol(_.derivesFrom(accessed.owner))
+        val curCls = ctx.owner.enclosingClass
+        var owner = owningClass(curCls) //`orElse` owningClass(curCls.linkedClass)
+        if (!owner.exists) {
+          transforms.println(i"${curCls.ownersIterator.toList}%, %")
+          ctx.error(i"illegal access to protected ${accessed.showLocated} from $curCls", reference.pos)
+          owner = curCls
+        }
+        owner
+      }
+
+      val accessorRawInfo =
         if (onLHS) MethodType(accessed.info :: Nil, defn.UnitType)
         else accessed.info.ensureMethodic
+      val accessorInfo =
+        accessorRawInfo.asSeenFrom(accessorClass.thisType, accessed.owner)
       val accessorName = nameKind(accessed.name)
+
       val accessorSymbol =
-        accessed.owner.info.decl(accessorName).suchThat(refersToAccessed).symbol
+        accessorClass.info.decl(accessorName).suchThat(refersToAccessed).symbol
           .orElse {
-            val acc = newAccessorSymbol(accessed, accessorName, accessorInfo)
+            val acc = newAccessorSymbol(accessorClass, accessorName, accessorInfo, accessed.pos)
             accessedBy(acc) = accessed
             acc
           }

compiler/src/dotty/tools/dotc/transform/Erasure.scala

@@ -20,7 +20,6 @@ import typer.ProtoTypes._
 import typer.ErrorReporting._
 import core.TypeErasure._
 import core.Decorators._
-import core.NameKinds._
 import dotty.tools.dotc.ast.{Trees, tpd, untpd}
 import ast.Trees._
 import scala.collection.mutable.ListBuffer
@@ -327,23 +326,6 @@ object Erasure {
       if (tree.isTerm) erasedRef(tp) else valueErasure(tp)
     }
 
-    object ProtectedAccessors extends AccessProxies {
-      def getterName = ProtectedAccessorName
-      def setterName = ProtectedSetterName
-
-      val insert = new Insert {
-        def needsAccessor(sym: Symbol)(implicit ctx: Context): Boolean =
-          false &&
-          sym.isTerm && sym.is(Flags.Protected) &&
-          ctx.owner.enclosingPackageClass != sym.enclosingPackageClass &&
-          !ctx.owner.enclosingClass.derivesFrom(sym.owner) &&
-            { println(i"need protected acc $sym accessed from ${ctx.owner}"); assert(false); false }
-        }
-    }
-
-    override def addAccessorDefs(cls: Symbol, body: List[Tree])(implicit ctx: Context): List[Tree] =
-      ProtectedAccessors.addAccessorDefs(cls, body)
-
     override def promote(tree: untpd.Tree)(implicit ctx: Context): tree.ThisTree[Type] = {
       assert(tree.hasType)
       val erasedTp = erasedType(tree)
@@ -378,9 +360,6 @@ object Erasure {
       else
         super.typedLiteral(tree)
 
-    override def typedIdent(tree: untpd.Ident, pt: Type)(implicit ctx: Context): Tree =
-      ProtectedAccessors.insert.accessorIfNeeded(super.typedIdent(tree, pt))
-
     /** Type check select nodes, applying the following rewritings exhaustively
      *  on selections `e.m`, where `OT` is the type of the owner of `m` and `ET`
      *  is the erased type of the selection's original qualifier expression.
@@ -461,12 +440,9 @@ object Erasure {
         }
       }
 
-      ProtectedAccessors.insert.accessorIfNeeded(recur(typed(tree.qualifier, AnySelectionProto)))
+      recur(typed(tree.qualifier, AnySelectionProto))
     }
 
-    override def typedAssign(tree: untpd.Assign, pt: Type)(implicit ctx: Context): Tree =
-      ProtectedAccessors.insert.accessorIfNeeded(super.typedAssign(tree, pt))
-
     override def typedThis(tree: untpd.This)(implicit ctx: Context): Tree =
       if (tree.symbol == ctx.owner.lexicallyEnclosingClass || tree.symbol.isStaticOwner) promote(tree)
       else {

compiler/src/dotty/tools/dotc/transform/ExtensionMethods.scala

@@ -45,7 +45,10 @@ class ExtensionMethods extends MiniPhase with DenotTransformer with FullParamete
   /** the following two members override abstract members in Transform */
   override def phaseName: String = ExtensionMethods.name
 
-  override def runsAfter = Set(ElimRepeated.name)
+  override def runsAfter = Set(
+    ElimRepeated.name,
+    ProtectedAccessors.name  // protected accessors cannot handle code that is moved from class to companion object
+  )
 
   override def runsAfterGroupsOf = Set(FirstTransform.name) // need companion objects to exist
 

compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala

@@ -0,0 +1,74 @@
+package dotty.tools.dotc
+package transform
+
+import core.Contexts.Context
+import core.NameKinds._
+import core.Symbols._
+import core.Flags._
+import core.Decorators._
+import MegaPhase.MiniPhase
+import ast.Trees._
+
+/** Add accessors for all protected accesses. An accessor is needed if
+ *  according to the rules of the JVM a protected class member is not accesissible
+ *  from the point of access, but is accessible if the access is from an enclosing
+ *  class. In this point a public access method is placed in that enclosing class.
+ */
+class ProtectedAccessors extends MiniPhase {
+  import ast.tpd._
+
+  override def phaseName = ProtectedAccessors.name
+
+  object Accessors extends AccessProxies {
+    def getterName = ProtectedAccessorName
+    def setterName = ProtectedSetterName
+
+    val insert = new Insert {
+      def needsAccessor(sym: Symbol)(implicit ctx: Context): Boolean = {
+        def insideBoundary = {
+          if (sym.is(JavaDefined)) {
+            sym.is(JavaStatic) ||  // Java's static protected definitions are treated as public
+            ctx.owner.enclosingPackageClass == sym.enclosingPackageClass
+          }
+          else {
+            // For Scala-defined symbols we currently allow private and protected accesses
+            // from inner packages, and compensate by widening accessibility of such symbols to public.
+            // It would be good if we could revisit this at some point.
+            val boundary = sym.accessBoundary(sym.enclosingPackageClass)
+            ctx.owner.isContainedIn(boundary) || ctx.owner.isContainedIn(boundary.linkedClass)
+          }
+        }
+        sym.isTerm && sym.is(Protected) &&
+        !sym.owner.is(Trait) && // trait methods need to be handled specially, are currently always public
+        !insideBoundary &&
+        !ctx.owner.enclosingClass.derivesFrom(sym.owner)
+      }
+    }
+  }
+
+  override def prepareForAssign(tree: Assign)(implicit ctx: Context) = tree.lhs match {
+    case tree: RefTree if Accessors.insert.needsAccessor(tree.symbol) =>
+      ctx.fresh.setTree(tree)
+    case _ =>
+      ctx
+  }
+
+  def isLHS(tree: RefTree)(implicit ctx: Context) = ctx.tree match {
+    case Assign(lhs, _) => tree.symbol == lhs.symbol
+    case _ => false
+  }
+
+  override def transformIdent(tree: Ident)(implicit ctx: Context): Tree =
+    if (isLHS(tree)) tree else Accessors.insert.accessorIfNeeded(tree)
+
+  override def transformSelect(tree: Select)(implicit ctx: Context): Tree =
+    if (isLHS(tree)) tree else Accessors.insert.accessorIfNeeded(tree)
+
+  override def transformAssign(tree: Assign)(implicit ctx: Context): Tree =
+    Accessors.insert.accessorIfNeeded(tree)
+
+  override def transformTemplate(tree: Template)(implicit ctx: Context): Tree =
+    cpy.Template(tree)(body = Accessors.addAccessorDefs(tree.symbol.owner, tree.body))
+}
+object ProtectedAccessors { val name = "protectedAccessors" }
+

tests/neg/protectedacc.scala

@@ -0,0 +1,37 @@
+package p {
+  package a {
+
+    /** Test type parameters */
+    abstract class PolyA[a] {
+      protected def m(x: a): Unit;
+
+      class B {
+
+        trait Node {
+          def s: String = "";
+        }
+        protected def tie(x: Node): Unit = { x.s; () }
+      }
+    }
+  }
+
+  package b {
+    import a._;
+
+    abstract class X[T] extends PolyA[T] {
+
+      trait Inner extends B {
+        def self: T;
+        def self2: Node;
+        def getB: Inner;
+
+        m(self)
+
+        trait InnerInner {
+          val g = getB
+          g.tie(self2.asInstanceOf[g.Node]) // error: acess not permitted
+        }
+      }
+    }
+  }
+}

tests/run/protectedacc.scala

@@ -143,7 +143,7 @@ package p {
 
         trait InnerInner {
           val g = getB
-          g.tie(self2.asInstanceOf[g.Node])
+          //g.tie(self2.asInstanceOf[g.Node]) -- this would be a static error
         }
       }
     }