use methods to find companion class #436

DarkDimius · 2015-03-26T15:15:55Z

No description provided.

DarkDimius · 2015-03-26T15:17:28Z

This branch includes change only to companionClass. I'll make a separate PR for companionModule when I implement it.

Otherwise we'll trigger early creation of companions that could shadow something.

…e name

DarkDimius · 2015-03-26T17:44:50Z

@odersky please review.

odersky · 2015-03-28T18:41:16Z

src/dotty/tools/dotc/ast/Desugar.scala

-      else if (defaultGetters.nonEmpty)
-        companionDefs(anyRef, defaultGetters)
-      else Nil
+      else {


Why the change in formatting? I liked the old one better.

This code was rewritten, it used to have more complicated logic between the commits. I have no problem changing it back.

odersky · 2015-03-28T19:18:04Z

I did not see a place where companion methods are turned into trees. Did I miss something? The concern here is that TreeTypeMap (and with it, subst, changeOwner) work on trees, not scopes. So they will not create new companion links unless the methods are part of the tree,

DarkDimius · 2015-03-28T19:26:24Z

Actually due to way how TreeTypeMap works now, I'm not synthesizing the tree. It's not obvious what does it mean for companion links to TreeTypeMap the tree, which includes a class but doesn't include the companion. In this case we can end up with several classes that have the same companion module, eg in case of

case class C[@specialized T](a: T)

we'll have companion object which is same for all specialized classes.

I wanted to discuss if this is what we want. Actually the more i think of this, the more I believe that this actually makes sense.

odersky · 2015-03-28T19:39:18Z

src/dotty/tools/dotc/core/pickling/UnPickler.scala

+      if (companionClassMethod.exists)
+        companionClassMethod.entered
+    }
+


It's a pity that we still need scalacLinkedClass here, because it means we cannot get rid of the associated complexity of isCoDefinedWith. I would much prefer if we got the linked class directly from the unpickling info, instead of looking at
the scope.

odersky · 2015-03-28T19:42:15Z

Overall, I am a bit ambivalent about this PR. It adds a lot of complications. So far the main benefit is that it makes linkedClass insensitive to naming issues. However, I would prefer simply to get naming right instead of inventing another mechanism to patch any faults it might have. And it looks that this (getting naming right) requires altogether less effort than this PR.

There would be a win in my mind if we could at the same time get rid of isCoDefinedWith which is fragile and very dependent on how the compiler is used. But so far the PR does not do that.

DarkDimius · 2015-03-28T19:52:17Z

@odersky, can you please tell which complications you mean in your comment? All this pr does:

Detects companion links using old scheme for Java and Scala2 defined classes.
Adds companion links for dotty-defined classes during appropriate moment that doesn't break anything in typer.
relies indeed currently relies on previous implementation, but if there's a simpler way to discover companion links, nothing stops us from using it.
doesn't seem to add any complications, but actually revealed several bugs\deviations from what scalac does

DarkDimius · 2015-03-28T19:59:52Z

So far the main benefit is that it makes linkedClass insensitive to naming issues. However, I would prefer simply to get naming right instead of inventing another mechanism to patch any faults it might have.

Yes, and it also gives us guarantees that any tool that somehow changes or interacts with TASTY files, or any compiler plugin, becomes easier to implement as there is one less thing that they can break: links between companions cannot be silently broken without intention anymore. I believe that this is an improvement on its own.

odersky · 2015-03-28T20:19:13Z

Regarding the TreeTypeMap issue, the problem is if you duplicate a code like this:

 def f() { class C { ... } object C { ... } ... }

I do not see how the link between the copies of the C's could be maintained.

odersky · 2015-03-28T20:30:36Z

Complications: I just look at (1) lines added, (2) files touched, and the pretty hairy code in Namer. It certainly looks like we cannot contain this to a single point, and have to add complications to several places.

Regarding robustness, I am still not sure. I am more of the school of "fail early". That is, if somebody gets naming wrong it's better to fail quickly rather than have another mechanism that works around it. Otherwise put, try to get to a state where every piece of information is encoded in exactly one way. One could argue that companion methods are what defines companionship and names do not matter. But then we need to (1) get rid of references where names do matter as in scalacLinkedClass and (2) make sure that names will never matter in any other way. If we fail to do that, it's probably better to make sure names work right, because that way we do not duplicate info and if we get it wrong we fail early.

But I noted another aspect where the new scheme beats the old: We no longer need to traverse the tree to find companions of inner classes. That's a big win! And furthermore, one could argue that scalac unpickling is the only reason we still need scalacLinkedClass, and hopefully that will go away and be replaced with TASTY. So that tilts the balance back in favor of this PR.

DarkDimius · 2015-03-28T20:33:55Z

@odersky, in your example, are you TreeTypeMap-ing def f()? or only class C?
If first one(def f()), it should use the same mechanism that works for every method in class C that could return object C. I've tested and it just works(withMappedSyms in transformDefs handles this).

If you are duplicating class C inside f without duplicating object C we need to understand what does this mean for those links at all.

I propose to take this discussion offline, but it would help me if you could tell me what do you find complicated in this PR, as I will have time to address this comments and come up with simpler implementation.

The only complicated thing that I see around is scalacLinkedClass, which is not added but only maintained. It is only used to retrieve the information from scalac, if we had TASTY, it would no longer be needed.

odersky · 2015-03-28T20:44:18Z

I meant: duplicate def f. On second thought, I now see how this works. So much the better if we do not need the trees, since there's no code generated for them anyway.

odersky · 2015-03-28T20:45:50Z

