Support ppxlib.0.36.0

[patricoferris]

This is a draft PR bump to bisect_ppx to the latest ppxlib.

I think this is realistically going to need #445 and I'll need to look into some of the failing tests (some are formatting/pretty-printing related I think).

[kit-ty-kate]

@patricoferris i believe this can be undrafted now

[glondu]

Any news?

[Kakadu]

@aantron ^^

[NathanReb]

I think this patch is causing #450. There's something fishy going on when instrumenting Pfunction_cases that can lead to it being converted to a Pfunction_body which triggers the compiler invariant in the linked issue.

I'll look into it and push a fix!

Once that's done I'm also happy to help getting a release in any way I can @aantron !

[NathanReb]

Opened a PR to @patricoferris' branch here: https://github.com/patricoferris/bisect_ppx/pull/1

[NathanReb]

The test suite is broken at the moment so I tried to make the minimal set of changes required to make my new regression test work, for anybody willing to try it out, you can run just the one test with:

dune runtest test/instrument/function.t

to get a clean output that's not polluted by the other errors.

I'd also be happy to contribute a refresh of the test suite and to help with the project maintenance. Don't hesitate to reach out if you're looking for a bit of help on that front!

[patricoferris]

Pulled your changes in @NathanReb -- thanks !

[aantron]

I took a look at all the background today and I'm going to continue with reviewing the code in the coming days.

[aantron]

Links for the future:

• The OCaml RFC that caused this https://github.com/ocaml/RFCs/pull/32
• The OCaml PR https://github.com/ocaml/ocaml/pull/12236, in particular https://github.com/ocaml/ocaml/commit/1fdfc412e4f8735cf27d5fa176fedbea49d8b132 and https://github.com/ocaml/ocaml/commit/437548629a74408515ad0059d417fe2091d9715b
• Ppxlib PR https://github.com/ocaml-ppx/ppxlib/pull/451

[kit-ty-kate]

FYI: OCaml 5.4.0 was released yesterday

[aantron]

The test suite presently works for master on 4.08-4.13 with ppxlib 0.35.0, including in CI. I just merged master into this PR, to have the CI test ppxlib's translation of what this PR does to older versions. Could you pull these commits down and comment on the failures?

dune build -p bisect_ppx
NO_COLOR=yes dune build -p bisect_ppx @runtest
File "test/instrument/control/function.t", line 1, characters 0-0:
/usr/bin/git --no-pager diff --no-index --color=always -u _build/default/test/instrument/control/function.t _build/default/test/instrument/control/function.t.corrected
diff --git a/_build/default/test/instrument/control/function.t b/_build/default/test/instrument/control/function.t.corrected
index bfb978d..a39693a 100644
--- a/_build/default/test/instrument/control/function.t
+++ b/_build/default/test/instrument/control/function.t.corrected
@@ -59,6 +59,7 @@ Or-pattern.
   > EOF
   let _ =
    fun ___bisect_matched_value___ ->
+    ___bisect_visit___ 2;
     match ___bisect_matched_value___ with
     | None | Some _ ->
         (match[@ocaml.warning "-4-8-9-11-26-27-28-33"]
File "test/instrument/control/newtype.t", line 1, characters 0-0:
/usr/bin/git --no-pager diff --no-index --color=always -u _build/default/test/instrument/control/newtype.t _build/default/test/instrument/control/newtype.t.corrected
diff --git a/_build/default/test/instrument/control/newtype.t b/_build/default/test/instrument/control/newtype.t.corrected
index 630a122..12927d2 100644
--- a/_build/default/test/instrument/control/newtype.t
+++ b/_build/default/test/instrument/control/newtype.t.corrected
@@ -28,6 +28,6 @@ Subexpression in tail position iff whole expression is in tail position.
   let _ = fun (type _t) -> ___bisect_post_visit___ 0 (print_endline "foo")
   
   let _ =
-   fun () ->
+   fun () (type _t) ->
     ___bisect_visit___ 1;
-    fun (type _t) -> print_endline "foo"
+    print_endline "foo"
File "test/instrument/pattern/exception.t", line 1, characters 0-0:
/usr/bin/git --no-pager diff --no-index --color=always -u _build/default/test/instrument/pattern/exception.t _build/default/test/instrument/pattern/exception.t.corrected
diff --git a/_build/default/test/instrument/pattern/exception.t b/_build/default/test/instrument/pattern/exception.t.corrected
index 5c660dc..e383838 100644
--- a/_build/default/test/instrument/pattern/exception.t
+++ b/_build/default/test/instrument/pattern/exception.t.corrected
@@ -38,10 +38,12 @@ patterns.
   >   | _ -> print_endline "baz"
   > EOF
   let _ =
-    let ___bisect_case_0___ x () =
+    let ___bisect_case_0___ x =
+     fun [@ppxlib.migration.stop_taking] () ->
       ignore x;
       ___bisect_post_visit___ 0 (print_endline "foo")
-    and ___bisect_case_1___ y () =
+    and ___bisect_case_1___ y =
+     fun [@ppxlib.migration.stop_taking] () ->
       ignore y;
       ___bisect_post_visit___ 1 (print_endline "bar")
     in
File "test/instrument/recent/gadt.t", line 1, characters 0-0:
/usr/bin/git --no-pager diff --no-index --color=always -u _build/default/test/instrument/recent/gadt.t _build/default/test/instrument/recent/gadt.t.corrected
diff --git a/_build/default/test/instrument/recent/gadt.t b/_build/default/test/instrument/recent/gadt.t.corrected
index d65beb4..789f6d4 100644
--- a/_build/default/test/instrument/recent/gadt.t
+++ b/_build/default/test/instrument/recent/gadt.t.corrected
@@ -39,6 +39,7 @@ With function.
   
   let f : type a. a t -> unit =
    fun ___bisect_matched_value___ ->
+    ___bisect_visit___ 2;
     match ___bisect_matched_value___ with
     | A | B ->
         (match[@ocaml.warning "-4-8-9-11-26-27-28-33"]
File "test/instrument/value.t", line 1, characters 0-0:
/usr/bin/git --no-pager diff --no-index --color=always -u _build/default/test/instrument/value.t _build/default/test/instrument/value.t.corrected
diff --git a/_build/default/test/instrument/value.t b/_build/default/test/instrument/value.t.corrected
index 2d9e0fa..58562b7 100644
--- a/_build/default/test/instrument/value.t
+++ b/_build/default/test/instrument/value.t.corrected
@@ -194,7 +194,11 @@ No instrumentation is inserted into expressions that are (syntactic) values.
   
   let _ = (___bisect_post_visit___ 1 (f ()) :> [ `Foo | `Bar ])
   
-  let _ = fun () -> (f () :> [ `Foo | `Bar ])
+  let _ =
+   fun () ->
+    (___bisect_visit___ 2;
+     f ()
+      :> [ `Foo | `Bar ])
 
 
   $ bash test.sh <<'EOF'
make: *** [Makefile:13: test] Error 1

[aantron]

This difference

diff --git a/_build/default/test/instrument/value.t b/_build/default/test/instrument/value.t.corrected
index 2d9e0fa..08f2932 100644
--- a/_build/default/test/instrument/value.t
+++ b/_build/default/test/instrument/value.t.corrected
@@ -194,7 +194,10 @@ No instrumentation is inserted into expressions that are (syntactic) values.
   
   let _ = (___bisect_post_visit___ 1 (f ()) :> [ `Foo | `Bar ])
   
-  let _ = fun () -> (f () :> [ `Foo | `Bar ])
+  let _ =
+   fun () ->
+    ___bisect_visit___ 2;
+    (f () :> [ `Foo | `Bar ])
 
 
   $ bash test.sh <<'EOF'

appears in the 4.14 and 5.0.0 run in master, without this PR, so probably not caused by it. IIRC I have a commit in a branch locally that suppresses this, so I'll push that in later. Don't spend time addressing this one :)

[aantron]

I'd also be happy to contribute a refresh of the test suite and to help with the project maintenance. Don't hesitate to reach out if you're looking for a bit of help on that front!

Would be very glad to merge updates to the test suite, and help with project maintenance. What would you like to do? Do you need write access to the repo?

[patricoferris]

Thanks @aantron.

-    let ___bisect_case_0___ x () =
+    let ___bisect_case_0___ x =
+     fun [@ppxlib.migration.stop_taking] () ->
       ignore x;
       ___bisect_post_visit___ 0 (print_endline "foo")
-    and ___bisect_case_1___ y () =
+    and ___bisect_case_1___ y =
+     fun [@ppxlib.migration.stop_taking] () ->
       ignore y;
       ___bisect_post_visit___ 1 (print_endline "bar")
     in

these failures are a known bug that is fixed upstream, see https://github.com/ocaml-ppx/ppxlib/pull/604.

The others look like some interaction with the new function layout in the parsetree (which I may have not implemented correctly).

diff --git a/_build/default/test/instrument/control/newtype.t b/_build/default/test/instrument/control/newtype.t.corrected
index 630a122..12927d2 100644
--- a/_build/default/test/instrument/control/newtype.t
+++ b/_build/default/test/instrument/control/newtype.t.corrected
@@ -28,6 +28,6 @@ Subexpression in tail position iff whole expression is in tail position.
  let _ = fun (type _t) -> ___bisect_post_visit___ 0 (print_endline "foo")
  
  let _ =
-   fun () ->
+   fun () (type _t) ->
    ___bisect_visit___ 1;
-    fun (type _t) -> print_endline "foo"
+    print_endline "foo"

This looks like we go all the way to the end of the functions params before inserting the bisect code. Before ppxlib.0.36 this would have been Pexp_fun (x, Pexp_newtype (t ... )) but now it is Pexp_function([arg x; newtype t]...) so that might be causing this.

diff --git a/_build/default/test/instrument/control/function.t b/_build/default/test/instrument/control/function.t.corrected
index bfb978d..a39693a 100644
--- a/_build/default/test/instrument/control/function.t
+++ b/_build/default/test/instrument/control/function.t.corrected
@@ -59,6 +59,7 @@ Or-pattern.
   > EOF
   let _ =
    fun ___bisect_matched_value___ ->
+    ___bisect_visit___ 2;
     match ___bisect_matched_value___ with
     | None | Some _ ->
         (match[@ocaml.warning "-4-8-9-11-26-27-28-33"]

This looks similar, the representation before would have been Pexp_fun(__bisect..., Pexp_function(cases) whereas now it is Pexp_function([__bisect...], Pfunction_cases(cases))

[aantron]

```diff
-    let ___bisect_case_0___ x () =
+    let ___bisect_case_0___ x =
+     fun [@ppxlib.migration.stop_taking] () ->
       ignore x;
       ___bisect_post_visit___ 0 (print_endline "foo")
-    and ___bisect_case_1___ y () =
+    and ___bisect_case_1___ y =
+     fun [@ppxlib.migration.stop_taking] () ->
       ignore y;
       ___bisect_post_visit___ 1 (print_endline "bar")
     in

these failures are a known bug that is fixed upstream, see ocaml-ppx/ppxlib#604.

That looks like the wrong issue link but I was able to find https://github.com/ocaml-ppx/ppxlib/issues/597 from your hint, thank you. Leaving this comment here so that we have a link to that issue for future reading :)

[patricoferris]

Thanks @aantron for catching that!

I have pushed a commit ebb3526 that fixes the control/function.t and control/newtype.t. The fix was to match more closely how the original instrumentation caught let f = function A -> ... sooner as there was a dedicated parsetree node for this. Now it is hidden within a Pexp_function with no parameters.

[mbarbin]

Thank you all for the work on this PR!

In the meanwhile, I have found great value in using https://github.com/kit-ty-kate/opam-alpha-repository Thanks!

@kit-ty-kate may I request that you add a new preview package for bisect_ppx which would include the latest fix for https://github.com/aantron/bisect_ppx/issues/450 this would allow continue the 5.4 testing while things settles. I am not completely sure it's possible to find a candidate version though (just in case this is possible to find such a revision to build from). Thank you!

[AllanBlanchard]

Apparently, the fix is not robust enough. It still does not work with the following example:

[@@@warning "-37"]
type 'a bt = (int * 'a) list
type x

type ('a, 'b) t =
  | B  : 'a bt -> ('a, 'b) t
  | C : ('a, 'b) t
  | D : ('a, 'b) t

let _action (f  : int -> 'a -> 'b -> 'b) (acc : 'b) : ('a, x) t -> 'b =
  function
  | C | D -> acc
  | B l -> List.fold_left (fun acc (cs,x) -> f cs x acc) acc l
[@@@warning "+37"]

Without coverage, it compiles, with bisect we get the following error:

File "file.ml", line 20, characters 13-16:
20 |   | C | D -> acc
                  ^^^
Error: The value acc has type 'b but an expression was expected of type
         ('a, x) t -> 'b
       The type variable 'b occurs inside ('a, x) t -> 'b

This is a reduced example taken from Frama-C. I can point out the actual file if it is useful.

[NathanReb]

@AllanBlanchard thanks for the minimization, I confirm I can reproduce the error with it and am working on a fix!

[NathanReb]

Just opened https://github.com/patricoferris/bisect_ppx/pull/2 which should fix the error @AllanBlanchard !

[kit-ty-kate]

@mbarbin done: https://github.com/kit-ty-kate/opam-alpha-repository/commit/c9bdcd12a7ceec641ed54122bcee0c3b65980231

[mbarbin]

@mbarbin done: kit-ty-kate/opam-alpha-repository@c9bdcd1

@kit-ty-kate Very nice. I re-run a few CIs to pick up this new pkg and I can confirm this fixed the bisect_ppx related issues with 5.4 I had seen in these PRs. Continuing with the 5.4 transition here. Thanks a lot, and thank you too to maintainers and contributors for the work!

[NathanReb]

@aantron what are the remaining blockers on this PR?

[patricoferris]

Thanks @NathanReb! Merged into this branch.

[AllanBlanchard]

Just opened patricoferris#2 which should fix the error @AllanBlanchard !

Tested today, we can compile and test Frama-C with this fix. There are a few micro-differences on some counters but it changes global coverage by less than 0.05%.

Thanks a lot for that!

[aantron]

Thanks everyone! I will review it in its current state shortly, and if there are ~~no~~ any* obvious problems I'll try to fix them myself, then do a merge and release.