Ppxlib.0.26.0 compatibility

[pitag-ha]

This is a patch PR to make the PPX compatible with ppxlib.0.26.0 which has bumped the AST to 4.14/5.00.

[pitag-ha]

I've just added a cmdliner constraint, thinking that that might fix the CI given that the CI was failing exactly on the compiler versions on which cmdliner.1.1.0 can be installed. However, that hasn't fixed the CI. So I've reverted that.

So I've looked into the CI failures. There are three different kinds of failures:

The first one occurs on macos-latest and windows-latest and seems to concern ocamlformat. Is it possible that you only have an ocamlformat binary for linux, so on macos and windows, ocamlformat has to be installed via opam and therefore downgrades ppxlib? If so, that has always been a problem, hasn't it? Or is that new with this PR?

The second one is about

dune build ./not_excluded.bc --instrument-with bisect_ppx 2>&1

On old compilers, that outputs a line before printing the instrumented code, but on new compilers (>= 4.08) it doesn't. Has that always been the case? If not, I'm wondering if that might be somehow related to dune3.

The third one is the only worrying I'd say and I have no idea where it comes from:

$ echo "(lang dune 2.7)" > dune-project
$ cat > dune <<'EOF'
> (executables
> (names normal)
> (preprocess (pps bisect_ppx --bisect-sigterm)))
> EOF
$ cat > normal.ml <<'EOF'
> let () = ()
> EOF
$ dune exec ./normal.exe
$ bisect-ppx-report summary --verbose

used to yield 0% coverage and now it yields 100% (only on ocaml>=4.08). Do you happen to have some guess what might be happening there?

[pitag-ha]

Hey @aantron, any chance that you'll have time to have a look at this soon? For me, it would be great if you could have a quick glimpse at the CI to let me know if it's my PR that's breaking the CI (for more details, see my questions in the last comment). Of course, if my PR is breaking it, I'll look into it in more detail to fix it.

[ceastlund]

Ping, @aantron? There are further changes to ppxlib that would require bisect_ppx fixes. We can't update further until this PR goes out. Hope you have time to look at it soon!

[quinn-dougherty]

used to yield 0% coverage and now it yields 100%

In local development (on #405 branch), I'm getting this as well in the diff tests, it's the only problem left with make test

[pitag-ha]

Thanks for pointing that out @quinn-dougherty! I've just had a look and get the same diff (about coverage in test/sigterm/sigterm.t) on master. So maybe this PR can be merged, @aantron?

[pitag-ha]

Hey @aantron , there start to be more and more problems due to having this PR pending, see e.g. this issue and this comment. Would it be possible to merge this?

[barracuda156]

Just got this error:

File "src/ppx/instrument.mli", line 8, characters 11-57:
8 |    inherit Ppxlib.Ast_traverse.map_with_expansion_context
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Unbound class type Ppxlib.Ast_traverse.map_with_expansion_context

[ceastlund]

It has been replaced by map_with_expansion_context_and_errors. It will take a little plumbing to propagate the errors the new way, but beyond that it does the same job.

[barracuda156]

@ceastlund Thank you! For now I just replaced it with the new name (OCaml is not an area of my expertise at all), but it failed a bit further on:

File "src/ppx/instrument.ml", line 1049, characters 12-14:
1049 |       match ce.pcl_desc with
                   ^^
Error: This expression has type Ppxlib__.Import.Ast.class_expr *
                                Ppxlib__.Location.Error.t list
       which is not a record type.

[ceastlund]

Ppxlib exports a With_errors submodule to help propagate those bindings via >>| and >>= operators. Someone familiar with that convention will have to update the code. It's not hard for someone who's used OCaml monads before, but if you're new to it it'll be involved.

[pitag-ha]

Hi @barracuda156 and @ceastlund, given the problems that currently make it very difficult to have ppx_bisect actively maintained, there are already people who have added ppxlib.0.28.0 support on their own bisect_ppx fork. For example, @anmonteiro: https://github.com/anmonteiro/bisect_ppx/commit/cc442a08e3a2e0e18deb48f3a696076ac0986728. I think, there, possible errors aren't handled, but the code compiles.

I was originally thinking that it would be good, first to merge and release this PR to have a bisect_ppx version compatible with ppxlib 0.26.0 and 0.27.0 and then to merge and release a bisect_ppx PR adding 0.28.0 support. But at this point, I think we can just mix it all into one PR. I will see when I find time for that. In the meanwhile, feel free to open a PR, either to my branch or directly.

[pitag-ha]

Hi @aantron, a kind reminder and update on this: This PR makes bisect_ppx compatible with the latest compilers. It's also the first step to making it compatible with the latest ppxlib again.

[aantron]

@pitag-ha Thanks to you and everyone! I am working through bit rot issues in some of my other repos, and will reach Bisect_ppx in a few days. Then I will fix this and any other issues. Thanks for all the discussions, info, and links to date!

[aantron]

Thanks for your work and patience!

This PR looks good. I see the same failures in CI (and locally). They are not related to this PR. I'll work to address them outside this PR.

[aantron]

FYI it appears that after this PR, the right upper constraint is < "0.28.0":

File "src/ppx/instrument.mli", line 8, characters 11-57:
8 |    inherit Ppxlib.Ast_traverse.map_with_expansion_context
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Unbound class type Ppxlib.Ast_traverse.map_with_expansion_context

However, I'll probably try to port Bisect to 0.28.0 rather than add the constraint. @pitag-ha, do you know if people need a release of Bisect that is compatible with ppxlib 0.26.0 and 0.27.0, at this point?

[pitag-ha]

FYI it appears that after this PR, the right upper constraint is < "0.28.0":

Good catch, thanks!

@pitag-ha, do you know if people need a release of Bisect that is compatible with ppxlib 0.26.0 and 0.27.0, at this point?

I think ppxlib >= 0.28.0 support directly should be enough.

[aantron]

@pitag-ha, as the ppxlib maintainer, could you comment on the correctness of patches like https://github.com/anmonteiro/bisect_ppx/commit/cc442a08e3a2e0e18deb48f3a696076ac0986728 for 0.28.0? I am ready to review them from the Bisect point of view and cherry-pick them.

[pitag-ha]

@pitag-ha, as the ppxlib maintainer, could you comment on the correctness of patches like https://github.com/anmonteiro/bisect_ppx/commit/cc442a08e3a2e0e18deb48f3a696076ac0986728 for 0.28.0?

Thanks for reaching out on that! https://github.com/anmonteiro/bisect_ppx/commit/cc442a08e3a2e0e18deb48f3a696076ac0986728 doesn't handle the errors, but drops them. So that was good to have something that would build. However for something we want to merge and release, we should really handle the errors instead of dropping them. You can use the Ppxlib.With_errors module to comfortably propagate the errors.

E.g. instead of

method! expression ctxt e =
   (...)
   let cstr, _locs = (self#class_structure ctxt c) in
   Exp.object_ ~loc ~attrs cstr

you can use the With_error's map:

method! expression ctxt e =
   (...)
   self#class_structure ctxt c >>| fun cstr -> Exp.object_ ~loc ~attrs cstr

And then, at the level of the structure node, you can embed all errors, i.e. instead of

let instrumented_ast, _errs = super#structure ctxt ast in
let runtime_initialization =
  Generated_code.runtime_initialization points path in
runtime_initialization @ instrumented_ast

something along the lines of

let instrumented_ast, errs = super#structure ctxt ast in
let errs =
   List.map (fun error ->
      Ast_builder.Default.pstr_extension
        ~loc:(Location.Error.get_location error)
        (Location.Error.to_extension error)
        []) errs
in
let runtime_initialization =
  Generated_code.runtime_initialization points path in
errs @ runtime_initialization @ instrumented_ast

[pitag-ha]

Let us know if you have any questions about that!