Fix #97: version-specific magic numbers check in `Ast_4NN.register`

[thierry-martinez]

This commit fixes https://github.com/ocaml-ppx/ocaml-migrate-parsetree/issues/97, which caused segfaults: Ast_4NN.register now correctly checks AST magic number against the version-specific magic number instead of using the magic number defined in the Config module of the current compiler.

Version-specific magic numbers was already defined by overriding Config module later in the file: the fix just consists in riding up the overriding of Config before the overriding of Ast_helper. Following @gasche's trick recommended in https://github.com/ocaml-ppx/ppx_deriving/pull/210#issuecomment-632581603, a unit-value in_ast_4nn is defined in Config module and used in Ast_helper.register to ensure that the module are well ordered.

[gasche]

I believe that the fix is correct, in that it should correctly implement a semantics for Ast_4NN.register where the rewriter expects a 4NN-serialized AST file and correctly rewrites it.

However, I wonder what you think of my question in https://github.com/ocaml-ppx/ocaml-migrate-parsetree/issues/97#issuecomment-632660967 : is this the intended semantics, or were the people who introduced these .register functions rather intending to have them working correctly with current-version serialized ASTs?

My impression is that the other semantics is more useful, but also basically not implementable at this level: the conversion machinery is not usable at this point without introducing a dependency cycle. So I suspect that we can either fix the code as you propose, or we should remove the functions altogether.

In particular, it sounds reasonable to merge your PR now to have 4NN.register do something correctly (and, in particular, to never segfault), and then later remove the functions if the m-o-p maintainers decide it is not the semantics they want.

[ghost]

It seems to me that we started adding the registering functions by accident since 4.08. So really we should just delete them from Ast_408 on-wards. Users should use the Driver module to register rewriters.

Can you use one of the entry points of Migrate_parsetree.Driver or Ppxlib.Driver for your use case?

[ghost]

Looking in the code in the master of ppx_deriving, it seems to me that you are now relying on the register function from the compiler. I'm going to close the two PRs and remove all the register functions from the Ast_XXX modules.

[gasche]

Yes, we don't need the Ast_4NN register function, we used one of them by mistake (the Migrate_parsetree API encourages you to open the module that shadows the old-style modules, so we were working with its own Ast_mapper module).

In ppx_deriving there are two places that do driver stuff (... you wrote that code I think), one in src/api/ppx_deriving.cppo.ml that uses Migrate_parsetree.Driver.register , and one in src/ppx_deriving_main.ml that uses the old-school Ast_mapper directly to build a standalone ppx executable.