Port the ppx from omp to ppxlib

[pitag-ha]

Before, the ppx was directly based on omp: on the omp driver and on omp's Ast_411. Both of them will get dropped in omp 2.0.0. For more information on that omp change, see https://discuss.ocaml.org/t/ocaml-migrate-parsetree-2-0-0/5991 by @jeremiedimino.

That's why this commit ports the ppx from omp and ppx_tools_versioned to ppxlib.
Closes ocaml-ppx/ppxlib#131

[pitag-ha]

I have a question about the Bucklescript support: I'm not familiar with Bucklescript and it's divergence from OCaml. Does the support here still work? Is it worth making ppxlib be able to keep it?

[aantron]

BuckleScript support should work with this approach, as for BuckleScript support, we build the Bisect_ppx binaries using the opam ecosystem (though using esy as the package manager). However, it is broken in this PR at the moment: https://travis-ci.org/github/aantron/bisect_ppx/jobs/710719501.

Travis seems to not be reporting status to GitHub (a problem they have been having on and off for months now). The builds are running, however. They are at https://travis-ci.org/github/aantron/bisect_ppx.

[aantron]

I see that this makes Bisect_ppx transitively depend on Base. From discussions with @jeremiedimino and @rgrinberg, I got the impression that Base would eventually not be a dependency of ppxlib or other future PPX helper library. Is that still the case? I strongly prefer not to introduce Base as a dependency of Bisect_ppx.

[ghost]

@aantron the depencency on Base has been dropped in the master of ppxlib. The next release will not depend on Base.

[pitag-ha]

Thanks for the answer @aantron and for pointing me to the failing tests. We will integrate the BuckleScript support into ppxlib and once that's done apply it here.

I've been trying to figure out why the BuckleScript tests fail, but haven't found the connection to what I've ported, yet. I haven't touched that part of the code. For example it says that the -conditional option is not found. That part is still using the omp driver. And that ""make -C test/bucklescript install" failed. I'm not sure how I might have influenced that. Do you happen to have an idea how I can have triggered those errors by only porting the OCaml ppx?

I will leave the PR marked as WIP until the next ppxlib release to avoid the Base dependency.

[aantron]

I'm not familiar enough with ppxlib to immediately say what caused that error. I did rerun the CI of master to (apparently) rule out changes upstream as the cause.

[pitag-ha]

I hope the failure of the bucklescript tests will get solved once ppxlib has an integrated bucklescript support. I have a couple of bucklescript unrelated questions in the meanwhile if you don't mind: I don't understand the signature method of the instrumentor class. I have two questions about that:

1. In the comments it says that the signature method will only set a variable to true and do nothing else. But after setting that variable to true, it recurses through the AST. Is that intentional?
2. Why is it necessary to have that signature method? Is it a problem to insert the Bisect_ppx initialization code into a struct of an interface file? As far as I've been told, it would just get ignored by the compiler. Are there cases where that's not true?

I'm just asking because I've implemented the possibility to register a ppx as an instrumentor in ppxlib (which allows one to specify if one wants the instrumentor transformation to be applied before or after the other transformations). But that implementation assumes that an instrumentor only passes through implementation files.

[aantron]

Blame shows the code was added in #164, so:

Is it a problem to insert the Bisect_ppx initialization code into a struct of an interface file?

It was a problem in the past, in #164. Bisect_ppx would insert initialization code into structures that appeared inside attributes in interface files.

As far as I've been told, it would just get ignored by the compiler. Are there cases where that's not true?

Indeed, it should get ignored by the compiler, but attribute payloads are interpreted by other PPXs, and inserting Bisect_ppx initialization code into them would interfere with those PPXs, as happened in #164.

This code might be obsolete, because we now have an additional way of not instrumenting attributes:

bisect_ppx/src/ppx/instrument.ml