Overall I am now in favor of the PR. I would try hard to find a better implementation in Namer, however.

…scopes.

There's a non-standard interaction between explicitOuter and Pattern matcher, as patmat can request outer symbols to be available earlier. Note that this fix makes code between pattern matcher & explicitOuter non-Ycheck-able, as patmat adds reference to future symbol.

…ng symbol.

As private symbols aren't inherited, this does not break caching.

odersky · 2015-03-31T15:48:55Z

I've added the assertion, based on discussion that we had: if enter gets a scope as an argument, than
this is a scope that will eventually become decls of this symbol. And this should only happen if this is
first time the scope of symbol is coputed, ie symbol yet has no future.

OK, good! Then let's add this to the comment, to make it more understandable.

odersky · 2015-03-31T16:06:25Z

That's all I have.

This reverts commit b653007.

use methods to find companion class

DarkDimius added 3 commits March 25, 2015 18:21

scala#353 use methods to find companion class

13a05d5

Add companion class methods for files read from scala & java

d01ecb7

Fix installAfter for a Denotation List of single denotation

25af814

DarkDimius added 4 commits March 26, 2015 16:20

Add companion link symbols early only if companion actually exists

7baead9

Otherwise we'll trigger early creation of companions that could shadow something.

Add late companion symbols in firstTransform

880a6f5

scala#435 Fix conflict between package object and case class with sam…

5e09d2d

…e name

Fix error message in typer

f40c498

DarkDimius force-pushed the linked-class branch 2 times, most recently from 034cce7 to 93399e7 Compare March 26, 2015 16:34

Remove code duplication between Namer, ClassfileParser and UnPickler

0a94d6e

DarkDimius force-pushed the linked-class branch from 93399e7 to 0a94d6e Compare March 26, 2015 17:24

odersky reviewed Mar 28, 2015
View reviewed changes

DarkDimius added 4 commits March 28, 2015 20:34

Guard against absent symbols in synthesizeCompanionMethod.

7021570

Make companion-module links in UnPickler

f2221d0

Make companion-module links in ClassfileParser

595c3f6

Use methods to find companion modules

f9910eb

odersky reviewed Mar 28, 2015
View reviewed changes

DarkDimius added 9 commits March 30, 2015 13:24

Fix companion_class_method name

57027f7

Workaround scala#440 in FirstTransform.

b653007

Fix scala#440: entering symbol into scope also enters it into future …

cdbe81e

…scopes.

Fix scala#442.

042c2f0

There's a non-standard interaction between explicitOuter and Pattern matcher, as patmat can request outer symbols to be available earlier. Note that this fix makes code between pattern matcher & explicitOuter non-Ycheck-able, as patmat adds reference to future symbol.

Do not synthesizeCompanionMethod twice, and do not rewrite the existi…

84602a3

…ng symbol.

companionModule needs to return ModuleVal for Module.

e907c78

Allow to enter private symbols into Frozen scopes.

fdc92a6

As private symbols aren't inherited, this does not break caching.

Fix scala#443, set moduleClass of class being lazily unpickled.

14e0f72

Both module and class being unpickled need to register links.

34f1650

Revert "Workaround scala#440 in FirstTransform."

c560648

This reverts commit b653007.

DarkDimius added a commit to dotty-staging/dotty that referenced this pull request Apr 2, 2015

Simplify methods implemented in scala#436

78e46f4

Simplify methods implemented in scala#436

e5618d2

DarkDimius force-pushed the linked-class branch from 78e46f4 to e5618d2 Compare April 2, 2015 14:30

DarkDimius mentioned this pull request Apr 2, 2015

New attempt to fix the leaking companion problem #424

Closed

DarkDimius added a commit that referenced this pull request Apr 2, 2015

Merge pull request #436 from dotty-staging/linked-class

63d1f86

use methods to find companion class

DarkDimius merged commit 63d1f86 into scala:master Apr 2, 2015

DarkDimius mentioned this pull request Apr 3, 2015

Implement value classes #411

Merged

smarter mentioned this pull request Apr 3, 2015

Backend triggers assertion in assertClassNotArray #461

Closed

smarter mentioned this pull request Nov 18, 2015

Changing the way how "companion class <-> companion" object links work #353

Closed

allanrenucci deleted the linked-class branch December 14, 2017 19:23

use methods to find companion class #436

use methods to find companion class #436

Uh oh!

Conversation

DarkDimius commented Mar 26, 2015

Uh oh!

DarkDimius commented Mar 26, 2015

Uh oh!

DarkDimius commented Mar 26, 2015

Uh oh!

odersky Mar 28, 2015

Choose a reason for hiding this comment

Uh oh!

DarkDimius Mar 28, 2015

Choose a reason for hiding this comment

Uh oh!

odersky commented Mar 28, 2015

Uh oh!

DarkDimius commented Mar 28, 2015

Uh oh!

odersky Mar 28, 2015

Choose a reason for hiding this comment

Uh oh!

odersky commented Mar 28, 2015

Uh oh!

DarkDimius commented Mar 28, 2015

Uh oh!

DarkDimius commented Mar 28, 2015

Uh oh!

odersky commented Mar 28, 2015

Uh oh!

odersky commented Mar 28, 2015

Uh oh!

DarkDimius commented Mar 28, 2015

Uh oh!

odersky commented Mar 28, 2015

Uh oh!

odersky commented Mar 28, 2015

Uh oh!

odersky commented Mar 31, 2015

Uh oh!

odersky commented Mar 31, 2015

Uh oh!

Uh oh!