Traverser and Pickler improvements. #3033
Conversation
This enables a measure of "command/query separation", which is to say: the same method shouldn't go on a side effecting binge and also return a value.
There's a huge amount of tree traversal related duplication which is hard to eliminate only because trees aren't properly traversed. The not-quite-tree bits of key trees are ignored during traversals, making it impossible to write e.g. a pretty printer without duplicating almost the entire traversal logic (as indeed is done for printing purposes in more than one place.) And almost the entire pickler logic is redundant with Traverser, except since it's all duplicated of course it diverged. The pickler issue is remedied in the commit to follow. The not-quite-trees not quite being traversed were Modifiers, Name, ImportSelector, and Constant. Now every case field of every tree is traversed, with classes which aren't trees traversed via the following methods, default implementations as shown: def traverseName(name: Name): Unit = () def traverseConstant(c: Constant): Unit = () def traverseImportSelector(sel: ImportSelector): Unit = () def traverseModifiers(mods: Modifiers): Unit = traverseAnnotations(mods.annotations)
This commit drops about 700 lines of redundant traversal logic. There had been ad hoc adjustments to the pickling scheme here and there, probably in pursuit of tiny performance improvements. For instance, a Block was pickled expr/stats instead of stats/expr, a TypeDef was pickled rhs/tparams instead of tparams/rhs. The benefits derived are invisible compared to the cost of having several hundred lines of tree traversal code duplicated in half a dozen or more places. After making Traverser consistent/complete, it was a straightforward matter to use it for pickling. It is ALSO now possible to write a vastly cleaner tree printer than the ones presently in trunk, but I leave this as an exercise for Dear Reviewer.
|
@retronym - @gkossakowski made a comment on -internals which sounded suspiciously like LGTM so I'm going to hear it that way unless you pipe up. You want source-to-source translation, this PR is your future friend. |
|
I like the direction of this PR but I didn't have time to review it thoroughly and I'd like to. I'll have time to do it tomorrow. Can we defer the merge until tomorrow? |
|
Sure. |
|
@gkossakowski If you're 8+ time zones ahead of me like I think you are, then tomorrow is today and today's almost over. |
|
@paulp: I had my head deep in M6 release process and circular dependencies. I didn't realize the whole day passed. I'm looking into this now. |
|
LGTM. Thanks! I share @retronym's worry about pickling format being changed but it's only for trees so I hope we won't break the world. We'll see soon. |
Traverser and Pickler improvements.
|
Thanks a lot. For the next guy, there's a similar effort to be undertaken in having type traversal correspond to type pickling. That one will successfully break the world, and also requires a type traverser be written which hasn't been infiltrated by all manner of partially overlapping concerns in the way TypeMap has. |
This is an improvement on #3019 which implements pickling and unpickling of Trees with Traversers rather than only describing the possibility.