Skip to content

Ensure encodings aren't inadvertently tempered with#637

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

Ensure encodings aren't inadvertently tempered with#637
NathanReb wants to merge 1 commit intoocaml-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?

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