Skip to content

Ensure encodings aren't inadvertently tampered with#637

Draft
NathanReb wants to merge 1 commit into
ocaml-ppx:mainfrom
NathanReb:do-not-traverse-encodings
Draft

Ensure encodings aren't inadvertently tampered with#637
NathanReb wants to merge 1 commit into
ocaml-ppx:mainfrom
NathanReb:do-not-traverse-encodings

Conversation

@NathanReb
Copy link
Copy Markdown
Collaborator

No description provided.

Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
@NathanReb
Copy link
Copy Markdown
Collaborator Author

NathanReb commented Apr 7, 2026

I started this PR with the intent of making sure that:

  1. Encodings do not trigger extension checks
  2. Traversal classes based rewriters would not temper with encodings inadvertently

1 is already the case but now we have a test for this!

2 I think should be debated before proceeding further. Initially I wanted to overwrite the extension method of all Ast_traverse classes so that they do not traverse the payload of ppxlib.migration extensions by default but there are a few issues with 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 ocaml, merlin or other such attributes or extensions without us trying to prevent it.

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 x and y but preserve the wrapping tuples and the weird polymorphic variant encodings of the labels.
Note that this approach is only feasable for nodes for which we provide the Ast_builder/Ast_pattern interface and that have the 5.2 decoding function.

@patricoferris what do you think?

@NathanReb
Copy link
Copy Markdown
Collaborator Author

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.

@NathanReb NathanReb changed the title Ensure encodings aren't inadvertently tempered with Ensure encodings aren't inadvertently tampered with May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant