Skip to content
Browse files

SI-5954 Implementation restriction preventing companions in package objs

Companion objects (and thus also case classes) in package objects caused
an assert about an overloaded symbol when everything was compiled twice.
It's a hairy problem that doesn't fit in 2.10.1. So this fix adds an
implementation restriction. It also has a test to make sure the error
messages are clean and reasonably friendly, and does not catch other
things defined in package objects. The test includes a
commented out test in case somebody thinks they've solved the underlying
problem.

A handful of tests were falling afoul of the new implementation
restriction. I verified that they do in fact fail on second compile so
they aren't false casualties. But they do test real things we'd like
to work once the re-compile issue is fixed. So I added a -X flag to
disable the implementation restriction and made all the tests
accidentally clobbered by the restriction use that flag.
  • Loading branch information...
1 parent 5d65772 commit 3ef487ecb6733bfe3c13d89780ebcfc81f9a5ea0 @JamesIry JamesIry committed Jan 13, 2013
View
1 src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
@@ -174,6 +174,7 @@ trait ScalaSettings extends AbsScalaSettings
val etaExpandKeepsStar = BooleanSetting ("-Yeta-expand-keeps-star", "Eta-expand varargs methods to T* rather than Seq[T]. This is a temporary option to ease transition.")
val Yinvalidate = StringSetting ("-Yinvalidate", "classpath-entry", "Invalidate classpath entry before run", "")
val noSelfCheck = BooleanSetting ("-Yno-self-type-checks", "Suppress check for self-type conformance among inherited members.")
+ val companionsInPkgObjs = BooleanSetting("-Ycompanions-in-pkg-objs", "Allow companion objects and case classes in package objects. See issue SI-5954.")
val YvirtClasses = false // too embryonic to even expose as a -Y //BooleanSetting ("-Yvirtual-classes", "Support virtual classes")
val exposeEmptyPackage = BooleanSetting("-Yexpose-empty-package", "Internal only: expose the empty package.").internalOnly()
View
26 src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -1891,7 +1891,33 @@ trait Typers extends Modes with Adaptations with Tags {
})
}
val impl2 = finishMethodSynthesis(impl1, clazz, context)
+
+ // SI-5954. On second compile of a companion class contained in a package object we end up
+ // with some confusion of names which leads to having two symbols with the same name in the
+ // same owner. Until that can be straightened out we can't allow companion objects in package
+ // objects. But this code also tries to be friendly by distinguishing between case classes and
+ // user written companion pairs
+ def restrictPackageObjectMembers(mdef : ModuleDef) = for (m <- mdef.symbol.info.members) {
+ // ignore synthetic objects, because the "companion" object to a case class is synthetic and
+ // we only want one error per case class
+ if (!m.isSynthetic) {
+ // can't handle case classes in package objects
+ if (m.isCaseClass) pkgObjectRestriction(m, mdef, "case")
+ // can't handle companion class/object pairs in package objects
+ else if ((m.isClass && m.companionModule != NoSymbol && !m.companionModule.isSynthetic) ||
+ (m.isModule && m.companionClass != NoSymbol && !m.companionClass.isSynthetic))
+ pkgObjectRestriction(m, mdef, "companion")
+ }
+
+ def pkgObjectRestriction(m : Symbol, mdef : ModuleDef, restricted : String) = {
+ val pkgName = mdef.symbol.ownerChain find (_.isPackage) map (_.decodedName) getOrElse mdef.symbol.toString
+ context.error(if (m.pos.isDefined) m.pos else mdef.pos, s"implementation restriction: package object ${pkgName} cannot contain ${restricted} ${m}. Instead, ${m} should be placed directly in package ${pkgName}.")
+ }
+ }
+ if (!settings.companionsInPkgObjs.value && mdef.symbol.isPackageObject)
+ restrictPackageObjectMembers(mdef)
+
treeCopy.ModuleDef(mdef, typedMods, mdef.name, impl2) setType NoType
}
/** In order to override this in the TreeCheckers Typer so synthetics aren't re-added
View
2 src/reflect/scala/reflect/internal/settings/MutableSettings.scala
@@ -47,4 +47,6 @@ abstract class MutableSettings extends AbsSettings {
def XoldPatmat: BooleanSetting
def XnoPatmatAnalysis: BooleanSetting
def XfullLubs: BooleanSetting
+ def companionsInPkgObjs: BooleanSetting
+
}
View
1 src/reflect/scala/reflect/runtime/Settings.scala
@@ -43,6 +43,7 @@ private[reflect] class Settings extends MutableSettings {
val printtypes = new BooleanSetting(false)
val uniqid = new BooleanSetting(false)
val verbose = new BooleanSetting(false)
+ val companionsInPkgObjs = new BooleanSetting(false)
val Yrecursion = new IntSetting(0)
val maxClassfileName = new IntSetting(255)
View
16 test/files/neg/t5954.check
@@ -0,0 +1,16 @@
+t5954.scala:36: error: implementation restriction: package object A cannot contain case class D. Instead, class D should be placed directly in package A.
+ case class D()
+ ^
+t5954.scala:35: error: implementation restriction: package object A cannot contain companion object C. Instead, object C should be placed directly in package A.
+ object C
+ ^
+t5954.scala:34: error: implementation restriction: package object A cannot contain companion trait C. Instead, trait C should be placed directly in package A.
+ trait C
+ ^
+t5954.scala:33: error: implementation restriction: package object A cannot contain companion object B. Instead, object B should be placed directly in package A.
+ object B
+ ^
+t5954.scala:32: error: implementation restriction: package object A cannot contain companion class B. Instead, class B should be placed directly in package A.
+ class B
+ ^
+5 errors found
View
46 test/files/neg/t5954.scala
@@ -0,0 +1,46 @@
+// if you ever think you've fixed the underlying reason for the implementation restrictions
+// imposed by SI-5954, then here's a test that should pass with two "succes"es
+//
+//import scala.tools.partest._
+//
+//object Test extends DirectTest {
+// def code = ???
+//
+// def problemCode = """
+// package object A {
+// class B
+// object B
+// case class C()
+// }
+// """
+//
+// def compileProblemCode() = {
+// val classpath = List(sys.props("partest.lib"), testOutput.path) mkString sys.props("path.separator")
+// compileString(newCompiler("-cp", classpath, "-d", testOutput.path))(problemCode)
+// }
+//
+// def show() : Unit = {
+// for (i <- 0 until 2) {
+// compileProblemCode()
+// println(s"success ${i + 1}")
+// }
+// }
+//}
+
+package object A {
+ // these should be prevented by the implementation restriction
+ class B
+ object B
+ trait C
+ object C
+ case class D()
+ // all the rest of these should be ok
+ class E
+ object F
+ val g = "omg"
+ var h = "wtf"
+ def i = "lol"
+ type j = String
+ class K(val k : Int) extends AnyVal
+ implicit class L(val l : Int)
+}
View
1 test/files/pos/package-case.flags
@@ -0,0 +1 @@
+-Ycompanions-in-pkg-objs
View
1 test/files/pos/t2130-1.flags
@@ -0,0 +1 @@
+-Ycompanions-in-pkg-objs
View
1 test/files/pos/t2130-2.flags
@@ -0,0 +1 @@
+-Ycompanions-in-pkg-objs
View
1 test/files/pos/t3999b.flags
@@ -0,0 +1 @@
+-Ycompanions-in-pkg-objs
View
1 test/files/pos/t4052.flags
@@ -0,0 +1 @@
+-Ycompanions-in-pkg-objs

0 comments on commit 3ef487e

Please sign in to comment.
Something went wrong with that request. Please try again.