Skip to content

Conversation

NathanReb
Copy link
Collaborator

@NathanReb NathanReb commented Jul 10, 2025

This tool is inspired from the omp version of old.

It is invoked as follows:

dune exec -- dev/gencopy/gencopy.exe astlib/ast_X.ml astlib/ast_Y.ml > migrate_X_Y.ml

It will generates deep "copy" functions from ast version X to version Y. These are dummy function that are generated under the assumption that the ast types in X and Y are identical.

When adding support for new compiler versions, this should be used as a base to be editted, following the compiler errors that will point to AST nodes that actually differ between the two versions.

Differences with the OMP tool

The OMP tool used the compiler libraries to load compiled interfaces and a proper typing environment and used that to recursively generate copy functions for any types it encountered, starting from a given set of entry point types. This ensured it wouldn't generate unnecessary functions.

It turns out that this method as one major draw back: the tool heavily relied on unstable libraries and therefore could only build on a fixed version of OCaml. It was also a bit more complex than this new approach which does not bother with typing env lookups.

This version is much simpler, it uses the ppxlib driver to parse the ast_x.ml source file to our internal AST and generates the copy functions from the type declarations, all in one big set of recursive value bindings.

In practice this makes almost no difference as we end up needing copy functions for every type anyway. Worst case scenario: unused functions will be picked up by the compiler and can be deleted manually.
One advantage the old approach had is that it was able to generate copy functions for external types such as location, which we don't do anymore. Once again, in practice it doesn't matter much as location has not changed and therefore can simply be copied using the identity function since it's not versioned.

One thing to note is that it completely changes the order in which the copy functions appear in a migrate_X_Y.ml file compare to what we used to have.

This tool is inspired from the omp version of old.

It is invoked as follows:
```
dune exec -- dev/gencopy/gencopy.exe astlib/ast_X.ml astlib/ast_Y.ml > migrate_X_Y.ml
```
It will generates deep "copy" functions from ast version X to version Y.
These are dummy function that are generated under the assumption that
the ast types in X and Y are identical.

When adding support for new compiler versions, this should be used as a
base to be editted, following the compiler errors that will point to AST
nodes that actually differ between the two versions.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb NathanReb added the no changelog Use this label to disable the changelog check github action label Jul 10, 2025
@NathanReb
Copy link
Collaborator Author

I've tested it to generate the 5.4 -> 5.3 migration and it worked pretty well. Here are a few things I'd like to do before we merge this:

  • add basic tests so that it's built by our CI to make sure it is maintained
  • manually test the 5.3 -> 5.4 migration just in case
  • Add handling of location: at the moment it uses copy_location, which it does not generates, I had to define let copy_location = fun x -> x to make the migration build. We can either add an hardcoded copy_location at the start or simply use inline (fun x -> x) when we need to copy locations. I think the latter would work just fine.

I've been working on and off for a long period of time so review would be greatly appreciated while I wrap up these few extras!

Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @NathanReb, I had a quick pass and it seems good to me. I agree that some tests would be very useful, as well as testing it against some previous migrations.

Re:copy_location -- I think that makes sense to me, some hardcoded parts are probably nothing to worry about given how slow moving the Parsetree is (except in the ways we don't want it to change!)

@NathanReb NathanReb requested a review from patricoferris July 24, 2025 11:21
Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @NathanReb -- I think this will already be useful as is :) Let's merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Use this label to disable the changelog check github action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants