From d47a15b01557ca6c6416892a98d35324bf0a13d7 Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Thu, 20 Sep 2012 17:05:43 -0700 Subject: Pending tests for SI-5954, SI-6225, SI-5877, SI-4695. Which are all package object bugs. Plus one more test regarding package object overloading. --- test/pending/pos/overloading-boundaries.scala | 37 +++++++++++++++++++++++++++ test/pending/pos/t4695/T_1.scala | 4 +++ test/pending/pos/t4695/T_2.scala | 4 +++ test/pending/pos/t5877.scala | 5 ++++ test/pending/pos/t5954/T_1.scala | 8 ++++++ test/pending/pos/t5954/T_2.scala | 8 ++++++ test/pending/pos/t5954/T_3.scala | 8 ++++++ test/pending/pos/t6225.scala | 11 ++++++++ 8 files changed, 85 insertions(+) create mode 100644 test/pending/pos/overloading-boundaries.scala create mode 100644 test/pending/pos/t4695/T_1.scala create mode 100644 test/pending/pos/t4695/T_2.scala create mode 100644 test/pending/pos/t5877.scala create mode 100644 test/pending/pos/t5954/T_1.scala create mode 100644 test/pending/pos/t5954/T_2.scala create mode 100644 test/pending/pos/t5954/T_3.scala create mode 100644 test/pending/pos/t6225.scala diff --git a/test/pending/pos/overloading-boundaries.scala b/test/pending/pos/overloading-boundaries.scala new file mode 100644 index 0000000000..d2e9fdbb12 --- /dev/null +++ b/test/pending/pos/overloading-boundaries.scala @@ -0,0 +1,37 @@ +package bar { + object bippy extends (Double => String) { + def apply(x: Double): String = "Double" + } +} + +package object bar { + def bippy(x: Int, y: Int, z: Int) = "(Int, Int, Int)" +} + +object Test { + def main(args: Array[String]): Unit = { + println(bar.bippy(5.5d)) + println(bar.bippy(1, 2, 3)) + } +} + +/**** + +% scalac3 a.scala +a.scala:13: error: not enough arguments for method bippy: (x: Int, y: Int, z: Int)String. +Unspecified value parameters y, z. + println(bar.bippy(5.5d)) + ^ +one error found + +# Comment out the call to bar.bippy(5.5d) - compiles +% scalac3 a.scala + +# Compiles only from pure source though - if classes are present, fails. +% scalac3 a.scala +a.scala:2: error: bippy is already defined as method bippy in package object bar + object bippy extends (Double => String) { + ^ +one error found + +****/ diff --git a/test/pending/pos/t4695/T_1.scala b/test/pending/pos/t4695/T_1.scala new file mode 100644 index 0000000000..70fb1a7f21 --- /dev/null +++ b/test/pending/pos/t4695/T_1.scala @@ -0,0 +1,4 @@ +package foo + +class Bar { } +package object Bar { } diff --git a/test/pending/pos/t4695/T_2.scala b/test/pending/pos/t4695/T_2.scala new file mode 100644 index 0000000000..70fb1a7f21 --- /dev/null +++ b/test/pending/pos/t4695/T_2.scala @@ -0,0 +1,4 @@ +package foo + +class Bar { } +package object Bar { } diff --git a/test/pending/pos/t5877.scala b/test/pending/pos/t5877.scala new file mode 100644 index 0000000000..b77605f7f2 --- /dev/null +++ b/test/pending/pos/t5877.scala @@ -0,0 +1,5 @@ +package foo { } + +package object foo { + implicit class Foo(val s: String) { } +} diff --git a/test/pending/pos/t5954/T_1.scala b/test/pending/pos/t5954/T_1.scala new file mode 100644 index 0000000000..0064c596b6 --- /dev/null +++ b/test/pending/pos/t5954/T_1.scala @@ -0,0 +1,8 @@ +package p { + package base { + class X + } + package object base { + case class B() + } +} diff --git a/test/pending/pos/t5954/T_2.scala b/test/pending/pos/t5954/T_2.scala new file mode 100644 index 0000000000..0064c596b6 --- /dev/null +++ b/test/pending/pos/t5954/T_2.scala @@ -0,0 +1,8 @@ +package p { + package base { + class X + } + package object base { + case class B() + } +} diff --git a/test/pending/pos/t5954/T_3.scala b/test/pending/pos/t5954/T_3.scala new file mode 100644 index 0000000000..0064c596b6 --- /dev/null +++ b/test/pending/pos/t5954/T_3.scala @@ -0,0 +1,8 @@ +package p { + package base { + class X + } + package object base { + case class B() + } +} diff --git a/test/pending/pos/t6225.scala b/test/pending/pos/t6225.scala new file mode 100644 index 0000000000..d7dff3c419 --- /dev/null +++ b/test/pending/pos/t6225.scala @@ -0,0 +1,11 @@ +package library.x { + class X { + class Foo + implicit val foo = new Foo + } +} +package library { package object x extends X } +package app { + import library.x._ + object App { implicitly[Foo] } +} -- cgit v1.2.3 From b45a91fe228be063b9f9192cc135459f32d82ae0 Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Wed, 26 Sep 2012 12:11:48 -0700 Subject: Expanded an error message from the backend. --- src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala index c3fca13374..18e9ae620e 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala @@ -604,11 +604,18 @@ abstract class GenASM extends SubComponent with BytecodeWriters { val internalName = cachedJN.toString() val trackedSym = jsymbol(sym) reverseJavaName.get(internalName) match { - case None => + case Some(oldsym) if oldsym.exists && trackedSym.exists => + assert( + // In contrast, neither NothingClass nor NullClass show up bytecode-level. + (oldsym == trackedSym) || (oldsym == RuntimeNothingClass) || (oldsym == RuntimeNullClass), + s"""|Different class symbols have the same bytecode-level internal name: + | name: $internalName + | oldsym: ${oldsym.fullNameString} + | tracked: ${trackedSym.fullNameString} + """.stripMargin + ) + case _ => reverseJavaName.put(internalName, trackedSym) - case Some(oldsym) => - assert((oldsym == trackedSym) || (oldsym == RuntimeNothingClass) || (oldsym == RuntimeNullClass), // In contrast, neither NothingClass nor NullClass show up bytecode-level. - "how can getCommonSuperclass() do its job if different class symbols get the same bytecode-level internal name: " + internalName) } } -- cgit v1.2.3 From 83b5d4c0c9af462fc562c571f17dfcd00f47255d Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Wed, 26 Sep 2012 12:13:03 -0700 Subject: Comments explaining some brokenness in Namers. --- src/compiler/scala/tools/nsc/typechecker/Namers.scala | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala index df8eb9c6b9..55ec8bead4 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala @@ -161,6 +161,9 @@ trait Namers extends MethodSynthesis { else innerNamer } + // FIXME - this logic needs to be thoroughly explained + // and justified. I know it's wrong with repect to package + // objects, but I think it's also wrong in other ways. protected def conflict(newS: Symbol, oldS: Symbol) = ( ( !oldS.isSourceMethod || nme.isSetterName(newS.name) @@ -188,6 +191,19 @@ trait Namers extends MethodSynthesis { /** Enter symbol into given scope and return symbol itself */ def enterInScope(sym: Symbol, scope: Scope): Symbol = { + // FIXME - this is broken in a number of ways. + // + // 1) If "sym" allows overloading, that is not itself sufficient to skip + // the check, because "prev.sym" also must allow overloading. + // + // 2) There is nothing which reconciles a package's scope with + // the package object's scope. This is the source of many bugs + // with e.g. defining a case class in a package object. When + // compiling against classes, the class symbol is created in the + // package and in the package object, and the conflict is undetected. + // There is also a non-deterministic outcome for situations like + // an object with the same name as a method in the package object. + // allow for overloaded methods if (!allowsOverload(sym)) { val prev = scope.lookupEntry(sym.name) -- cgit v1.2.3 From e6f10b07d44f0ddde26246b4a41527a84eede81c Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Wed, 26 Sep 2012 12:14:13 -0700 Subject: Fixed SI-5604, selections on package objects. mkAttributedSelect, which creates a Select tree based on a symbol, has been a major source of package object bugs, because it has not been accurately identifying selections on package objects. When selecting foo.bar, if foo turns out to be a package object, the created Select tree must be foo.`package`.bar However mkAttributedSelect was only examining the owner of the symbol, which means it would work if the package object defined bar directly, but not if it inherited it. --- src/reflect/scala/reflect/internal/TreeGen.scala | 25 ++++++++++-- test/files/pos/t5604b/T_1.scala | 6 +++ test/files/pos/t5604b/T_2.scala | 6 +++ test/files/pos/t5604b/Test_1.scala | 7 ++++ test/files/pos/t5604b/Test_2.scala | 7 ++++ test/files/pos/t5604b/pack_1.scala | 5 +++ test/files/run/t5604.check | 8 ++++ test/files/run/t5604.scala | 50 ++++++++++++++++++++++++ 8 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 test/files/pos/t5604b/T_1.scala create mode 100644 test/files/pos/t5604b/T_2.scala create mode 100644 test/files/pos/t5604b/Test_1.scala create mode 100644 test/files/pos/t5604b/Test_2.scala create mode 100644 test/files/pos/t5604b/pack_1.scala create mode 100644 test/files/run/t5604.check create mode 100644 test/files/run/t5604.scala diff --git a/src/reflect/scala/reflect/internal/TreeGen.scala b/src/reflect/scala/reflect/internal/TreeGen.scala index ebf0998573..c1753fc5a1 100644 --- a/src/reflect/scala/reflect/internal/TreeGen.scala +++ b/src/reflect/scala/reflect/internal/TreeGen.scala @@ -172,10 +172,29 @@ abstract class TreeGen extends macros.TreeBuilder { if (qual.symbol != null && (qual.symbol.isEffectiveRoot || qual.symbol.isEmptyPackage)) mkAttributedIdent(sym) else { + // Have to recognize anytime a selection is made on a package + // so it can be rewritten to foo.bar.`package`.name rather than + // foo.bar.name if name is in the package object. + // TODO - factor out the common logic between this and + // the Typers method "isInPackageObject", used in typedIdent. + val qualsym = ( + if (qual.tpe ne null) qual.tpe.typeSymbol + else if (qual.symbol ne null) qual.symbol + else NoSymbol + ) + val needsPackageQualifier = ( + (sym ne null) + && qualsym.isPackage + && !sym.isDefinedInPackage + ) val pkgQualifier = - if (sym != null && sym.owner.isPackageObjectClass && sym.effectiveOwner == qual.tpe.typeSymbol) { - val obj = sym.owner.sourceModule - Select(qual, nme.PACKAGE) setSymbol obj setType singleType(qual.tpe, obj) + if (needsPackageQualifier) { + // The owner of a symbol which requires package qualification may be the + // package object iself, but it also could be any superclass of the package + // object. In the latter case, we must go through the qualifier's info + // to obtain the right symbol. + val packageObject = if (sym.owner.isModuleClass) sym.owner.sourceModule else qual.tpe member nme.PACKAGE + Select(qual, nme.PACKAGE) setSymbol packageObject setType singleType(qual.tpe, packageObject) } else qual diff --git a/test/files/pos/t5604b/T_1.scala b/test/files/pos/t5604b/T_1.scala new file mode 100644 index 0000000000..179dcb10c6 --- /dev/null +++ b/test/files/pos/t5604b/T_1.scala @@ -0,0 +1,6 @@ +// sandbox/t5604/T.scala +package t6504 + +trait T { + def foo: Boolean = false +} diff --git a/test/files/pos/t5604b/T_2.scala b/test/files/pos/t5604b/T_2.scala new file mode 100644 index 0000000000..179dcb10c6 --- /dev/null +++ b/test/files/pos/t5604b/T_2.scala @@ -0,0 +1,6 @@ +// sandbox/t5604/T.scala +package t6504 + +trait T { + def foo: Boolean = false +} diff --git a/test/files/pos/t5604b/Test_1.scala b/test/files/pos/t5604b/Test_1.scala new file mode 100644 index 0000000000..f7c58ebe83 --- /dev/null +++ b/test/files/pos/t5604b/Test_1.scala @@ -0,0 +1,7 @@ +// sandbox/t5604/Test.scala +package t6504 + +object Test { + def blerg1(a: Any): Any = if (foo) blerg1(0) + def blerg2(a: Any): Any = if (t6504.foo) blerg2(0) +} diff --git a/test/files/pos/t5604b/Test_2.scala b/test/files/pos/t5604b/Test_2.scala new file mode 100644 index 0000000000..f7c58ebe83 --- /dev/null +++ b/test/files/pos/t5604b/Test_2.scala @@ -0,0 +1,7 @@ +// sandbox/t5604/Test.scala +package t6504 + +object Test { + def blerg1(a: Any): Any = if (foo) blerg1(0) + def blerg2(a: Any): Any = if (t6504.foo) blerg2(0) +} diff --git a/test/files/pos/t5604b/pack_1.scala b/test/files/pos/t5604b/pack_1.scala new file mode 100644 index 0000000000..f50d568bfa --- /dev/null +++ b/test/files/pos/t5604b/pack_1.scala @@ -0,0 +1,5 @@ +// sandbox/t5604/pack.scala +package t6504 + +object `package` extends T { +} diff --git a/test/files/run/t5604.check b/test/files/run/t5604.check new file mode 100644 index 0000000000..53a2fc8894 --- /dev/null +++ b/test/files/run/t5604.check @@ -0,0 +1,8 @@ +long +double +long +double +long +double +long +double diff --git a/test/files/run/t5604.scala b/test/files/run/t5604.scala new file mode 100644 index 0000000000..a06c8aab3e --- /dev/null +++ b/test/files/run/t5604.scala @@ -0,0 +1,50 @@ +// a.scala +// Fri Jan 13 11:31:47 PST 2012 + +package foo { + object regular extends Duh { + def buh(n: Long) = println("long") + def buh(n: Double) = println("double") + } + class regular { + import regular._ + + duh(33L) + duh(3.0d) + foo.regular.duh(33L) + foo.regular.duh(3.0d) + buh(66L) + buh(6.0d) + foo.regular.buh(66L) + foo.regular.buh(6.0d) + } + + trait Duh { + def duh(n: Long) = println("long") + def duh(n: Double) = println("double") + } + package object bar extends Duh { + def buh(n: Long) = println("long") + def buh(n: Double) = println("double") + } + package bar { + object Main { + def main(args:Array[String]) { + duh(33L) + duh(3.0d) + foo.bar.duh(33L) + foo.bar.duh(3.0d) + buh(66L) + buh(6.0d) + foo.bar.buh(66L) + foo.bar.buh(6.0d) + } + } + } +} + +object Test { + def main(args: Array[String]): Unit = { + foo.bar.Main.main(null) + } +} -- cgit v1.2.3 From 97ede5a64a60c739f03619f9c0d7e2bf88a97207 Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Wed, 26 Sep 2012 12:19:50 -0700 Subject: Simplifications in typedIdent. These accompany the changes to mkAttributedSelect in the prior commit, and document additional brokenness with package objects which stillr emains. --- .../scala/tools/nsc/typechecker/Typers.scala | 93 ++++++++++++---------- 1 file changed, 51 insertions(+), 42 deletions(-) diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 2344e71883..6e222459c9 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -605,23 +605,31 @@ trait Typers extends Modes with Adaptations with Tags { } /** Is `sym` defined in package object of package `pkg`? + * Since sym may be defined in some parent of the package object, + * we cannot inspect its owner only; we have to go through the + * info of the package object. However to avoid cycles we'll check + * what other ways we can before pushing that way. */ private def isInPackageObject(sym: Symbol, pkg: Symbol) = { - def isInPkgObj(sym: Symbol) = - !sym.owner.isPackage && { - sym.owner.isPackageObjectClass && - sym.owner.owner == pkg || - pkg.isInitialized && { - // need to be careful here to not get a cyclic reference during bootstrap - val pkgobj = pkg.info.member(nme.PACKAGEkw) - pkgobj.isInitialized && - (pkgobj.info.member(sym.name).alternatives contains sym) - } + val pkgClass = if (pkg.isTerm) pkg.moduleClass else pkg + def matchesInfo = ( + pkg.isInitialized && { + // need to be careful here to not get a cyclic reference during bootstrap + val module = pkg.info member nme.PACKAGEkw + module.isInitialized && (module.info.member(sym.name).alternatives contains sym) } - pkg.isPackageClass && { + ) + def isInPkgObj(sym: Symbol) = ( + !sym.isPackage + && !sym.owner.isPackageClass + && (sym.owner ne NoSymbol) + && (sym.owner.owner == pkgClass || matchesInfo) + ) + + pkgClass.isPackageClass && ( if (sym.isOverloaded) sym.alternatives forall isInPkgObj else isInPkgObj(sym) - } + ) } /** Post-process an identifier or selection node, performing the following: @@ -4768,6 +4776,22 @@ trait Typers extends Modes with Adaptations with Tags { defSym = rootMirror.EmptyPackageClass.tpe.nonPrivateMember(name) defSym != NoSymbol } + def correctForPackageObject(sym: Symbol): Symbol = { + if (sym.isTerm && isInPackageObject(sym, pre.typeSymbol)) { + val sym1 = pre member sym.name + if ((sym1 eq NoSymbol) || (sym eq sym1)) sym else { + qual = gen.mkAttributedQualifier(pre) + log(s""" + | !!! Overloaded package object member resolved incorrectly. + | prefix: $pre + | Discarded: ${sym.defString} + | Using: ${sym1.defString} + """.stripMargin) + sym1 + } + } + else sym + } def startingIdentContext = ( // ignore current variable scope in patterns to enforce linearity if ((mode & (PATTERNmode | TYPEPATmode)) == 0) context @@ -4779,11 +4803,11 @@ trait Typers extends Modes with Adaptations with Tags { // which are methods (note: if we don't do that // case x :: xs in class List would return the :: method) // unless they are stable or are accessors (the latter exception is for better error messages). - def qualifies(sym: Symbol): Boolean = { - sym.hasRawInfo && // this condition avoids crashing on self-referential pattern variables - reallyExists(sym) && - ((mode & PATTERNmode | FUNmode) != (PATTERNmode | FUNmode) || !sym.isSourceMethod || sym.hasFlag(ACCESSOR)) - } + def qualifies(sym: Symbol): Boolean = ( + sym.hasRawInfo // this condition avoids crashing on self-referential pattern variables + && reallyExists(sym) + && ((mode & PATTERNmode | FUNmode) != (PATTERNmode | FUNmode) || !sym.isSourceMethod || sym.hasFlag(ACCESSOR)) + ) if (defSym == NoSymbol) { var defEntry: ScopeEntry = null // the scope entry of defSym, if defined in a local scope @@ -4791,32 +4815,17 @@ trait Typers extends Modes with Adaptations with Tags { var cx = startingIdentContext while (defSym == NoSymbol && cx != NoContext && (cx.scope ne null)) { // cx.scope eq null arises during FixInvalidSyms in Duplicators pre = cx.enclClass.prefix + // !!! FIXME. This call to lookupEntry is at the root of all the + // bad behavior with overloading in package objects. lookupEntry + // just takes the first symbol it finds in scope, ignoring the rest. + // When a selection on a package object arrives here, the first + // overload is always chosen. "correctForPackageObject" exists to + // undo that decision. Obviously it would be better not to do it in + // the first place; however other things seem to be tied to obtaining + // that ScopeEntry, specifically calculating the nesting depth. defEntry = cx.scope.lookupEntry(name) - if ((defEntry ne null) && qualifies(defEntry.sym)) { - // Right here is where SI-1987, overloading in package objects, can be - // seen to go wrong. There is an overloaded symbol, but when referring - // to the unqualified identifier from elsewhere in the package, only - // the last definition is visible. So overloading mis-resolves and is - // definition-order dependent, bad things. See run/t1987.scala. - // - // I assume the actual problem involves how/where these symbols are entered - // into the scope. But since I didn't figure out how to fix it that way, I - // catch it here by looking up package-object-defined symbols in the prefix. - if (isInPackageObject(defEntry.sym, pre.typeSymbol)) { - defSym = pre.member(defEntry.sym.name) - if (defSym ne defEntry.sym) { - qual = gen.mkAttributedQualifier(pre) - log(s""" - | !!! Overloaded package object member resolved incorrectly. - | prefix: $pre - | Discarded: ${defEntry.sym.defString} - | Using: ${defSym.defString} - """.stripMargin) - } - } - else - defSym = defEntry.sym - } + if ((defEntry ne null) && qualifies(defEntry.sym)) + defSym = correctForPackageObject(defEntry.sym) else { cx = cx.enclClass val foundSym = pre.member(name) filter qualifies -- cgit v1.2.3