Drop empty companion objects #1075

odersky · 2016-02-11T08:35:42Z

@DarkDimius

Add operations to NameOps to detect and produce names for lazy locals. @DarkDimius Maybe there is already another way to do this? I could not find it.

The underlying problem on MacOS/Windows remains: We have a class `B` and an object `b` in the same scope. We used to get a conflict on `B$/b$` because we created an empty companion object for `B`. Now we get a conflict for `B/b`, because the `b` object creates to classes: `b.class` an `b$.class` and `b.class` clashes with `B.class`.

If the object was explicitly written, it might be referenced, even if it is empty.

odersky · 2016-02-11T16:46:19Z

src/dotty/tools/dotc/transform/LazyVals.scala

@@ -367,7 +367,7 @@ class LazyVals extends MiniPhaseTransform with IdentityDenotTransformer {
            appendOffsetDefs += (companion.moduleClass -> new OffsetInfo(List(offsetTree), ord))
        }

-        val containerName = ctx.freshName(x.name ++ StdNames.nme.LAZY_LOCAL).toTermName


@DarkDimius This is just DRY in order not to do the same low-level name plumbing over and over again.

odersky · 2016-02-11T16:50:25Z

@DarkDimius This was unexpectedly messy. It makes me wonder whether having synthesized companion objects is worth it. Could we not just use static members everywhere? We'd shed a ton
of complexity that way.

DarkDimius · 2016-02-12T22:37:14Z

Could we not just use static members everywhere?

I'm not sure I understood what you're proposing. Are you proposing to deprecate value classes and revert to previous implementation of lazy vals?

odersky · 2016-02-12T23:16:30Z

No, I am proposing neither of that.

odersky · 2016-02-13T10:02:22Z

I mean: Instead putting things in companion objects, make them static members of their class.

DarkDimius · 2016-02-13T14:01:29Z

@odersky, it seems to me that instances of synthetic objects are never
passed around.

I guess we can compile members of synthetic objects into static methods on
class itself, by using scala/docs.scala-lang#491.
If all members of synthetic object are marked static we're free not to emit
the $ class.

On 13 February 2016 11:02:35 odersky [email protected] wrote:

I mean: Instead putting things in companion objects, make them static
members of their class.

Reply to this email directly or view it on GitHub:
#1075 (comment)

odersky · 2016-02-13T14:38:16Z

Yes, but why do we put synthetic members into companion objects in the
first place? Creating synthetic companion objects for all classes is a
minor pain and getting rid of them again is a huge pain. Why do it?

On Sat, Feb 13, 2016 at 3:01 PM, Dmitry Petrashko [email protected]
wrote:

@odersky, it seems to me that instances of synthetic objects are never
passed around.

I guess we can compile members of synthetic objects into static methods on
class itself, by using scala/docs.scala-lang#491.

If all members of synthetic object are marked static we're free not to
emit
the $ class.

On 13 February 2016 11:02:35 odersky [email protected] wrote:

I mean: Instead putting things in companion objects, make them static
members of their class.

Reply to this email directly or view it on GitHub:
#1075 (comment)

—
Reply to this email directly or view it on GitHub
#1075 (comment).

Martin Odersky
EPFL

DarkDimius · 2016-02-13T14:56:17Z

Creating synthetic companion objects for all classes is a
minor pain and getting rid of them again is a huge pain.

I believe it can be implemented with less pain using functionality from
scala/docs.scala-lang#491

why do we put synthetic members into companion objects in the
first place?

Because it allows to ensure correct scoping using Ycheck, because those
methods can be called without an instance of the class. etc: those methods
do not behave as-if they were members of the class.

I'm convinced that's its fundamentally better to handle them entirely in
backend instead of special-casing them in ycheck, tpd, type-assigner,
lambdalift, flatten and increasing overall complexity. Even more given that
we can reuse implementation needed for
scala/docs.scala-lang#491

I would have agreed if we had full control of code that we make static.
This is true for lazy Vals, but false for value classes. Given this, I'd
prefer to have Ycheck fully working, as bodies of methods of value classes
that are lifted to static can contain arbitrarily complex code.

I have a simpler proposal, though less principled than @static: we can
allow phases to register predicates that apply to classes and say if
companion module is needed. Such predicates will be registered by lazy vals
and value classes phases.

On 13 February 2016 15:38:25 odersky [email protected] wrote:

Yes, but why do we put synthetic members into companion objects in the
first place? Creating synthetic companion objects for all classes is a
minor pain and getting rid of them again is a huge pain. Why do it?

On Sat, Feb 13, 2016 at 3:01 PM, Dmitry Petrashko [email protected]
wrote:

@odersky, it seems to me that instances of synthetic objects are never
passed around.

I guess we can compile members of synthetic objects into static methods on
class itself, by using scala/docs.scala-lang#491.

If all members of synthetic object are marked static we're free not to
emit
the $ class.

