Internal ppxlib migration attributes are sometimes left in the AST

[patricoferris]

Ppxlib sometimes uses AST attributes to encode information needed to perform a correct round-trip migration. For example, to correctly roundtrip function arity, we make use of ppxlib.migration.stop_taking to indicate that we should "stop taking" function arguments:

ppxlib/astlib/migrate_502_501.ml

Line 156 in a5be80d

let attr = mk_ghost_attr "ppxlib.migration.stop_taking" in

In the corresponding, opposite migration (in this example, upwards from 501 to 502) there is code to make use of this attribute, and importantly remove it.

However, this opposite migration does not always happen. For example, ppxlib 0.35.0 has an internal AST from OCaml 4.14.0. If ppxlib is used on a compiler 5.2.0 and above, only the downward migration will happen which introduces the ppxlib.migration.stop_taking attribute which is then never removed as we do not migrate the AST back up through 501->502.

We need to remove any extra information ppxlib encodes in the AST for migrations. There are at least three options:

1. Use a separate data-structure to encode extra information to help with migrations but that leaves the AST itself intact.
2. Remove the extra information until it is needed. For example, for a vast majority of ppxlib use-cases, 0.35.0 does not need the extra migration information (unless someone is manually migrating the AST back up).
3. Have some function that removes all of this extra information and enforce some policy that users will have to call this function in order to have a clean AST (this could perhaps be enforce via the type system?).

Test Case

Using a 5.2.0 (or higher) compiler with ppxlib.0.35.0 installed, create a standalone executable:

let () = Ppxlib.Driver.standalone ()

Call it on the following test.ml file:

let f a = fun b -> a + b

Which produces the following:

let f a = ((fun b -> a + b)[@ppxlib.migration.stop_taking ])

Thanks to @n-osborne for setting me on this issue.

[davesnx]

Yes, this make some of the cram test not compatible with multiple versions of ocaml, for example, in CI.

[patricoferris]

Thanks @davesnx -- do you have a concrete example somewhere, cram tests for a particular project?

[davesnx]

https://github.com/ml-in-barcelona/server-reason-react contains a few tests that renders ppxlib.migration.stop_taking and I need to run it in 5.2.0 only with (enabled_if (>= %{ocaml_version} 5.2.0)), example:
https://github.com/ml-in-barcelona/server-reason-react/blob/main/packages/browser-ppx/tests/playground.t/run.t

[patricoferris]

We've just merged a fix for this. We're in the middle of a 0.37.0 release for OCaml 5.4, after that, when the dust settles, I might look at backporting this to 0.35 as this where most people are being bitten by this. Stuck behind the 0.36 upgrade but using new OCaml compilers making it more likely for the ppxlib migration attributes to be lurking in the AST :/