ocaml-ppx / ppx_tools

Tools for authors of ppx rewriters
MIT License
134 stars 39 forks source link

Upgrade to OCaml 4.10 #79

Closed kit-ty-kate closed 4 years ago

kit-ty-kate commented 4 years ago

Fixes https://github.com/ocaml-ppx/ppx_tools/issues/78 (cc @gasche)

For the change in ast_mapper_class.ml, I followed the change from this commit: https://github.com/ocaml/ocaml/commit/8e928caea7c47e6ba8508cf2caaaa1ba9f8dca85

And for the change in genlifter.ml I looked up what changed in: https://github.com/ocaml/ocaml/commit/c19e8b23506e38fc47c110138d50b57fe93f7d7e. The only thing I'm not too sure here is the use of mknoloc.

This is complete guess work, I have no clues about the internals of the compilers nor the ones of ppx_tools. Luckely enough for me it wasn't too long nor too hard.

gasche commented 4 years ago

Have you, by any chance, tried ppx_tools with 4.09? Is the current version of ppx_tools working with 4.09 (and it's just that nobody tried), or is the after-the-PR version compatible with 4.09? Are some of the changes here necessary for 4.09 and some incompatible with 4.09, in which case it could make sense to split the PR in two to have a "working with 4.09" intermediate state?

kit-ty-kate commented 4 years ago

The latest release compiles with 4.09 (see http://check.ocamllabs.io/log/1576461049-0e61d9c70233f946d970e31ec715fadef7dc0da5/4.09/good/ppx_tools.5.3+4.08.0) and I haven't heard anybody complain about it not working so far.

gasche commented 4 years ago

Ah, I misread the .opam dependency as <= 4.08 requirement, apologies for the confusion.

kit-ty-kate commented 4 years ago

CI is finally green

kit-ty-kate commented 4 years ago

The PR is now good to be merged. Can someone confirm everything is alright? I'm ready to make a release when deemed necessary.

kit-ty-kate commented 4 years ago

For the noloc question I looked up the documentation and in the end it was not the right fix:

The [lookup_foo] functions will emit proper error messages (by
raising [Error]) if the identifier cannot be found, whereas the
[find_foo_by_name] functions will raise [Not_found] instead.

from https://github.com/ocaml/ocaml/blob/b52e11dd414d0bb9583d70025a742924291db0c2/typing/env.mli#L172

So using Env.find_type_by_name instead of Env.lookup_type is the right fix here, my bad.