From 1a59c55ed5c6a0d53acd0259090cf57328748451 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 2 Mar 2016 20:51:49 +1000 Subject: Refactor transform of case apply in refchecks I've identified a dead call to `transformCaseApply` that seems to date back to Scala 2.6 vintages, in which case factory methods were a fictional companion method, rather than a real apply method in a companion module. This commit adds an abort in that code path to aide code review (if our test suite still passes, we know that I've removed dead code, rather than silently changing behaviour.) The following commit will remove it altogether I then inlined a slightly clunky abstraction in the two remaining calls to `transformCaseApply`. It was getting in the way of a clean fix to SI-9546, the topic of the next commit. --- test/files/run/t9546b.scala | 13 +++++++++++++ test/files/run/t9546c.scala | 13 +++++++++++++ test/files/run/t9546d.scala | 16 ++++++++++++++++ 3 files changed, 42 insertions(+) create mode 100644 test/files/run/t9546b.scala create mode 100644 test/files/run/t9546c.scala create mode 100644 test/files/run/t9546d.scala (limited to 'test') 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 + } +} + -- cgit v1.2.3 From fe5bd09861994734bc394813d069ea40c89d39de Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 2 Mar 2016 22:02:15 +1000 Subject: SI-9546 Fix regression in rewrite of case apply to constructor call In SI-9425, I disabled the rewrite of `CaseClass.apply(x)` to `new CaseClass(x)` if the constructor was was less accessible than the apply method. This solved a problem with spurious "constructor cannot be accessed" errors during refchecks for case classes with non-public constructors. However, for polymorphic case classes, refchecks was persistent, and even after refusing to transform the `TypeApply` within: CaseClass.apply[String]("") It *would* try again to transform the enclosing `Select`, a code path only intended for monomorphic case classes. The tree has a `PolyType`, which foiled the newly added accessibility check. I've modified the call to `isSimpleCaseApply` from the transform of `Select` nodes to exclude polymorphic apply's from being considered twice. --- src/compiler/scala/tools/nsc/typechecker/RefChecks.scala | 7 +++---- test/files/run/t9546.scala | 13 +++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 test/files/run/t9546.scala (limited to 'test') diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index f2607b6bde..517271e5eb 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -1571,7 +1571,9 @@ 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)) - if (isSimpleCaseApply(tree)) { + // SI-9546 isHigherKinded excludes generic case classes which are instead considered when transforming + // the enclosing `TypeApply`. + if (!tree.tpe.isHigherKinded && isSimpleCaseApply(tree)) { transformCaseApply(tree) } else { qual match { @@ -1725,9 +1727,6 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans case Ident(name) => checkUndesiredProperties(sym, tree.pos) - if (isSimpleCaseApply(tree)) { - abort("case factory methods are now always selected from prefix (since https://github.com/scala/scala/commit/76c06b4)") - } if (name != nme.WILDCARD && name != tpnme.WILDCARD_STAR) { assert(sym != NoSymbol, "transformCaseApply: name = " + name.debugString + " tree = " + tree + " / " + tree.getClass) //debug enterReference(tree.pos, sym) 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 + } +} + -- cgit v1.2.3 From f08282946647ec4049af965024b4638a4a55f5fd Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Thu, 3 Mar 2016 22:30:39 +1000 Subject: SI-9425 Fix a residual bug with multi-param-list case classes During code review for the fix for SI-9546, we found a corner case in the SI-9425 that remained broken. Using `finalResultType` peels off all the constructor param lists, and solves that problem. --- src/compiler/scala/tools/nsc/typechecker/RefChecks.scala | 13 ++++++++++--- test/files/run/t9546e.scala | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 test/files/run/t9546e.scala (limited to 'test') diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 517271e5eb..3b2e07bdbd 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -1522,7 +1522,7 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans sym.isCase && sym.name == nme.apply && isClassTypeAccessible(tree) && - !tree.tpe.resultType.typeSymbol.primaryConstructor.isLessAccessibleThan(tree.symbol) + !tree.tpe.finalResultType.typeSymbol.primaryConstructor.isLessAccessibleThan(tree.symbol) } private def transformCaseApply(tree: Tree) = { @@ -1571,8 +1571,15 @@ 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)) - // SI-9546 isHigherKinded excludes generic case classes which are instead considered when transforming - // the enclosing `TypeApply`. + // 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]), )`. 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 { 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 + } + +} -- cgit v1.2.3