Ensure encodings aren't inadvertently tampered with#637
Conversation
Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
|
I started this PR with the intent of making sure that:
For starters, the traversal classes have no such restrictions at the moment and it's already possible to inadvertently temper or with important bits of the AST by being too aggressive in the rewriting, for instance rewriting payloads of If we decide to do it, should we do it for all traversal classes? Only map-like classes? Since we only care about someone altering an encoding in a way that breaks the roundtrip here, that could make sense. Another approach would be to decode the node, traverse its "valid" children, i.e. ones that are children of the original, non encoded node that are simply copied into the encoding and re-encode the result. Example: (~a: x, ~b: y)is encoded as: [%ppxlib.migration.pexp_labeled_tuple_5_4 (((`Some `a), x), ((`Some `b), y))]Here we could make it recurse into @patricoferris what do you think? |
|
One possible approach would be to wait and see how the ecosystem will react to this, I might have been overthinking this. I think the most likely scenario is that encodings will probably not be touched by the vast majority of all AST transforms. If one happens to rewrite the inside of the payload, it will either touch original children node, which should be fine, or it will temper with the encoding itself and the round trip migration will fail. The error will be confusing for end users but it should still point to ppxlib clearly enough for them to report the issue here. |
No description provided.