Merge pull request #5002 from retronym/ticket/9546

SI-9546 Fix regression in rewrite of case apply to constructor call

6 files changed, 107 insertions, 29 deletions

diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
index c5abd756f8..3b2e07bdbd 100644
--- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -1503,9 +1503,8 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
       }
     }
 
-    private def transformCaseApply(tree: Tree, ifNot: => Unit) = {
+    private def isSimpleCaseApply(tree: Tree): Boolean = {
       val sym = tree.symbol
-
       def isClassTypeAccessible(tree: Tree): Boolean = tree match {
         case TypeApply(fun, targs) =>
           isClassTypeAccessible(fun)
@@ -1513,31 +1512,26 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
           ( // SI-4859 `CaseClass1().InnerCaseClass2()` must not be rewritten to `new InnerCaseClass2()`;
             //          {expr; Outer}.Inner() must not be rewritten to `new Outer.Inner()`.
             treeInfo.isQualifierSafeToElide(module) &&
-            // SI-5626 Classes in refinement types cannot be constructed with `new`. In this case,
-            // the companion class is actually not a ClassSymbol, but a reference to an abstract type.
-            module.symbol.companionClass.isClass
-          )
+              // SI-5626 Classes in refinement types cannot be constructed with `new`. In this case,
+              // the companion class is actually not a ClassSymbol, but a reference to an abstract type.
+              module.symbol.companionClass.isClass
+            )
       }
 
-      val doTransform =
-        sym.isSourceMethod &&
+      sym.isSourceMethod &&
         sym.isCase &&
         sym.name == nme.apply &&
         isClassTypeAccessible(tree) &&
-        !tree.tpe.resultType.typeSymbol.primaryConstructor.isLessAccessibleThan(tree.symbol)
+        !tree.tpe.finalResultType.typeSymbol.primaryConstructor.isLessAccessibleThan(tree.symbol)
+    }
 
-      if (doTransform) {
-        tree foreach {
-          case i@Ident(_) =>
-            enterReference(i.pos, i.symbol) // SI-5390 need to `enterReference` for `a` in `a.B()`
-          case _ =>
-        }
-        toConstructor(tree.pos, tree.tpe)
-      }
-      else {
-        ifNot
-        tree
+    private def transformCaseApply(tree: Tree) = {
+      tree foreach {
+        case i@Ident(_) =>
+          enterReference(i.pos, i.symbol) // SI-5390 need to `enterReference` for `a` in `a.B()`
+        case _ =>
       }
+      toConstructor(tree.pos, tree.tpe)
     }
 
     private def transformApply(tree: Apply): Tree = tree match {
@@ -1577,12 +1571,24 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
         // term should have been eliminated by super accessors
         assert(!(qual.symbol.isTrait && sym.isTerm && mix == tpnme.EMPTY), (qual.symbol, sym, mix))
 
-      transformCaseApply(tree,
+      // Rewrite eligible calls to monomorphic case companion apply methods to the equivalent constructor call.
+      //
+      // Note: for generic case classes the rewrite needs to be handled at the enclosing `TypeApply` to transform
+      // `TypeApply(Select(C, apply), targs)` to `Select(New(C[targs]), <init>)`. In case such a `TypeApply`
+      // was deemed ineligible for transformation (e.g. the case constructor was private), the refchecks transform
+      // will recurse to this point with `Select(C, apply)`, which will have a type `[T](...)C[T]`.
+      //
+      // We don't need to perform the check on the Select node, and `!isHigherKinded will guard against this
+      // redundant (and previously buggy, SI-9546) consideration.
+      if (!tree.tpe.isHigherKinded && isSimpleCaseApply(tree)) {
+        transformCaseApply(tree)
+      } else {
         qual match {
           case Super(_, mix)  => checkSuper(mix)
           case _              =>
         }
-      )
+        tree
+      }
     }
     private def transformIf(tree: If): Tree = {
       val If(cond, thenpart, elsepart) = tree
@@ -1706,7 +1712,10 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
 
           case TypeApply(fn, args) =>
             checkBounds(tree, NoPrefix, NoSymbol, fn.tpe.typeParams, args map (_.tpe))
-            transformCaseApply(tree, ())
+            if (isSimpleCaseApply(tree))
+              transformCaseApply(tree)
+            else
+              tree
 
           case x @ Apply(_, _)  =>
             transformApply(x)
@@ -1725,12 +1734,11 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
 
           case Ident(name) =>
             checkUndesiredProperties(sym, tree.pos)
-            transformCaseApply(tree,
-              if (name != nme.WILDCARD && name != tpnme.WILDCARD_STAR) {
-                assert(sym != NoSymbol, "transformCaseApply: name = " + name.debugString + " tree = " + tree + " / " + tree.getClass) //debug
-                enterReference(tree.pos, sym)
-              }
-            )
+            if (name != nme.WILDCARD && name != tpnme.WILDCARD_STAR) {
+              assert(sym != NoSymbol, "transformCaseApply: name = " + name.debugString + " tree = " + tree + " / " + tree.getClass) //debug
+              enterReference(tree.pos, sym)
+            }
+            tree
 
           case x @ Select(_, _) =>
             transformSelect(x)
diff --git a/test/files/run/t9546.scala b/test/files/run/t9546.scala
new file mode 100644
index 0000000000..7016881084
--- /dev/null
+++ b/test/files/run/t9546.scala
@@ -0,0 +1,13 @@
+package foo {
+  case class Opt[A] private[foo](val get: A) extends AnyVal
+  object Opt {
+    def mkOpt = Opt("")
+  }
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    foo.Opt.mkOpt
+  }
+}
+
diff --git a/test/files/run/t9546b.scala b/test/files/run/t9546b.scala
new file mode 100644
index 0000000000..0b4d2d3fe5
--- /dev/null
+++ b/test/files/run/t9546b.scala
@@ -0,0 +1,13 @@
+package foo {
+  case class Opt[A](val get: A) extends AnyVal {
+  }
+  object Opt {
+    def mkOpt = Opt("")
+  }
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    foo.Opt.mkOpt
+  }
+}
diff --git a/test/files/run/t9546c.scala b/test/files/run/t9546c.scala
new file mode 100644
index 0000000000..ea6a5a36b4
--- /dev/null
+++ b/test/files/run/t9546c.scala
@@ -0,0 +1,13 @@
+package foo {
+  case class Opt[A] private[foo](val get: A)
+  object Opt {
+    def mkOpt = Opt("")
+  }
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    foo.Opt.mkOpt
+  }
+}
+
diff --git a/test/files/run/t9546d.scala b/test/files/run/t9546d.scala
new file mode 100644
index 0000000000..00bf37dc18
--- /dev/null
+++ b/test/files/run/t9546d.scala
@@ -0,0 +1,16 @@
+class X {
+  def test: Any = {
+    object Opt {
+      def mkOpt = Opt("")
+    }
+    case class Opt[A] private[X](val get: A)
+    Opt.mkOpt
+  }
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    new X().test
+  }
+}
+
diff --git a/test/files/run/t9546e.scala b/test/files/run/t9546e.scala
new file mode 100644
index 0000000000..b19d0871aa
--- /dev/null
+++ b/test/files/run/t9546e.scala
@@ -0,0 +1,15 @@
+case class A private (x: Int)
+case class B private (x: Int)(y: Int)
+
+class C {
+  def f = A(1)
+  def g = B(1)(2) // was: constructor B in class B cannot be accessed in class C
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    new C().f
+    new C().g
+  }
+
+}