Merge pull request #4597 from dotty-staging/change-protected-accessors

Change protected accessors

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/NameKinds.scala

@@ -354,8 +354,8 @@ object NameKinds {
 
   val SuperAccessorName = new PrefixNameKind(SUPERACCESSOR, "super$")
   val InitializerName = new PrefixNameKind(INITIALIZER, "initial$")
-  val ProtectedAccessorName = new PrefixNameKind(PROTECTEDACCESSOR, "protected$")
-  val ProtectedSetterName = new PrefixNameKind(PROTECTEDSETTER, "protected$set") // dubious encoding, kept for Scala2 compatibility
+  val ProtectedGetterName = new PrefixNameKind(PROTECTEDGETTER, "protected_get$")
+  val ProtectedSetterName = new PrefixNameKind(PROTECTEDSETTER, "protected_set$")
   val InlineGetterName = new PrefixNameKind(INLINEGETTER, "inline_get$")
   val InlineSetterName = new PrefixNameKind(INLINESETTER, "inline_set$")
 
@@ -389,9 +389,12 @@ object NameKinds {
     def infoString: String = "Signed"
   }
 
-  /** Possible name kinds of a method that comes from Scala2 pickling info. */
+  /** Possible name kinds of a method that comes from Scala2 pickling info.
+   *  and that need to be unmangled. Note: Scala2 protected accessors and setters
+   *  can be left mangled, so they are not included in thus list.
+   */
   val Scala2MethodNameKinds: List[NameKind] =
-    List(DefaultGetterName, ExtMethName, UniqueExtMethName, ProtectedAccessorName, ProtectedSetterName)
+    List(DefaultGetterName, ExtMethName, UniqueExtMethName)
 
   def simpleNameKindOfTag      : collection.Map[Int, ClassifiedNameKind] = simpleNameKinds
   def qualifiedNameKindOfTag   : collection.Map[Int, QualifiedNameKind]  = qualifiedNameKinds

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

@@ -15,6 +15,10 @@ object NameTags extends TastyFormat.NameTags {
                                 // outer accessor that will be filled in by ExplicitOuter.
                                 // <num> indicates the number of hops needed to select the outer field.
 
+  final val PROTECTEDGETTER = 24 // The name of a protected getter `protected_get$<name>` created by ProtectedAccessors.
+
+  final val PROTECTEDSETTER = 25 // The name of a protected setter `protected_set$<name>` created by ProtectedAccessors.
+
   final val INITIALIZER = 26    // A mixin initializer method
 
   final val AVOIDCLASH = 27     // Adds a suffix to avoid a name clash;
@@ -47,7 +51,9 @@ object NameTags extends TastyFormat.NameTags {
     case OUTERSELECT => "OUTERSELECT"
 
     case SUPERACCESSOR => "SUPERACCESSOR"
-    case PROTECTEDACCESSOR => "PROTECTEDACCESSOR"
+    case INLINEGETTER => "INLINEGETTER"
+    case INLINESETTER => "INLINESETTER"
+    case PROTECTEDGETTER => "PROTECTEDGETTER"
     case PROTECTEDSETTER => "PROTECTEDSETTER"
     case INITIALIZER => "INITIALIZER"
     case AVOIDCLASH => "AVOIDCLASH"

compiler/src/dotty/tools/dotc/core/tasty/TastyFormat.scala

@@ -39,8 +39,8 @@ Macro-format:
                   VARIANT           Length underlying_NameRef variance_Nat      // 0: Contravariant, 1: Covariant
 
                   SUPERACCESSOR     Length underlying_NameRef
-                  PROTECTEDACCESSOR Length underlying_NameRef
-                  PROTECTEDSETTER   Length underlying_NameRef
+                  INLINEGETTER      Length underlying_NameRef
+                  INLINESETTER      Length underlying_NameRef
                   OBJECTCLASS       Length underlying_NameRef
 
                   SIGNED            Length original_NameRef resultSig_NameRef paramSig_NameRef*
@@ -226,8 +226,8 @@ Standard Section: "Positions" Assoc*
 object TastyFormat {
 
   final val header = Array(0x5C, 0xA1, 0xAB, 0x1F)
-  val MajorVersion = 7
-  val MinorVersion = 1
+  val MajorVersion = 8
+  val MinorVersion = 0
 
   /** Tags used to serialize names */
   class NameTags {
@@ -252,18 +252,12 @@ object TastyFormat {
 
     final val SUPERACCESSOR = 20     // The name of a super accessor `super$name` created by SuperAccesors.
 
-    final val PROTECTEDACCESSOR = 21 // The name of a protected accessor `protected$<name>` created by SuperAccesors.
+    final val INLINEGETTER = 21    // The name of an inline getter `inline_get$name`
 
-    final val PROTECTEDSETTER = 22   // The name of a protected setter `protected$set<name>` created by SuperAccesors.
-                                     // This is a dubious encoding for its risk for ambiguity.
-                                     // It is kept for Scala-2 compatibility.
+    final val INLINESETTER = 22     // The name of an inline setter `inline_set$name`
 
     final val OBJECTCLASS = 23       // The name of an object class (or: module class) `<name>$`.
 
-    final val INLINEGETTER = 24     // The name of an inline getter `inline_get$name`
-
-    final val INLINESETTER = 25     // The name of an inline setter `inline_set$name`
-
     final val SIGNED = 63            // A pair of a name and a signature, used to idenitfy
                                      // possibly overloaded methods.
   }

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

@@ -14,12 +14,14 @@ 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.
  */
 abstract class AccessProxies {
   import ast.tpd._
+  import AccessProxies._
 
   def getterName: ClassifiedNameKind
   def setterName: ClassifiedNameKind
@@ -56,9 +58,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 +77,26 @@ abstract class AccessProxies {
 
       def refersToAccessed(sym: Symbol) = accessedBy.get(sym) == Some(accessed)
 
-      val accessorInfo =
+      var accessorClass = hostForAccessorOf(accessed: Symbol)
+      if (!accessorClass.exists) {
+        val curCls = ctx.owner.enclosingClass
+        transforms.println(i"${curCls.ownersIterator.toList}%, %")
+        ctx.error(i"illegal access to protected ${accessed.showLocated} from $curCls",
+          reference.pos)
+        accessorClass = curCls
+      }
+
+      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
           }
@@ -106,4 +122,8 @@ abstract class AccessProxies {
         tree
     }
   }
+}
+object AccessProxies {
+  def hostForAccessorOf(accessed: Symbol)(implicit ctx: Context): Symbol =
+    ctx.owner.ownersIterator.findSymbol(_.derivesFrom(accessed.owner))
 }

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
@@ -315,10 +314,6 @@ object Erasure {
       }
   }
 
-  /** The erasure typer.
-   *  Also inserts protected accessors where needed. This logic is placed here
-   *  since it is most naturally done in a macro transform.
-   */
   class Typer extends typer.ReTyper with NoChecking {
     import Boxing._
 
@@ -327,23 +322,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 +356,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 +436,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/PostTyper.scala

@@ -216,12 +216,10 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase
             case sel: Select =>
               val args1 = transform(args)
               val sel1 = transformSelect(sel, args1)
-              if (superAcc.isProtectedAccessor(sel1)) sel1 else cpy.TypeApply(tree1)(sel1, args1)
+              cpy.TypeApply(tree1)(sel1, args1)
             case _ =>
               super.transform(tree1)
           }
-        case tree @ Assign(sel: Select, _) =>
-          super.transform(superAcc.transformAssign(tree))
         case Inlined(call, bindings, expansion) =>
           // Leave only a call trace consisting of
           //  - a reference to the top-level class from which the call was inlined,

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

@@ -0,0 +1,88 @@
+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._
+import util.Property
+
+/** 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.
+ */
+object ProtectedAccessors {
+  val name = "protectedAccessors"
+
+  private val LHS = new Property.StickyKey[Unit]
+
+  /** Is the current context's owner inside the access boundary established by `sym`? */
+  def insideBoundaryOf(sym: Symbol)(implicit ctx: Context): Boolean = {
+    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)
+    }
+  }
+
+  /** Do we need a protected accessor if the current context's owner
+   *  is not in a subclass or subtrait of `sym`?
+   */
+  def needsAccessorIfNotInSubclass(sym: Symbol)(implicit ctx: Context): Boolean =
+    sym.isTerm && sym.is(Protected) &&
+    !sym.owner.is(Trait) && // trait methods need to be handled specially, are currently always public
+    !insideBoundaryOf(sym)
+
+   /** Do we need a protected accessor for accessing sym from the current context's owner? */
+   def needsAccessor(sym: Symbol)(implicit ctx: Context): Boolean =
+    needsAccessorIfNotInSubclass(sym) &&
+    !ctx.owner.enclosingClass.derivesFrom(sym.owner)
+}
+
+class ProtectedAccessors extends MiniPhase {
+  import ast.tpd._
+  import ProtectedAccessors._
+
+  override def phaseName = ProtectedAccessors.name
+
+  object Accessors extends AccessProxies {
+    def getterName = ProtectedGetterName
+    def setterName = ProtectedSetterName
+
+    val insert = new Insert {
+      def needsAccessor(sym: Symbol)(implicit ctx: Context) = ProtectedAccessors.needsAccessor(sym)
+    }
+  }
+
+  override def prepareForAssign(tree: Assign)(implicit ctx: Context) = {
+    tree.lhs match {
+      case lhs: RefTree if needsAccessor(lhs.symbol) => lhs.putAttachment(LHS, ())
+      case _ =>
+    }
+    ctx
+  }
+
+  private def isLHS(tree: RefTree) = tree.removeAttachment(LHS).isDefined
+
+  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))
+}