support merging companion objects in expanded trees #2057
Conversation
liufengyun
changed the title
liufengyun
force-pushed
the
merge-companion
branch
from
e097d51 to
7c63258
Compare
Previous implementation is problematic when there are multiple transforms before typer: 1. There might be objects needing merging that only exist in the expanded trees, which cannot be handled by the previous algorithm. 2. There may be companion object and class defined only in the expanded trees, the previous algorithm doesn't create links for them. This PR simplifies the companion object merging logic and fixes the problems above. In fact, this PR supports multiple pre-typer transform during expansion. The protocol is that the expansion of a MemberDef is either a flattened thicket or a non-thicket tree.
liufengyun
force-pushed
the
merge-companion
branch
from
7c63258 to
d2c0f12
Compare
liufengyun
changed the title
| mdef.putAttachment(ExpandedTree, Thicket(vdef :: mcls1 :: rest)) | ||
| moduleDef(typName) = mcls1 | ||
| cdef.putAttachment(ExpandedTree, Thicket(cls :: crest)) | ||
| def valid(mdef: MemberDef): Boolean = !mdef.mods.is(Package) && mdef.mods.is(Module) |
There was a problem hiding this comment.
Shorter and mode efficient:
mdef.mods.is(Module, butNot = Package)
| case Thicket(trees) => // companion object always expands to thickets | ||
| trees.map { | ||
| case mcls @ TypeDef(name, impl: Template) if valid(mcls) => | ||
| if (moduleClsDef.contains(name)) { |
There was a problem hiding this comment.
I'd use a get instead of contains + apply:
moduleClsDef.get(name) match {
case Some((stat1, mcls1 @ TypeDef(_, impl1: Template)) => ...
case None => ...
| if (moduleClsDef.contains(name)) { | ||
| val (stat1, mcls1 @ TypeDef(_, impl1: Template)) = moduleClsDef(name) | ||
| if (mcls.mods.is(Synthetic) && !mcls1.mods.is(Synthetic)) { // merge synthetic into user-defined module | ||
| removeInExpanded(stat, mcls) |
There was a problem hiding this comment.
Factor the two preceding and the next two lines into a separate local method mergeIfSynthetic(synthCls, userCls. Then the whole 12 line if-else can be expressed as
mergeIfSynthetic(mcls, mcls1)
mergeIfSynthetic(mcls1, mcls)
| moduleClsDef(name) = (stat, mcls2) | ||
| } | ||
| else { | ||
| // redefinition of objects or case classes, handled elsewhere |
There was a problem hiding this comment.
You mean, we have a double definition? (Just asking for clarification)
There was a problem hiding this comment.
Yes, I mean double definition. I'll refine the comment for clarity.
| case vdef @ ValDef(name, _, _) if valid(vdef) => | ||
| if (moduleValDef.contains(name)) { | ||
| val (stat1, vdef1) = moduleValDef(name) | ||
| if (vdef.mods.is(Synthetic) && !vdef1.mods.is(Synthetic)) // merge synthetic into user-defined module |
There was a problem hiding this comment.
Similar remarks as for module defs apply here. Probably better to factor out in a method mergeIfSynthetic.
There was a problem hiding this comment.
The comments are addressed in the latest commit.
For the last one, I think it's probably good to keep it as it is, as the processing is not symmetric as the case above, and the code is simple and short. Refactoring into a method seems to make readability worse.
Support merging companion objects in expanded trees
Previous implementation is problematic when there are multiple transforms before typer:
There might be objects needing merging that only exist in the expanded trees, which cannot be handled by the previous algorithm.
There may be companion object and class defined only in the expanded trees, the previous algorithm doesn't create links for them.
This PR simplifies the companion object merging logic and fixes the problems above.
In fact, this PR supports multiple pre-typer transform during expansion. The protocol is that the expansion of a statement is either a flattened thicket or a non-thicket tree.