SI-6734 Synthesize companion near case class #5486
Conversation
Tweak the "should I synthesize now" test for case modules, so that the tree is inserted in the same tree as the case class.
som-snytt
force-pushed
the
issue/6734-synths
branch
from
a1181b8
to
9e1de6e
Compare
| sym.moduleClass.attachments.get[ClassForCaseCompanionAttachment] match { | ||
| case Some(att) => | ||
| val cdef = att.caseClass | ||
| stats.exists { |
There was a problem hiding this comment.
Doesn't this introduce quadratic complexity in the size of the package? This method is called for every member in scope, and the test searches linearly to find a ClassDef tree.
I'm also not sure if assuming source code is going to work always. What if the case class in scope comes from binaries?
There was a problem hiding this comment.
Only invoked for synthetic modules of case classes with attachment introduced by Namers.
There was a problem hiding this comment.
This code still looks strange to me. The attachment is a ClassDef, but the code does a linear search for a(nother?) ClassDef with the same symbol. Isn't it the same ClassDef? If not, why do they have the same symbol? In either case it might be good to document it.
This may be perfectly ok, but it's still not clear to me why it only treats symbols that are currently compiled (that have trees around).
There was a problem hiding this comment.
Currently, the moduleclass has attachment pointing to a classdef; but meanwhile the in-flight classdef is something transformed, so eq fails. The test is to say, add synthetic companion only if the associated classdef is in these statements, too. (The big loop also tries to match associated synths in an ad-hoc way.)
I'll try a bigger clean-up, where the classdef bears an attachment (to mean it has an associated synthetic; similarly for other defs). Then the big loop should just traverse looking for things with synths to add; and then at the end, add whatever remaining synths belong here.
Maybe the attachment has a Tree => Boolean to match something invariant that says, yes, this tree is associated with me.
There was a problem hiding this comment.
That would be useful for a unit with a huge number of case classes, since right now each tree is checked against all (remaining) synths. The check itself must always do work, as noted.
There was a problem hiding this comment.
Thanks for explaining, I start to understand a bit more what's happening. If I understand correctly, this PR tightens the conditions under which such synthetics are generated, presumably making it "deterministic" -- the same place where the ClassDef is. But where were they added before this PR? Maybe there's an equivalent condition that doesn't require iterating?
I agree the code won't run for all stats in a package, but the typer is generally "hot", and even if this doesn't seem much, it's still going to be doing more work than before, work that's not a constant but a function of code size.
There was a problem hiding this comment.
See the PR comment and test case -- they'd be added to the package tree containing the package object if that was first in source-text-order.
There was a problem hiding this comment.
Yeah, I stared at that test case long and hard. It's impenetrable. Thanks again for explaining. I'd really add some explanation to the test
There was a problem hiding this comment.
I was just using the test case the guy submitted on the ticket. I agree that many test cases make you wonder what's being tested and require archaeology. I'll add a comment benefiting from hindsight.
|
|
||
| //single file badimp.scala | ||
| // adding package object gives not found: type SortedMap | ||
| package object badimp |
There was a problem hiding this comment.
How about:
// this package object used to contain the synthetic companion object of `Nodal` and fail because the
// necessary import is not in this scopeThere was a problem hiding this comment.
The funny thing is that it's not the package object, but the package introduced by desugaring that received the synthetic.
|
|
||
| package badimp { | ||
|
|
||
| // move before package object works |
There was a problem hiding this comment.
This comment is more confusing than clarifying. This test case is the way it is now, and the ticket you link to gives the background and additional details.
|
|
||
| // adding target object restores sanity | ||
| // but adding it before the import does not | ||
| //object Nodal |
There was a problem hiding this comment.
I would remove this part as well since the ticket describes all these variations.
|
LGTM |
|
Sorry for missing this one, thanks for closing it @retronym |
Tweak the "should I synthesize now" test for
case modules, so that the tree is inserted in
the same tree as the case class.
This for the case where there are multiple
package p { }trees, which happens implicitly when there is apackage object p { }.No attempt made in this quick fix to improve locality-based synthesis.