ocaml-ppx / ocaml-migrate-parsetree

DEPRECATED. See https://ocaml.org/changelog/2023-10-23-omp-deprecation. Convert OCaml parsetrees between different major versions
Other
87 stars 43 forks source link

Support for OCaml 4.08 #70

Closed xclerc closed 5 years ago

xclerc commented 5 years ago

This pull request adds support for OCaml 4.08 (beta3), and has been successfully compiled with 4.0{2,3,4,5,6,7,8}.

Due to changes in the Location module in 4.08, it also adds some utility functions to deal with Location.error values (see the Locations module). By treating the Locations.location_error type as an abstract type, it should be possible to write code that works transparently across OCaml versions.

The associated package is available on opam as ocaml-migrate-parsetree.1.3.0~4.08.0+beta3 (see https://github.com/ocaml/opam-repository/blob/master/packages/ocaml-migrate-parsetree/ocaml-migrate-parsetree.1.3.0~4.08.0%2Bbeta3/opam).

(Joint work with @hhugo)

xclerc commented 5 years ago

@avsm @kit-ty-kate Can you comment on the opam/CI testing related to the package?

kit-ty-kate commented 5 years ago

Can you comment on the opam/CI testing related to the package?

So, the run with this branch + ppx_tools_versioned patched for 4.08 just finished this morning and it seems to work pretty well so far. lwt_ppx, js_of_ocaml-ppx and tyxml-ppx all compile for example.

I noticed two problems in old versions for some packages:

#=== ERROR while compiling js_of_ocaml-ppx.3.3.0 ==============================#
# context              2.0.3 | linux/x86_64 | ocaml-variants.4.08.0+beta3 | file:///home/opam/opam-repository
# path                 ~/.opam/4.08.0+beta3/.opam-switch/build/js_of_ocaml-ppx.3.3.0
# command              ~/.opam/4.08.0+beta3/bin/dune build -p js_of_ocaml-ppx -j 71
# exit-code            1
# env-file             ~/.opam/log/js_of_ocaml-ppx-1-7b04f3.env
# output-file          ~/.opam/log/js_of_ocaml-ppx-1-7b04f3.out
### output ###
# [...]
# stdlib-shims library: https://github.com/ocaml/stdlib-shims
# File "ppx/ppx_js/lib/ppx_js_internal.ml", line 430, characters 41-44:
# 430 |       Location.raise_errorf ~loc:id.loc ~sub
#                                                ^^^
# Error: This expression has type
#          Migrate_parsetree.OCaml_406.Ast.Location.error list
#        but an expression was expected of type
#          Migrate_parsetree.OCaml_406.Ast.Location.msg list
#        Type Migrate_parsetree.OCaml_406.Ast.Location.error = Location.report
#        is not compatible with type
#          Migrate_parsetree.OCaml_406.Ast.Location.msg =
#            (Format.formatter -> unit) Location.loc 

but in this case I'm guessing there were using the Location module directly from compiler-libs, and the latest version of this package works with 4.08.0+beta3 anyway.

There is also this one but I think it's the same problem (i.e. they use compiler-libs as well as omp and ppx_tools_versioned)

#=== ERROR while compiling ppxx.2.3.2 =========================================#
# context              2.0.3 | linux/x86_64 | ocaml-variants.4.08.0+beta3 | file:///home/opam/opam-repository
# path                 ~/.opam/4.08.0+beta3/.opam-switch/build/ppxx.2.3.2
# command              ~/.opam/4.08.0+beta3/bin/jbuilder build -p ppxx -j 71
# exit-code            1
# env-file             ~/.opam/log/ppxx-1-e77cc5.env
# output-file          ~/.opam/log/ppxx-1-e77cc5.out
### output ###
# Error: Unbound value Syntaxerr.report_error
# [...]
# (cd _build/default && /home/opam/.opam/4.08.0+beta3/bin/ocamlopt.opt -w -40 -w -9 -g -I src/.ppxx.objs/byte -I src/.ppxx.objs/native -I /home/opam/.opam/4.08.0+beta3/lib/ocaml-migrate-parsetree -I /home/opam/.opam/4.08.0+beta3/lib/ocaml/compiler-libs -I /home/opam/.opam/4.08.0+beta3/lib/ppx_derivers -I /home/opam/.opam/4.08.0+beta3/lib/ppx_tools_versioned -I /home/opam/.opam/4.08.0+beta3/lib/[...]
# File "src/compilerlib.ml", line 27, characters 67-85:
# 27 |   let format_verbose ppf id = fprintf ppf "%s/%d" (Ident.name id) (Ident.binding_time id)
#                                                                         ^^^^^^^^^^^^^^^^^^
# Error: Unbound value Ident.binding_time
#     ocamlopt src/.ppxx.objs/native/ppxx__Helper.{cmx,o}
# File "src/helper.ml", line 3, characters 5-12:
# 3 | open Ast_405
#          ^^^^^^^
# Alert deprecated: module Ast_405
# Access modules via the Migrate_parsetree toplevel module. Use Migrate_parsetree.Ast_405 instead.

Here is the full diff (there are also unrelated changes as it keeps track of all opam-repository, also note that the content of this page will change in about 2 days): http://check.ocamllabs.io/diff

Overall I don't know which packages to check to tell whether or not this PR is good as I'm not really a ppx user nor very knowledgable about ppx, but I'd say it looks pretty ok from the looks of it I have on opam-health-check. From this point of view only, this PR looks pretty good in my opinion.

xclerc commented 5 years ago

Thanks for the feedback!

I will have a look at the js_of_ocaml-ppx issue (@hhugo adapted the whole js_of_ocaml stack so it might just be a missing contraint in the opam repository).

Regarding ppxx, the package should be marked incompatible with 4.08+ as the Ident.binding_time function from the compiler is gone.

ghost commented 5 years ago

Alright, I didn't look at the new ast_408 or migration modules, but the rest of the changes look sensible to me and this PR has been tested, so I'm happy to merge it now.

ghost commented 5 years ago

Release submitted: https://github.com/ocaml/opam-repository/pull/14116