Fix application of context-free rules inside payloads #297
Conversation
a569211 to
2331f50
Compare
|
@ceastlund can you try this out, it should fix the bug you ran into! I'm also curious to see if there's anything I didn't account for and I guess running it on the janestreet codebase will give me confidence that it works even for power users! I'll try to test this in the opam world as well! |
2331f50 to
0f597e7
Compare
There was a problem hiding this comment.
Thanks for fixing this! Apart from that one question I have about context-free rules adding attributes, it looks good to me. And testing it on janestreet and opam sounds great!
I'm happy to add better tests as well if that makes you feel safer.
Better tests are always great! What kind of tests do you have in mind?
It might be worth adding a check that the attributes are equal between the expanded and unexpanded versions with the exception of nodes that are indeed expanded
What do you mean here?
|
I think in the future we should really look into allowing looking into either the expanded or unexpanded node as it doesn't cost much. I'd still rather wait until there are concrete use cases before doing it as it requires some API changes. They can be hidden using a It's clear this needs to be cleaned up a bit but since @ceastlund tested it (with #299) at Jane Street it's fairly unlikely that it will break anything in else in the wild, I think Jane Street is our best power user of ppx but we never know. I'll ask @kit-ty-kate for a bit of opam testing! |
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
0f597e7 to
bc1436c
Compare
|
I unified a bit the error handling and documented the otherwise obscur assertion on rec flags equality! I think this is ready for review now! |
|
From the look of it, it seems that ppx_import leaves the attributes of type declarations untouched so there shouldn't be any problem with this! |
|
This pull request works for jane street code. |
|
We have the results of the opam health check now and this PR seems all good. Thank you @kit-ty-kate! @NathanReb, if you want you could double-check, but this PR really doesn't seem to affect anyone on opam. The health check has revealed another problem we have thouth, which is that our improvement of error messages has broken the cram tests of ppxlib users (e.g. here, here etc), but that problem is unrelated to this PR. So I'd say, let's go ahead and merge this! Do you agree? |
CHANGES: - Add support for OCaml 4.14 (ocaml-ppx/ppxlib#304, @kit-ty-kate) - Expand nodes before applying derivers or other inline attributes based transformation, allowing better interactions between extensions and derivers (ocaml-ppx/ppxlib#279, ocaml-ppx/ppxlib#297, @NathanReb) - Add support for registering ppx_import as a pseudo context-free rule (ocaml-ppx/ppxlib#271, @NathanReb) - Add `input_name` to the `Expansion_context.Extension` and `Expansion_context.Deriver` modules (ocaml-ppx/ppxlib#284, @tatchi) - Improve `gen_symbol` to strip previous unique suffix before adding a new one (ocaml-ppx/ppxlib#285, @ceastlund) - Improve `name_type_params_in_td` to use prefixes `a`, `b`, ... instead of `v_x`. (ocaml-ppx/ppxlib#285, @ceastlund) - Fix a bug in `type_is_recursive` and `really_recursive` where they would consider a type declaration recursive if the type appeared inside an attribute payload (ocaml-ppx/ppxlib#299, @NathanReb)
This is a follow up to (meaning it also depends on) #295 and fixes the bug we introduced, reverting back to the old behaviour regarding the application of context-free rules inside paylaod.
The fix
The only part that was broken was the expansion of attributes payload.
I wanted to fix it without reverting the order of expansions while still preserving the old behaviour for whole AST transformations in case anyone relied on it.
What happens is that the part that handles the application of attribute based rules, namely
handle_attr_group_inlineandhandle_attr_inlinenow receive two copies of the node, the "original" and the expanded version.It uses the original to parse attributes but passes the expanded version to the expand function (for nice interactions with other context-free rules such as
ppx_import).It works on the assumption that context-free rules cannot rewrite attributes at that level, which is true of everything EXCEPT ppx_import precisely. It should be alright since we are involved in ppx_import development and can make sure they don't break this.
I'm a bit unsure about all this so extensive review would be welcome, I'm happy to add better tests as well if that makes you feel safer.
It might be worth adding a check that the attributes are equal between the expanded and unexpanded versions with the exception of nodes that are indeed expanded so that if we ever break this in the future somehow, we'll know what happened.
Having some kind of type safety or making sure ppxlib prevents unexpected transformation here would be best but it might be hard to achieve.
Future possibilities
One good thing about this change is that it opens up possibilities. We could imagine slightly modifying the API for derivers so that they decide whether they want to interpret the expanded and unexpanded versions, both of their agrs and the notes they're attached to.
I think we'd need to change the type
Expansion_contex.Deriver.tto take a type argument and do a bit of work on the Args but nothing too involved. We could probably do it in a way that doesn't break most users code.