package object misbehaves in the presence of classfiles

[scabug]

Multiple issues here. (And that's before I get to the question of whether one's companion object can be a package object -- although I assume the answer is "no", I find nothing unambiguous in the spec.)

package foo

class Bar { }
package object Bar { }

1. Non-idempotence of source compilation
2. Classes not even on the classpath fail the compilation

// first time it compiles, second time it doesn't.
% scalac3 p.scala 
% scalac3 p.scala 
p.scala:1: error: package foo contains object and package with same name: Bar
one of them needs to be removed from classpath
package foo
        ^
one error found

// it will not compile as long as those classfiles are visible from the
// current directory, even though "." is explicitly unreferenced.
% scalac3 -d /tmp -cp /asldkfjlasdfj p.scala 
p.scala:1: error: package foo contains object and package with same name: Bar
one of them needs to be removed from classpath
package foo
        ^
one error found

// delete them, it compiles.
% rm -rf foo
% scalac3 -d /tmp -cp /asldkfjlasdfj p.scala 
%

Edit: Another, possibly more serious, related issue follows. Example from mark harrah.

// demo/A.scala containing:
package demo
trait A

// demo/package.scala
package object demo extends demo.A

#!/usr/bin/env bash
#

rm -rf ./build
mkdir build
scalac -d build -cp build demo/A.scala demo/package.scala
rm -f build/demo/A.class
scalac -d build -cp build demo/A.scala demo/package.scala

It fails even when given all the source files it has just compiled:

error: error while loading demo, class file needed by package is missing.
reference type A of package demo refers to nonexisting symbol.
one error found

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-4695?orig=1
Reporter: @paulp

[scabug]

@adriaanm said:
could you have a look and bounce back if needed?

[scabug]

@som-snytt said:
This crash was more interesting before it was minimized; it's the same example, with the twist that if you compile the package object first, then compile the class with the same name, you get:

how can getCommonSuperclass() do its job if different class symbols get the same bytecode-level internal name: top/baz/package$

...
at scala.tools.nsc.backend.jvm.GenASM$JBuilder.javaName(GenASM.scala:611)
at scala.tools.nsc.backend.jvm.GenASM$JBuilder$$anonfun$addInnerClasses$4.apply(GenASM.scala:704)
at scala.tools.nsc.backend.jvm.GenASM$JBuilder$$anonfun$addInnerClasses$4.apply(GenASM.scala:698)
at scala.collection.immutable.List.foreach(List.scala:309)
at scala.tools.nsc.backend.jvm.GenASM$JBuilder.addInnerClasses(GenASM.scala:698)
at scala.tools.nsc.backend.jvm.GenASM$JPlainBuilder.genClass(GenASM.scala:1476)
at scala.tools.nsc.backend.jvm.GenASM$AsmPhase.run(GenASM.scala:180)

It must be serious when the message is a rhetorical question.

[scabug]

@retronym said:
Here's a means to partest this sort of problem:

retronym/scala@2.10.0-wip...retronym:topic/package-object-stub-flakiness

I'm going to take a look at this one.

[scabug]

@retronym said:
Regarding the case that I've seen a lot in practice:

package object demo extends demo.A

Martin suggests that if we took a strict approach we would outlaw this with a cyclic reference error, as it is conceptually the same manner as:

scala> object demo extends demo.A {
     |   trait A
     | }
<console>:7: error: illegal cyclic reference involving object demo
       object demo extends demo.A {
                           ^

This would break quite a lot of code, as nested types in the package are the most appealing for the package object to inherit. So we would have to deprecate it, or just leave it in a "use-at-your-own-risk" zone. We can tailor the error message for package objects to suggest a clean build.

However, if there is enough will to make this work, there might be a way. In this patch, I detect if any parents of a package module loaded from a classfile is missing (ie, is a stub symbol). If so, we pretend that we didn't see the package.class at all. At the conclusion of the run, we can check that the a package module we discarded was indeed reconstructed from sources.

retronym/scala@scala:2.10.0-wip...retronym:ticket/4695

I'm going to sit on this one for a while; comments are very welcome.

[scabug]

@harrah said:
If it helps add context, I put my sbt package object in the final subproject that aggregates everything. So, I'm personally ok with a restriction that the contents of package object x are not visible within x. I realize that this is probably a common use case in practice (if I remember correctly, my example above was a minimization of a user's code), but I figured I'd mention it as how I deal with package objects. It has the drawback that I haven't figured out how to build scaladoc that includes the package object (introduces cycles).

[scabug]

@odersky said:
I think it's best to be consistent with normal objects. My recommendation is that we should

1. Deprecate use of classes inside package p as base classes of package object p. The deprecation message should contain a note that says that such a pattern will lead to failures in incremental builds.

2. Instead of crashing, issue an error message that indicates that there's a cyclic reference, that this pattern has been deprecated, and that you should change your code or else do a clean build.

[scabug]

@adriaanm said:
upped to critical because we should add deprecation warnings as soon as we can in the release cycle -- if we decide to deprecate, that is

[scabug]

@paulp said:
Is there really any particular urgency? We can't meaningfully deprecate it before 2.11.0. Giving people warning in M3 or whatever would be a lovely gesture but hardly seems critical. I've spent a lot of time on this now and I don't think we need to deprecate, but it's not in pull-request shape yet.

[scabug]

@adriaanm said:
let's consider again whether to emit a warning now that M3 has suddenly turned into RC1

[scabug]

@retronym said:
My vote is to leave this one as is. SBT works around it by deleting package.class when recompiling the package object. One of my roadmap items for 2.12 was to spend a bit of time in package objects (detecting package objects from source before loading the one from the classpath, as Paul has prototyped)

[scabug]

@dwijnand said:
Semi-related, handing package objects in the incremental compiler was improved in sbt/sbt#2432.

[retronym]

s/wont-fix/have-fixed/

scala/scala#9661