SI-6601 Close access loophole for value class constructors

ExtensionMethods marks private constructors of value classes as
notPRIVATE before pickling. When the pickler reads the flags of this
symbol, the anti-shift mechanism folds this into the regular PRIVATE
flag, so the class is pickled as though it was public all along. A
seprately compiled client can then call this constructor.

To remedy this, we must:

 - pickle `rawFlags`, rather than `flags`. This is symmetric with
   unpickling, which sets `rawFlags` with the value it reads.
 - Add `notPRIVATE` to the flagset `PickledFlags`.

We cannot make this change in a minor version, as the pickler
and unpickler must agree on `PickledFlags`.

I believe that this won't change the size of pickled flags
for the majority of symbols (ie, those without the notPRIVATE flag)
due to the variable length encoding in `writeLongNat`.

This also improves the situation for SI-6608. Reflection and
scalap (and, by extension, IntelliJ), no longer will see
as public methods that have had their access widened in
SuperAccessors (which is done selectively to support inlining
under separate compilation.)

8 files changed, 33 insertions, 2 deletions

diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/Pickler.scala b/src/compiler/scala/tools/nsc/symtab/classfile/Pickler.scala
index 140be0e17b..9b33ae8ba1 100644
--- a/src/compiler/scala/tools/nsc/symtab/classfile/Pickler.scala
+++ b/src/compiler/scala/tools/nsc/symtab/classfile/Pickler.scala
@@ -528,7 +528,7 @@ abstract class Pickler extends SubComponent {
     private def writeSymInfo(sym: Symbol) {
       writeRef(sym.name)
       writeRef(localizedOwner(sym))
-      writeLongNat((rawToPickledFlags(sym.flags & PickledFlags)))
+      writeLongNat((rawToPickledFlags(sym.rawflags & PickledFlags)))
       if (sym.hasAccessBoundary) writeRef(sym.privateWithin)
       writeRef(sym.info)
     }
diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
index b7221a78ec..b32fc6b977 100644
--- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -1553,6 +1553,8 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
             val bridges = addVarargBridges(currentOwner)
             checkAllOverrides(currentOwner)
             checkAnyValSubclass(currentOwner)
+            if (currentOwner.isDerivedValueClass)
+              currentOwner.primaryConstructor makeNotPrivate NoSymbol // SI-6601, must be done *after* pickler!
             if (bridges.nonEmpty) deriveTemplate(tree)(_ ::: bridges) else tree
 
           case dc@TypeTreeWithDeferredRefCheck() => abort("adapt should have turned dc: TypeTreeWithDeferredRefCheck into tpt: TypeTree, with tpt.original == dc")
diff --git a/src/reflect/scala/reflect/internal/Flags.scala b/src/reflect/scala/reflect/internal/Flags.scala
index 1987f34474..fe46a0471e 100644
--- a/src/reflect/scala/reflect/internal/Flags.scala
+++ b/src/reflect/scala/reflect/internal/Flags.scala
@@ -307,7 +307,11 @@ class Flags extends ModifierFlags {
   assert((OverloadedFlagsMask & FlagsNotPickled) == 0, flagsToString(OverloadedFlagsMask & FlagsNotPickled))
 
   /** These flags are pickled */
-  final val PickledFlags  = InitialFlags & ~FlagsNotPickled
+  final val PickledFlags  = (
+      (InitialFlags & ~FlagsNotPickled)
+    | notPRIVATE // for value class constructors (SI-6601), and private members referenced
+                 // in @inline-marked methods publicized in SuperAccessors (see SI-6608, e6b4204604)
+  )
 
   /** If we have a top-level class or module
    *  and someone asks us for a flag not in TopLevelPickledFlags,
diff --git a/test/files/neg/t6601.check b/test/files/neg/t6601.check
new file mode 100644
index 0000000000..1410e1b11a
--- /dev/null
+++ b/test/files/neg/t6601.check
@@ -0,0 +1,4 @@
+AccessPrivateConstructor_2.scala:2: error: constructor PrivateConstructor in class PrivateConstructor cannot be accessed in class AccessPrivateConstructor
+  new PrivateConstructor("") // Scalac should forbid accessing to the private constructor!
+  ^
+one error found
diff --git a/test/files/neg/t6601/AccessPrivateConstructor_2.scala b/test/files/neg/t6601/AccessPrivateConstructor_2.scala
new file mode 100644
index 0000000000..816bc10d79
--- /dev/null
+++ b/test/files/neg/t6601/AccessPrivateConstructor_2.scala
@@ -0,0 +1,3 @@
+class AccessPrivateConstructor {
+  new PrivateConstructor("") // Scalac should forbid accessing to the private constructor!
+}
diff --git a/test/files/neg/t6601/PrivateConstructor_1.scala b/test/files/neg/t6601/PrivateConstructor_1.scala
new file mode 100644
index 0000000000..f09d7ad068
--- /dev/null
+++ b/test/files/neg/t6601/PrivateConstructor_1.scala
@@ -0,0 +1 @@
+class PrivateConstructor private(val s: String) extends AnyVal
diff --git a/test/files/run/t6608.check b/test/files/run/t6608.check
new file mode 100644
index 0000000000..15628b322e
--- /dev/null
+++ b/test/files/run/t6608.check
@@ -0,0 +1 @@
+(C$$yyy,true)
diff --git a/test/files/run/t6608.scala b/test/files/run/t6608.scala
new file mode 100644
index 0000000000..2f956bfb35
--- /dev/null
+++ b/test/files/run/t6608.scala
@@ -0,0 +1,16 @@
+import reflect.runtime.universe
+
+class C {
+  private val yyy: Any = 1
+  @inline def foo = yyy
+}
+
+object Test extends App {
+  import universe._
+  val access = typeOf[C].declarations
+    .toList
+    .filter(_.name.toString.endsWith("yyy"))
+    .map(x => (x.name, x.isPrivate))
+  println(access.head)
+}
+