Skip to content

SI-8134 SI-5954 Fix companions in package object under separate comp.#3389

Merged
retronym merged 4 commits intoscala:masterfrom
retronym:ticket/8134-2
Feb 13, 2014
Merged

SI-8134 SI-5954 Fix companions in package object under separate comp.#3389
retronym merged 4 commits intoscala:masterfrom
retronym:ticket/8134-2

Conversation

@retronym
Copy link
Member

Review by @odersky

The tests cases enclosed exhibited two failures modes under
separate compilation.

  1. When a synthetic companion object for a case- or implicit-class
     defined in a package object is called for,
     `Namer#ensureCompanionObject` is used to check for an explicitly
     defined companion before decided to create a synthetic one.
     This lookup of an existing companion symbol by `companionObjectOf`
     would locate a symbol backed by a class file which was in the
     scope of the enclosing package class. Furthermore, because the
     owner of that symbol is the package object class that has now
     been noted as corresponding to a source file in the current
     run, the class-file backed module symbol is *also* deemed to
     be from the current run. (This logic is in `Run#compiles`.)

     Thinking the companion module already existed, no synthetic
     module was created, which would lead to a crash in extension
     methods, which needs to add methods to it.

  2. In cases when the code explicitly contains the companion pair,
     we still ran into problems in the backend whereby the class-file
     based and source-file based symbols for the module ended up in
     the same scope (of the package class). This tripped an assertion
     in `Symbol#companionModule`.

We get into these problems because of the eager manner in which
class-file based package object are opened in `openPackageModule`.
The members of the module are copied into the scope of the enclosing
package:

    scala> ScalaPackage.info.member(nme.List)
    res0: $r#59116.intp#45094.global#28436.Symbol#29451 = value List#2462

    scala> ScalaPackage.info.member(nme.PACKAGE).info.member(nme.List)
    res1: $r#59116.intp#45094.global#28436.Symbol#29451 = value List#2462

This seems to require a two-pronged defense:

  1. When we attach a pre-existing symbol for a package object symbol
     to the tree of its new source, unlink the "forwarder" symbols
     (its decls from the enclosing
     package class.

  2. In `Flatten`, in the spirit of `replaceSymbolInCurrentScope`,
     remove static member modules from the scope of the enclosing
     package object (aka `exitingFlatten(nestedModule.owner)`).

This commit also removes the warnings about defining companions
in package objects and converts those neg tests to pos (with
-Xfatal-warnings to prove they are warning free.)

Defining nested classes/objects in package objects still has
a drawback: you can't shift a class from the package to the
package object, or vice versa, in a binary compatible manner,
because of the `package$` prefix on the flattened name of
nested classes. For this reason, the `-Xlint` warning about
this remains. This issue is tracked as SI-4344.

However, if one heeds this warning and incrementatlly recompiles,
we no longer need to run into a DoubleDefinition error (which
was dressed up with a more specific diagnostic in SI-5760.)
The neg test case for that bug has been converted to a pos.
I noticed that the pos/5954d was tripping a println "assertion".
This stemmed from an inconsistency between
`TypeRef#{parents, baseTypeSeq}` for a package objects compiled
from source that also has a class file from a previous compilation
run.

I've elevated the println to a devWarning, and changed
`updatePosFlags`, the home of this evil symbol overwriting,
to invalidate the caches in the symbols info. Yuck.

I believe that this symptom is peculiar to package objects because
of the way that the completer for packages calls `parents` during
the Namer phase for package objects, before we switch the symbol
to represent the package-object-from-source. But it seems prudent
to defensively invalidate the caches for any symbol that finds its
way into `updatePosFlags`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

As the prophet once said:

  "'Tis better to never do at all than to have do and undo"

Let's try that in 2.12.
@retronym
Copy link
Member Author

retronym commented Feb 1, 2014

Review by @lrytz, perhaps?

@ghost ghost assigned lrytz Feb 1, 2014
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be done in the packageobjects phase to treat the same concern in one place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to be late: the namer has already noticed the stale members in ensureCompanionObject and decided it doesn't need to create a synthetic companion.

@lrytz
Copy link
Member

lrytz commented Feb 2, 2014

The changes LGTM but i don't understand yet how they fix the bug :-)
According to the stack trace, makeExtensionMethodSymbol fails to enter an extension method into the scope of a companion because it is EmptyScope. How does the example trigger that?

package object pkg {
  class AnyOps(val x: Any) extends AnyVal
}

@xeno-by
Copy link
Contributor

xeno-by commented Feb 6, 2014

@retronym ping

@retronym
Copy link
Member Author

retronym commented Feb 6, 2014

@lrytz makeExtensionMethodSymbol finds an empty scope because ensureCompanionObject decided not to create a synthetic companion object for the implicit class because companionSymbolOf mistakenly found the forwarder symbol in the scope of the enclosing package class. This was put there in openPackageModule, based on before we named the package object from source.

During the namer, the search for companions routes through scopes in enclosing contexts, rather then owner.info.decls. That's to avoid cycles. But that's why it is fooled into thinking a forwarder symbol in the package class info could be a companion object of an implicit class nested in a package object in source.

phew!

@retronym
Copy link
Member Author

ping @lrytz

@lrytz
Copy link
Member

lrytz commented Feb 13, 2014

LGTM.

Phew indeed. How about prohibiting class and object definitions inside package objects? Is there a ticket for that?

Sorry i didn't see the reply, I improved my gmail filters.

@retronym
Copy link
Member Author

Seeing as implicit classes can't be declared at the top level we almost encourage people to declare them in a package object.

I don't think there is anything fundamentally wrong with it, its just we were very light on testing for separate compilation.

https://issues.scala-lang.org/browse/SI-4344 tracks a desirable change, which is to give package-object nested classes/objects the same post-flatten names as eponymous top-level classes. This would mean you could move them in and out of the package object without breaking binary compatibility.

retronym added a commit that referenced this pull request Feb 13, 2014
SI-8134 SI-5954 Fix companions in package object under separate comp.
@retronym retronym merged commit c83e01d into scala:master Feb 13, 2014
@lrytz
Copy link
Member

lrytz commented Feb 13, 2014

Seeing as implicit classes can't be declared at the top level we almost encourage people to declare them in a package object.

OK. Maybe we could lift that restriction by putting the generated implicit def into a (synthetic) (package) object.

@retronym
Copy link
Member Author

Separate compilation makes that pretty hard.

It might have been cleaner to spec that the constructor of the implicit class is itself implicit, rather than desugaring to an implicit class+def. The desugaring was already the source of interesting bugs for things like implicit case- or value-classes, where you end up with a factory method and a companion with the same name. And if the companion is nested, it might later need a module accessor method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants