Only convert needed parts of the AST instead of converting mapper

[thierry-martinez]

This fixes the segmentation fault reported by kit-ty-kate:
#210 (comment)

I don't know where this segmentation fault precisely occurred, but I suppose that converting the mapper makes Migrate_parsetree make a lot of conversion between each sub-tree of the AST. The new approach is a bit more verbose but conceptually simpler and should be more efficient.

[kit-ty-kate]

Thanks a lot for that, I was really going nowhere trying to understand where the issue came from ^^"

The CI failure is a transient failure and can be ignored (or retried). I've launched a check with this branch checking all the opam packages for 4.10 and 4.11, but considering nocrypto now builds with this PR, I am very hopeful this fixes all the issues.

Thanks a lot for that!

[gasche]

I would prefer if we isolated precisely the issue that causes the segfault, and fixed only this in a minimal way, to make sure that we understand well what the problem is. We can make other improvements suggested by the bug hunt as a separate commits. I don't know if your PR currently has this property.

Our current understanding of the segfault is that we are incorrectly using OCaml_408.Ast.Ast_mapper.register, when we should be using OCaml_current.Ast.Ast_mapper.register. Is this change enough to fix the issue? If not, why not?

[thierry-martinez]

OCaml_current.Ast.Ast_mapper.register expects a mapper of type OCaml_current.Ast.Ast_mapper.mapper whereas Ppx_deriving_main.mapper
returns a mapper of type Migrate_parsetree.OCaml_408.Ast.Ast_mapper.mapper. We can compose another call to a Migrate_parsetree converter to obtain a mapper with the right type, but I don't know if it is really a better fix...

[gasche]

Wow, the headaches... Let me summarize what I now understand.

The code in the current master comes from your original ppxlib PR, /~https://github.com/ocaml-ppx/ppx_deriving/pull/206/files#diff-2e347c45109b096b774cef8af562932aR6, which was on top of previous code to use ocaml-migrate-parsetree for the driver machinery (but not for anything else). In the PR, you redefined Ast_mapper from the compiler-libs version to Migrate_parsetree.OCaml_408.Ast.Ast_mapper. (Now we understand that this silently broke the call to Ast_mapper.register lower down in the file). The mapper we define in this source file had type OCaml_current.mapper (simplifying), but the magical open Ppxlib had the effect of silently shadowing the AST definitions, and so the same code now had type OCaml_408.mapper.

There was one remaining issue, though: this code is doing weird driver stuff by inheriting from a base mapper obtained through Migrate_parsetree.Driver.run_as_ast_mapper (instead of the compilerlibs or PPxlib default mapper), and that has type OCaml_current.mapper. So you added the Convert module to convert this base mapper into an OCaml_408.mapper.

Now we realize that we need to call OCaml_current.Ast_mapper.register and not OCaml_408.Ast_mapper.register (indeed!), so we suddenly want a OCaml_current.mapper again. We could do a second conversion in the other direction:

  (run_as_ast_mapper : current.mapper)
    {convert to 408.mapper}
  (our code : 408.mapper) 
    {convert to current.mapper}
  current.Ast_mapper.register

but that sounds very unreasonable, I agree, and it would also break if non-408 features are used in some traversed parts of the AST that we don't care about. Instead what your PR does is to be more careful in the way our code is called, so that it is presented as a current.mapper while matching only on the 4.08 AST (which is the point of ppxlib's approach to compatibility: we stick to a fixed AST version).

[gasche]

If my understanding above is correct, then I agree that your PR goes in the right direction. However, then I think we need to do more changes to have a reasonable final state.

I don't think it is reasonable to have a Ast_mapper module such that it is dangerous (only a bug at runtime, even on good days where it does not segfault) to call Ast_mapper.register by mistake. I would prefer if we defined Ast_mapper as OCaml_current.Ast.Ast_mapper throughout the whole module. In particular I would prefer if the Ast_mapper.{get,set}_cookie functions were called without a translation layer.

[gasche]