On 13 February 2016 11:02:35 odersky [email protected] wrote:

I mean: Instead putting things in companion objects, make them static
members of their class.

Reply to this email directly or view it on GitHub:
#1075 (comment)

—
Reply to this email directly or view it on GitHub
#1075 (comment).

Martin Odersky
EPFL

Reply to this email directly or view it on GitHub:
#1075 (comment)

odersky · 2016-02-16T09:38:50Z

OK, I understand the arguments. Alternatives are not necessarily better. Let's get this in, it is quite workable as is.

DarkDimius · 2016-02-16T09:40:14Z

Agreed. LGTM

Drop empty companion objects

DarkDimius · 2016-02-18T10:23:02Z

src/dotty/tools/dotc/transform/DropEmptyCompanions.scala

+      case TypeDef(_, impl: Template) if tree.symbol.is(SyntheticModule) &&
+        tree.symbol.companionClass.exists &&
+        impl.body.forall(_.symbol.isPrimaryConstructor) =>
+        println(i"removing ${tree.symbol}")


println left in code.
I'll make a pr to remove it.

It's already removed in another PR of mine. So it should go away by itself
soon.

On Thu, Feb 18, 2016 at 11:23 AM, Dmitry Petrashko <[email protected]

wrote:

In src/dotty/tools/dotc/transform/DropEmptyCompanions.scala
#1075 (comment):

* level, so we know that all objects moved by LambdaLift and Flatten have arrived

* at their destination.

*/
+class DropEmptyCompanions extends MiniPhaseTransform { thisTransform =>

import ast.tpd._

override def phaseName = "dropEmpty"

override def runsAfter: Set[Class[_ <: Phase]] = Set(classOf[Flatten])

override def transformPackageDef(pdef: PackageDef)(implicit ctx: Context, info: TransformerInfo) = {

/** Is tree an empty companion object? */

def isEmptyCompanion(tree: Tree) = tree match {

case TypeDef(_, impl: Template) if tree.symbol.is(SyntheticModule) &&

tree.symbol.companionClass.exists &&

impl.body.forall(_.symbol.isPrimaryConstructor) =>

println(i"removing ${tree.symbol}")

println left in code.
I'll make a pr to remove it.

—
Reply to this email directly or view it on GitHub
https://github.com/lampepfl/dotty/pull/1075/files#r53295488.

Martin Odersky
EPFL

odersky added 5 commits February 11, 2016 09:16

Abstract out lazy local names somewhat

c16ead8

Add operations to NameOps to detect and produce names for lazy locals. @DarkDimius Maybe there is already another way to do this? I could not find it.

New phase to drop empty companion objects

df7d891

Only remove synthetic companion objects

1839369

If the object was explicitly written, it might be referenced, even if it is empty.

Fix problems in DropEmptyCompanions

d3c3317

odersky reviewed Feb 11, 2016
View reviewed changes

DarkDimius added a commit that referenced this pull request Feb 16, 2016

Merge pull request #1075 from dotty-staging/fix-t920-test

5e80233

Drop empty companion objects

DarkDimius merged commit 5e80233 into scala:master Feb 16, 2016

DarkDimius reviewed Feb 18, 2016
View reviewed changes

allanrenucci deleted the fix-t920-test branch December 14, 2017 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Drop empty companion objects #1075

Drop empty companion objects #1075

Uh oh!

odersky commented Feb 11, 2016

Uh oh!

odersky Feb 11, 2016

Uh oh!

odersky commented Feb 11, 2016

Uh oh!

DarkDimius commented Feb 12, 2016

Uh oh!

odersky commented Feb 12, 2016

Uh oh!

odersky commented Feb 13, 2016

Uh oh!

DarkDimius commented Feb 13, 2016

Uh oh!

odersky commented Feb 13, 2016

Uh oh!

DarkDimius commented Feb 13, 2016

Uh oh!

odersky commented Feb 16, 2016

Uh oh!

DarkDimius commented Feb 16, 2016

Uh oh!

DarkDimius Feb 18, 2016

Uh oh!

odersky Feb 18, 2016

Uh oh!

Uh oh!

Drop empty companion objects #1075

Drop empty companion objects #1075

Uh oh!

Conversation

odersky commented Feb 11, 2016

Uh oh!

odersky Feb 11, 2016

Choose a reason for hiding this comment

Uh oh!

odersky commented Feb 11, 2016

Uh oh!

DarkDimius commented Feb 12, 2016

Uh oh!

odersky commented Feb 12, 2016

Uh oh!

odersky commented Feb 13, 2016

Uh oh!

DarkDimius commented Feb 13, 2016

Uh oh!

odersky commented Feb 13, 2016

Uh oh!

DarkDimius commented Feb 13, 2016

Uh oh!

odersky commented Feb 16, 2016

Uh oh!

DarkDimius commented Feb 16, 2016

Uh oh!

DarkDimius Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

odersky Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

Uh oh!