Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interaction with other ppx rewriters in 1.3.1 #164

Closed
emillon opened this issue Dec 19, 2017 · 5 comments
Closed

Interaction with other ppx rewriters in 1.3.1 #164

emillon opened this issue Dec 19, 2017 · 5 comments
Labels

Comments

@emillon
Copy link

emillon commented Dec 19, 2017

Hello,

I hit a regression with bisect_ppx. 1.2.0 works, but when using 1.3.1 we now have a build error.

I reduced the following test case:

# _tags
true: package(ppx_deriving.std)
true: package(ppx_bin_prot)
true: package(bisect_ppx)
(* test.ml *)
module type EQ = sig
  type t [@@deriving eq]
end
(* test.mli *)
module type EQ = sig
  type t [@@deriving eq]
end

(build with ocamlbuild -clean && ocamlbuild -use-ocamlfind test.byte)

Using bisect_ppx.1.3.0, ppx_deriving.4.2.1 and ppx_bin_prot.v0.9.0, this fails with the following error:

File "_none_", line 1:
Error: eval expected
Command exited with code 2.

Some remarks:

  • removing the package(bisect_ppx) line fixes it
  • removing the package(ppx_bin_prot) changes the error to Unrecognized [@@deriving] annotation syntax (note that this module does not use it it)
  • removing the interface file fixes it
  • this works with 1.2.0 and not with 1.3.1. I bisected it down to 8fe184c.

It seems that it's a new bug in the exclusion mechanism, maybe triggered by strange locations emitted by these ppx rewriters.

Let me know if I can be of further help.

Thanks!

@aantron
Copy link
Owner

aantron commented Dec 19, 2017

Thanks for the report. I saw what is probably another symptom of this problem. It looked like in some cases, Bisect_ppx would write its per-module instrumentation initialization code into an attribute of the module, rather than the module itself. This is probably what is causing the eval expected message above, as when Bisect does that, it prepends other structure items before the expression-evaluation structure item that was originally in the attribute.

Other symptoms were losing documentation, when the attribute was an ocamldoc string, and not generating any instrumentation output, as the code written into the attribute doesn't ever run.

I couldn't reproduce it after twiddling my opam switch, though, so I didn't fix it. Hopefully, I'll be able to track it down firmly now. I'll take a look in some hours or maybe days.

@aantron
Copy link
Owner

aantron commented Dec 20, 2017

I just realized what the bug almost certainly is. Bisect_ppx chooses which structure to insert its initialization code into by inserting it into the first structure that it encounters while traversing the AST.

In an .ml file, that first structure is always the one contained in the module representing the whole file.

In an .mli file, the only way to inject a structure I can immediately think of, is by writing an attribute. So, if there is an attribute, it will contain the first structure, and Bisect_ppx will insert code into it. Bisect_ppx should instead do nothing in .mli files.

So this is probably what's happening. I'll add a test case, confirm, and fix.

You should be able to work around the issue in the meantime by not applying Bisect_ppx to .mli files, so don't use true: package(bisect_ppx), but <**/*.ml>: package(bisect_ppx), or whatever applies to your project.

This is tricky to remember, and also not easy to control with Jbuilder, so we definitely have to fix it in Bisect as well.

@aantron aantron added the bug label Dec 20, 2017
@aantron
Copy link
Owner

aantron commented Dec 20, 2017

The linked commit adds a test reproducing it, and fixes it.

I'm going to fix a couple more issues, and release Bisect_ppx 1.3.2.

@emillon
Copy link
Author

emillon commented Dec 21, 2017

Great, that worked!

We're using a custom ocamlbuild plugin and here's what our fix looks like, in case anyone else is interested:

       let bisect = S [ A "-package" ; A "bisect_ppx" ] in
-      flag ["ocaml"; "compile"; "with_coverage"] & bisect;
+      flag ["ocaml"; "implem"; "compile"; "with_coverage"] & bisect;
       flag ["ocaml"; "link"; "with_coverage"] & bisect

aantron added a commit that referenced this issue Dec 21, 2017
To further ensure that Bisect_ppx does not instrument structures nor
expressions in .mli files, the .mli test is modified to include a
sequence expression, which is something that Bisect_ppx definitely does
normally instrument (in .ml files).

The test checks that it is not instrumented.

Part of #164.
@aantron
Copy link
Owner

aantron commented Dec 22, 2017

Ok, this is now fixed in opam in Bisect_ppx 1.3.2 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants