Refactor driver transformer #489
Conversation
Fixes ocaml-ppx#461 This PR adds a new command line flag that tells the driver not to write to the output file if there is no rewriting to be done. It's not 100% accurate if there are non context free transformations registered as we do not compare the AST for this feature but simply keep track of generated code via a hook. If any non context free transformation is registered, we simply assume it will rewrite something and always output. Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
patricoferris
requested review from
ceastlund,
panglesd and
pitag-ha
as code owners
patricoferris
force-pushed
the
refactor-driver-transformer
branch
2 times, most recently
from
7d454d2
to
70ffddf
Compare
This refactors the transform data-structure in favour of easing on-boarding new developers of ppxlib. Signed-off-by: Patrick Ferris <patrick@sirref.org>
patricoferris
force-pushed
the
refactor-driver-transformer
branch
from
70ffddf
to
a3a784b
Compare
There was a problem hiding this comment.
Thanks for working on this! We'll really benefit from these changes.
I probably did not specify this work very well and I apologize for the inconvenience. What I had in mind was not to replace the record type all the way here.
Instead, I would like to keep the original record type but only use it to represent the transformations as registered by ppx authors and to use better suited types to represent the ordered list of transformations to apply.
In other words, I'd like the types you introduced or similar types to be used to turn a Transform.t list into a list of str -> str/sig -> sig.
This task is currently split between apply_transforms, get_whole_ast_passes, partition_transformations and merge_into_generic_mappers which all take Transform.t and roughly return modified Transform.t where all but relevant fields are set to None. I'd like for them to instead take a Transform.t and return a structured type that represent exactly what we want here.
Does it make more sense? I'm happy to discuss this over a voice channel if that helps or to try and write signatures for the above mentioned functions if you'd like.
| type 'result sig_fun = | ||
| Expansion_context.Base.t -> Parsetree.signature -> 'result | ||
|
|
||
| type impl_intf_pass = |
There was a problem hiding this comment.
What would you think about something along the lines of full_ast_rewrite_pass instead?
I think it would clarify a bit what those are about.
There was a problem hiding this comment.
I'm also thinking it might be good to add a tiny bit of documentation next to each individual type decl here. Just a one liner for the ones that need clarification. What do you think?
This builds on top of #482 after a conversation about the confusion around the
Transform.tdata structure and how it relates to ppxlib applying various preprocessors to OCaml code.This PR changes the
Transform.tfrom a record to a polymorphic variant with the intention to make it clearer how the driver works in terms of transforming code. Most notably, what was once accessing fields is now a lookup in a list where the list is always in the order of 10 elements (max).As a newcomer to ppxlib, I find this approach a little easier to understand as there are more types and names to help guide a new developer, and the various kinds of passes are grouped under distinct types (rather than a big record where everything is optional).