Problem using with Esy/Reason/Dune

[bandersongit]

I haven't been able to get bisect_ppx working with my current setup (Dune/Esy/Reason). I've tried adopting the dune instructions from the advanced section of the docs and I am pretty sure after examination that
#{self.target_dir}/build is what I want to pass to the -I flag, and I know that #132 is still open, however I don't think anything mentioned there should be relevant.

Repo: https://github.com/bandersongit/rely-talk/tree/bisect_ppx-integration

The README.md on the bisect_ppx-integration branch has a fairly detailed description.

I'm really excited about being able to use bisect_ppx for a bunch of my projects and would love to get this working.

[aantron]

The immediate problem of <command-line>: No such file or directory can be bypassed by adding -ignore-missing-files:

--- a/package.json
+++ b/package.json
@@ -11,7 +11,7 @@
   },
   "scripts": {
     "test": "esy x RunTests.exe",
-    "coverage": "esy bisect-ppx-report -I '#{self.target_dir}/default/' -html coverage/ bisect0001.out -verbose"
+    "coverage": "esy bisect-ppx-report -I '#{self.target_dir}/default/' -html coverage/ bisect0001.out -ignore-missing-files -verbose"
   },
   "dependencies": {
     "@opam/dune": "*",

However, this results in an empty coverage report, and the two problems are probably connected. Looking further...

[aantron]

So far, this isn't an esy problem; I can reproduce it in a "pure-Dune" project. Still looking into whether it is Dune or Reason, my guess is the latter. A bit busy, but hope to have an answer to this soon.

[aantron]

This is what seems to be happening:

• When not using Reason, Dune runs the following commands:

  # build ppx.exe
  ppx.exe -o ... -impl foo.ml
  

  foo.ml is the original source file.

• When using Reason, Dune does this instead:

  # build ppx.exe
  refmt --print binary foo.re > foo.re.ml
  ppx.exe -o ... -impl foo.re.ml
  

  foo.re.ml is a binary dump of the AST produced by refmt.

ppx.exe is an ocaml-migrate-parsetree driver. It behaves differently with respect to Location.input_name from compiler-libs, depending on whether it is reading a binary AST or a source file:

• For a regular source file, it sets Location.input_name to the file's name.

• For a binary AST, it doesn't set Location.input_name, so Location.input_name retains its previous value. This can be either its initial value, "_none_", or "<command-line>", a string that comes from elsewhere in OMP.

Bisect_ppx uses Location.input_name to decide the source file name, so using it with Dune, Reason, and OMP doesn't work as a result.

I think Bisect_ppx has to use Location.input_name, as we cannot rely on locations in the AST in the presence of # directives and code generated by other PPXs running before Bisect_ppx.

It seems that the solution should be to have OMP set Location.input_name in all cases, or to have OMP pass the original file name to Bisect_ppx.

However, with the way Dune calls ppx.exe in the Reason case, it seems that OMP doesn't have access to the source file name anyway.

@rgrinberg and/or @diml, what do you think should be done to address this?

[ghost]

The original source file name is marshaled along the binary ast, so normally that shouldn't be an issue. I remember that we fixed something related to that in ppxlib, could you try adding ppxlib to the preprocess list to see if it fixes the issue? If yes, then we will need to apply the same change to omp.

[aantron]

Adding ppxlib produces

Processing file 'foo.re.ml'...
 *** system error: foo.re.ml: No such file or directory

which is better, as it seems that ppxlib sets input_name. However, instead of setting it to the name of the original file, it sets it to the name of the AST dump, which is still wrong, at least for Bisect_ppx.

[ghost]

The original filename is stored in the binary ast at the beginning. Could you inspect foo.re.ml to see what filename it contains?

[aantron]

Yes, I'm aware, and it is foo.re.

[ghost]

Ok, and what about in the output of ppx.exe -o ...?

[ghost]

Actually, I missed a detail in your previous comment. When you say Bisect_ppx uses Location.input_name, this is when it is linked as part of ppx.exe? In this case, then yes fixing omp is the only solution.

[aantron]

Yes, when linked as part of ppx.exe. I guess I hadn't specified :/

[ghost]

I wrote the PRs for omp and ppxlib, testing would be welcome :)

[ulrikstrid]

@diml @aantron I can confirm that this works on a mac. It actually works if I just add resolutions to those changes without putting ppxlib before bisect_ppx.

[aantron]

I can confirm that the OMP change works, too. I didn't try the ppxlib change because Bisect_ppx doesn't depend on it (and I don't have a project handy that does). Thanks, @diml!

[aantron]

I'm closing this issue in favor of asking people to track ocaml-ppx/ocaml-migrate-parsetree#66. This should be fixed by an OMP release. Thanks for the report!

[bandersongit]

@diml I think I found a case where the omp change isn't working, far from a minimal example, but tough to repro otherwise https://github.com/facebookexperimental/reason-native/tree/benderson/play-with-bisect_ppx .

Dune's copy_files wasn't working (separate problem, but kinda understandable) so I moved the file that was being copied directly into the library and am running into (I think) what @aantron hit earlier.

(the minimal example I posted with this PR works now for me too).

[aantron]

@bandersongit At first glance, this looks unrelated to Bisect_ppx. Would you mind trying to preprocess the files with any other PPX (e.g., ppx_let)?

[bandersongit]

@aantron Using ppx_let worked successfully, I also was able to use copy_files with ppx_let.

[ghost]

Ok, since ppx_let is using ppxlib, then I guess the omp patch is not enough.

[ghost]

I can't really make sense of the console output. Does bisect_ppx accesses the original file at preprocessing time?

[ghost]

BTW, just a thought: this pipeline (refmt --> ppx) is too complicated. We could simply statically link in the refmt parser in the ppx driver. That would simplify things.

[aantron]

@bandersongit, @diml is correct that Bisect_ppx accesses the original source file during preprocessing. To fix that, try adding a resolution to the commit linked above this comment, and making this change:

--- a/src/rely/dune
+++ b/src/rely/dune
@@ -1,7 +1,7 @@
 (library
    (name Rely)
    (public_name rely.lib)
-   (preprocess (pps bisect_ppx))
+   (preprocess (pps bisect_ppx -no-comment-parsing))
    (libraries pastel.lib file-context-printer.lib unix junit re)
 )
 (copy_files# ../../shared-src/common/*)

The reason for the source file access is that Bisect_ppx supports special comments like (*BISECT-IGNORE*) for excluding individual lines from coverage analysis. This is a holdover from pre-PPX days, and we actually have wanted to remove it for years (#83), but we haven't gotten around to doing it. The comment support is growing increasingly problematic.

We'll probably drop comment support soon-ish. The -no-comment-parsing flag should continue to work, we'll just have it do nothing at that point.

I'll merge this into master once CI passes.

[ghost]

Ok. At least the filename seems to be correct in the error message, which makes me think that the omp change is doing what it is supposed to, so I'll merge the PR

[bandersongit]

@aantron using the resolution to that commit and the -no-comment flag makes things work without needing to undo the copy_files stuff.

I did have to use -ignore-missing-files because bisect-ppx-report was still having problems with missing files, but I could see that just being required. However with that flag I was able to generate a useful coverage report, which I am super stoked about!

[aantron]

Great :) I'm going to work on a couple more issues slowly over the coming week or so, then release this in 1.4.1. Thanks @bandersongit, @ulrikstrid, and @diml!

[aantron]

This is now released and installable from opam in Bisect_ppx 1.4.1.