Lift ppx expansion errors as extension nodes to top level of file #358
Conversation
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
There was a problem hiding this comment.
Thanks, I like the idea of lifting the errors and treating them at signature/structure level a lot, so either #357 or this PR are definitely a good improvement! As a side-note: doing so introduces one small inconsistency: ppxlib errors will be lifted, while embedded errors by context free rules will stay at the nodes they arise at. I don't think that's a problem though.
The main differences I see with #357 are:
- the
map_top_downclass doesn't get duplicated. I think that's a good thing. Folding errors when AST traversing, to embed them at the toplovel structure/signature. #357 could very easily be adapted to do the same though, right? - the
Ppxlib.Ast_traverse.map_with_expansion_contextgets erased. Could also be adapted though. See my comment about that below concerningbisect_ppx. - error handling is factored through the new
With_errorsmonad. That seems very nice to me and makes things very well readable!
I haven't had a close look though. Are there more differences?
In any case, if you aren't in a strong hurry with #352, I think it would be cool to wait for @panglesd to come back from vacation to see what he thinks, both in general about this PR and compared to his #357.
I haven't fully reviewed yet, but I've already dropped some comments below.
There was a problem hiding this comment.
I think that using a lift_map rather than a fold_map is better (we do not need to pass the accumulator when calling methods).
It also leads with the infix syntax to a cleaner code than the fold_map, although I generally prefer the let syntax (unfortunately custom let syntax is unavailable in old ocaml versions...)
I have small comments on the code or the names, but that is good for merge on my side!
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
…on_context_and_errors. Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
CHANGES: - Make `esequence` right-associative. (ocaml-ppx/ppxlib#366, @ceastlund) - Deprecate unused attributes in `Deriving.Generator` (ocaml-ppx/ppxlib#368, @sim642) - Remove a pattern match on mutable state in a function argument. (ocaml-ppx/ppxlib#362, @ceastlund) - Add code-path manipulation attributes. (ocaml-ppx/ppxlib#352, @ceastlund) - Update context-free rules to collect expansion errors generated by ppxlib and propagate them to top level without failing. (ocaml-ppx/ppxlib#358 and ocaml-ppx/ppxlib#361, @ceastlund)
CHANGES: - Make `esequence` right-associative. (ocaml-ppx/ppxlib#366, @ceastlund) - Deprecate unused attributes in `Deriving.Generator` (ocaml-ppx/ppxlib#368, @sim642) - Remove a pattern match on mutable state in a function argument. (ocaml-ppx/ppxlib#362, @ceastlund) - Add code-path manipulation attributes. (ocaml-ppx/ppxlib#352, @ceastlund) - Update context-free rules to collect expansion errors generated by ppxlib and propagate them to top level without failing. (ocaml-ppx/ppxlib#358 and ocaml-ppx/ppxlib#361, @ceastlund)
Changes
map_top_downto collect errors explicitly. Like #357 but with fewer added/duplicated classes, and with errors mostly handled by aWith_errorsmonad.