Lines 1478 to 1483 in a4b8cb6

	(* Don't instrument payloads of extensions and attributes. *)
	method! extension e =
	e
	method! attribute a =
	a

It probably got left in as a form of extra defensive programming. During and after #164, Bisect_ppx still had some convoluted way of avoiding attributes that evidently wasn't working.

If you intend to avoid applying Bisect to interface files by other means, this method would become even more redundant.

The recursive call in the method probably dates from the time when Bisect_ppx was still trying to do something fancy with attributes, since, to my knowledge, attributes are still the only way "instrumentable" code can appear inside a signature (and we don't instrument it now anyway).

[pitag-ha]

Thanks a lot for that detailed answer! I've avoided applying Bisect to interface files via the ppxlib API now. I've also used that API to do the work that was done by the saw_top_level_structure_or_signature variable before . Please, let me know if you don't like that change. I don't have any problem in reverting it.

[aantron]

The code looks good, with a couple comments.

Please don't force-push into this PR until the end, it complicates incremental review :) We can rewrite the history as necessary at the very end, probably with just a squash-merge.

[pitag-ha]

I had to exclude the test/usage/reason directory from the dune build, since the ppxlib version I need has a conflict with reason. Is that ok? That problem has always been there, but I've noticed it now after merging your dune2 integration.

I've also commented out the bucklescript integration, since it's not working the way it is now. I think that's why the CI is failing now. I'll fix it as soon as ppxlib supports bucklescript.

[aantron]

Thanks, that all sounds reasonable. It's ok to disable the Reason test. Hopefully, we can enable it again after some time and a few releases of the upstream projects.

[aantron]

I think Bisect needs this PR for proper, maintainable 4.12 support. What's the status of the PR and ppxlib now?

[pitag-ha]

I'm integrating the possibility into ppxlib to access the input_name, so that we don't need to use compiler-libs for that. That's almost done. Afterwards I need to implement bucklescript support for ppxlib. I don't know yet how much work that will be. But knowing that this is needed for 4.12 support I'll prioritize ppxlib and this PR and try to go faster.

[pitag-ha]

@aantron , can I rebase over master and force-push now to see if the CI passes? I've fixed the bucklescript support in Ppxlib and made the input name accessible via Ppxlib in whole file transformations. And I've adapted the PR to those two changes.

[pitag-ha]

Ok, I've merged master into this branch. I'm having two problems now:

• In my last commit, I changed the version of ocamlformat from 0.15.0 to 0.15.1 to have it compatible with omp2. But I've just noticed that you're using a binary from your fork (so that won't work). Would it be possible to add ocamlformat 0.15.1 or 0.16.0 to your fork to have omp2 compatibility?

• There's one test that's failing locally at the moment (test/instrument/class/). It's also failing in master.

[pitag-ha]

And a third problem: the bucklescript test installation fails on the CI, so I can't see if the bucklescript test would pass or not (locally it did). Do you happen to know what fails in the installation?

[aantron]

• In my last commit, I changed the version of ocamlformat from 0.15.0 to 0.15.1 to have it compatible with omp2. But I've just noticed that you're using a binary from your fork (so that won't work). Would it be possible to add ocamlformat 0.15.1 or 0.16.0 to your fork to have omp2 compatibility?

Bisect never used 0.15.0, but a specific commit of OCamlFormat, as listed in its dependencies:

bisect_ppx/bisect_ppx.opam

Lines 29 to 36 in 2fc5b79

	"ocamlformat" {with-test & = "git.1bf68a7"}
	]
	pin-depends: [
	# This is the last commit before ocamlformat's ppxlib conversion. A newer
	# commit, or no pin at all, can be used once Bisect_ppx is also converted to
	# ppxlib.
	["ocamlformat.git.1bf68a7" "git+https://github.com/ocaml-ppx/ocamlformat.git#1bf68a70f1480df80a8ab0bd20a799b8e1df0081"]

This commit is closer to 0.16.0 than to 0.15.0 or 0.15.1. Looking in the OCamlFormat repo, it was the latest commit prior to the (then-unreleased) 0.16.0 before the switch to omp2. So, you should switch to 0.16.0 instead of 0.15.1. 0.15.1 is behavior-identical with 0.15.0, and is definitely incompatible with 0.16.0 for Bisect's purposes. 0.15.0 is buggy with Bisect's tests — that's why I had the need to use a git commit in the first place, and AFAIK 0.15.1 preserves the same bugs, just over omp2.

As for the binary, it is a build of the same git commit. I used a misleading tag name for the binary for hosting it on GitHub (I just chose what was the most recent tag in the OCamlFormat repo at the time; looking back, I should have probably given it part of the OCamlFormat commit hash). In any case, you shouldn't have to change the binary, as it should behave the same as 0.16.0 already, and its omp2/non-omp2 dependencies don't matter, as it's pre-built.

You should be able to just change dune to have "ocamlformat" {with-test & = "0.16.0"} and remove pin-depends. I did this locally in testing, and it seems to work.

• There's one test that's failing locally at the moment (test/instrument/class/). It's also failing in master.

I can't reproduce that — you may be seeing this if you are using OCamlFormat 0.15.1, rather than either the git commit that is listed in Bisect's dune and would have been installed by opam install --deps-only --with-test ., or 0.16.0. If changing to these versions doesn't fix the problem in master, could you show the message you get, and confirm the exact OCamlFormat version and/or git commit you have installed when receiving it?

[aantron]

And a third problem: the bucklescript test installation fails on the CI, so I can't see if the bucklescript test would pass or not (locally it did). Do you happen to know what fails in the installation?

The BuckleScript issue looked like a caching/upstream conflict, so I cleared the cache some days ago, and your most recent commit has already passed the BuckleScript tests :)

The opam tests look like they will be fixed, or at least advanced, by updating the OCamlFormat dependency in dune to 0.16.0, and by restoring .travis.yml to download the existing pre-built binary (as suggested above).

[aantron]

Overall, the PR looks just about ready, except for the changes from the comments above. So, I added a couple last nits :P

[pitag-ha]

If changing to these versions doesn't fix the problem in master, could you show the message you get, and confirm the exact OCamlFormat version and/or git commit you have installed when receiving it?

I have Ocamlformat 0.16.0 installed, but the test that's failing doesn't seem to be related to ocamlformat. I'm getting the following on master:

+++ b/test/instrument/class/class.t.corrected
@@ -56,9 +56,9 @@ Nested expressions and initializers instrumented.
   >   end
   > EOF
   class foo =
-    let () = ___bisect_post_visit___ 2 (print_endline "bar") in
+    let () = ___bisect_post_visit___ 0 (print_endline "bar") in
     object
       initializer
-      ___bisect_visit___ 1;
-      ___bisect_post_visit___ 0 (print_endline "baz")
+      ___bisect_visit___ 2;
+      ___bisect_post_visit___ 1 (print_endline "baz")
     end

edit: never mind this! It's not true. I've just noticed that without downgrading omp, dune just returns the same test result as before since it can't build master. So it turns out that on master everything is fine. I'll try to figure out how I've broken that test.

[aantron]

I'll try to figure out how I've broken that test.

This doesn't look like a real failure, but just a different order of traversal used by ppxlib. You should be able to just do make promote, commit it, and continue.

[aantron]

Looking ahead, you will probably need to remove the 4.02.3 and 4.03.0 rows in Travis. Then, you should take 4.04.2 out of allow_failures so that it becomes the new lowest version in the required-to-suceed builds. There are also various mentions of 4.02.3 and 4.03.0 throughout .travis.yml that can be cleaned up at that point.

[aantron]

Thanks @pitag-ha for your work here, and upstream in ppxlib!