Ah, but this suggestion has negative implications for backward-compatibility: if the Ast_mapper API were to change in future OCaml versions, we would have to fix our code, which we want to avoid.

Your PR has the advantage that both the Parsetree fragments and the Ast_mapper API are used at a fixed version, except for the very final call to .register. My suggestion would have a fixed-version Parsetree handling, with a current-version Ast_mapper logic. It may be more robust in some ways (we avoid compatibility logic which, as we have seen, is not always completely reliable), but it is also more work for us. I'm not sure anymore this is the right choice.

[gasche]

@thierry-martinez I pushed an experiment on top of your PR as gasche@67b118a . I have the impression that the code is cleaner; what do you think?

[thierry-martinez]

@gasche I think that your experiment is not only cleaner but also "more correct" than mine, indeed: OCaml_current's cookies and Ast_408's cookies are stored in distinct references, and it is OCaml_current's one which is updated when the AST is read. I just didn't like much converting twice the cookies' payload: I think that it is only due to the fact that we are paying the price of using Ppxlib.Ast_helper instead of the Ast_helper of the current compiler-lib. I suggest on top of your suggestion another version: the only moot point was to take care of not using current OCaml's Pconst_string to stay compatible with 4.11.

[kit-ty-kate]

After checking all the opam packages, I can confirm 167a9fc fixes all the issues I was seeing.

Should I check again with the latest commit?

[gasche]

@kit-ty-kate thanks for the opam checking! I think that we might want to iterate just a bit more to get a nice PR. I would propose that you wait until we decide to merge a final fix, and then it would be very nice if you could another round of testing.

[gasche]

@thierry-martinez the cookie problem is a good catch, thanks!

I realized that I had forgotten a Pexp_tuple expression which was still being type-checked in the "current" AST, while I intended all parsetree-related constructs to be fixed at 4.08. I pushed a supplementary fix in db650a5 .

There are subtle differences between my WIP and your proposal. In my proposal, I intended to have all Parsetree logic at OCaml_408, and all the logic is Ppxlib's or Migrate_parsetree's, never compilerlibs. You version feels a more complex because you construct and deconstruct ASTs in different versions (note: I think the copies are neglectible performance-wise, and do not incur any extra risk of dynamic failure), and you mix Ppxlib/Migrate_parstree logic with Compilerlibs logic (which is fine as OCaml_current should be compatible with Compilerlibs).
I'm happy to have either. (But I think it would be nice to fix the Pexp_tuple matching.)

I also realized something unrelated while reading the code. In practice the mapper from this module runs copy_structure_item on all signature items, so it is not converting only needed parts of the AST, it is converting everything. I wonder whether it is possible to protect ourselves from dynamic failures (due to not-convertible parts of the AST that we do not care about) by catching an exception _ case in match copy_structure_item hd, doing the same as in the wildcard case. (I believe, but I have not tested, that those cases throw a Migrate_parsetree_def.Migration_error exception.)

[kit-ty-kate]

This raises a syntax error in the CI

[thierry-martinez]

@kit-ty-kate oops, fixed! Thanks!

[kit-ty-kate]

No worries, but I was more pointing out the -> without any values on the right

[thierry-martinez]

Sorry, I should have been very more cautious: I didn't notice that make test didn't recompile Ppx_deriving_main...
I did some changes to accomodate OCaml <4.08 compilers (no Option module, and no get_cookie in OCaml_current.Ast_mapper). Sadly, it makes me mix again the logic between Migrate_parsetree and compiler-libs, since Ast_mapper.get_cookie should be taken from Ocaml_common.Ast_mapper instead of Migrate_parsetree.Ocaml_current.Ast_mapper, since it is not defined by Migrate_parsetree for OCaml <4.08. @gasche, what do you think? Can we accomodate with such a mix?

[gasche]

Yes! The new code looks very nice to me. Is there anything left to do before merging?

[thierry-martinez]

I think everything is OK now. Merging! Thank you very much, @kit-ty-kate and @gasche.