search

ocaml-ppx / ppxlib

Base library and tools for ppx rewriters
MIT License
246 stars 98 forks source link

Migrate ocaml ppx context #478

Closed NathanReb closed 6 months ago

NathanReb commented 7 months ago

This PR adds migration of ocaml.ppx.context's load_path between 5.1 and 5.2.

No information is lost in the round-trip.

It comes with a test that should run only with the latest OCaml version, for now that's 5.2. The test consists of adding a manually written ocaml.ppx.context to an ml file after an actual structure item so it is not discarded by the driver. The custom driver will look for this floating attribute, manually migrate it to 5.01 (any version before 5.02 would do since the format was stable before that point), and pretty print to stdout. It will then manually migrate it to the compiler version (i.e. at least 5.02) and pretty print again. The driver actual output is discarded.

The only downside to this approach is that we have to re-implement a small part of pprintast for 5.01 in the custom driver but I'd say it's acceptable since we don't have to do it for any other version and it makes the cram test quite simple.

Let me know what you think!

NathanReb commented 7 months ago

@Octachron it seems from our conversation during the last ppxlib dev meeting that the migration from 5.02 to 5.01 should include all load paths, both hidden and visible into the 5.01 load_path field, is that correct?

The migration currently discards the hidden ones. I'll change that if you confirm!

Octachron commented 7 months ago

I think it would be better in term of backward compatibility: the idea of the hidden includes is to split the current list of includes between the direct dependencies (which should be fully visible) and the transitive dependencies (which should only be visible internally by the compiler). Thus, if we have a context that has been split into hidden and visible parts and we need to convert it to a pre-5.2 world, the closest approximation would be to make everything visible.

NathanReb commented 7 months ago

I changed the migration to merge visible and hidden when migrating down to 5.1!

NathanReb commented 7 months ago

We should probably wait on the outcome of the following discussion to decide whether we should merge this into trunk-support: https://github.com/ocaml/ocaml/pull/12246#issuecomment-1956748745

NathanReb commented 6 months ago

We're going to go with this migration and keep the 5.2 changes on the OCaml side so we do need to get this merged!

pitag-ha commented 6 months ago

Btw, thanks, @anmonteiro! Every help we can get is always appreciated!

A question out of curiosity: Have you been affected by the problem this PR solves? I'm asking because I know that in the past, some OCaml to JS compiler (concretely bucklescript) would have been affected. I'm not expecting melange to be affected, but am curious now.