"Corrected code doesn't round-trip" with deriving_inline

[sim642]

I have defined a deriver named "leq" with ppxlib, but I guess any ppxlib-based deriver would have the same issue.
First, I use it in a test file as follows:

type t = L1.t [@@deriving_inline leq][@@@end]

Second, running dune's build and promote changes it to the following:

type t = L1.t [@@deriving_inline leq]
let _ = fun (_ : t) -> ()
let rec (leq : t -> t -> bool) =
  let __0 () = L1.leq in
  ((let open! ((Ppx_deriving_runtime)[@ocaml.warning "-A"]) in __0 ())
    [@ocaml.warning "-A"])[@@ocaml.warning "-39"]
let _ = leq
[@@@end]

This promoted file now refuses to build by giving the following error:

Error: ppxlib: the corrected code doesn't round-trip.
This is probably a bug in the OCaml printer:
<no differences produced by diff>
diff: /tmp/build_f87c44_dune/ppxlibb4e945: No such file or directory
diff: /tmp/build_f87c44_dune/ppxlib38b9eb: No such file or directory

Third, I discovered that removing the let _ definitions and manually changed the file to the following:

type t = L1.t [@@deriving_inline leq]
let rec (leq : t -> t -> bool) =
  let __0 () = L1.leq in
  ((let open! ((Ppx_deriving_runtime)[@ocaml.warning "-A"]) in __0 ())
    [@ocaml.warning "-A"])[@@ocaml.warning "-39"]
[@@@end]

Then the round-tripping error disappears, but dune build again suggests a promotion to add the two definitions back, leading back to the second code snippet above.

Therefore I cannot get deriving_inline to a stable state, where it builds without round-trip errors and doesn't keep suggesting additional promotions.

[panglesd]

Thanks for the report!

Time allocated to ppxlib has become a scarce ressource, however I'll do my best to investigate this issue. Sorry in advance for the dealy!

[pitag-ha]

Hey, thanks for reporting and sorry for the delay! I've just tried to reproduce the problem and am not able to reproduce it. Are you using a mix between ppx_deriving and ppxlib to define your leq deriver? I'm wondering because of the open of the ppx_deriving runtime (which might be related to the round-trip problem, but I'm not sure).

[sim642]

Indeed, I am using ppx_deriving, but only for some utilities (#317). I'll have to see, if I can minimize what I have.

[sim642]

Removing the usage of ppx_deriving's quoter, the original error disappears, but it still doesn't seem to work right.

Instead of the above, second dune build and promote changes it to:

type t = L1.t [@@deriving_inline leq]
let _ = fun (_ : t) -> ()
let rec (leq : t -> t -> bool) = L1.leq[@@ocaml.warning "-39"]
let _ = leq
[@@@end]

Building that fails with the following diff:

----- ppx_lattice_test.ml
++++++ ppx_lattice_test.ml.ppx-corrected
File "ppx_lattice_test.ml", line 21, characters 0-1:
 |type t = L1.t [@@deriving_inline leq]
-|let _ = fun (_ : t) -> ()
 |let rec (leq : t -> t -> bool) = L1.leq[@@ocaml.warning "-39"]
-|let _ = leq
 |[@@@end]
 |end

That again wants to get rid of those let _-s, but actually manages to print the diff instead of crashing. But whatever that diff is, dune promote doesn't promote anything.

Manually removing to

type t = L1.t [@@deriving_inline leq]
let rec (leq : t -> t -> bool) = L1.leq[@@ocaml.warning "-39"]
[@@@end]

finally builds successfully and doesn't print any diff.

Therefore it seems to me that there's still some inconsistency, because initially @@deriving_inline suggests those and then suggests removing them.

The round-tripping error might be orthogonal then and due to some of that ppx_deriving quoter wrapping expression.

[mbarbin]

I have been experimenting with @@deriving_inline today, with other ppx than mentioned here, and been hitting this "doesn't round-trip" issue too. Except in my case, there's no flickering in the generation. After I fmt the code, the error goes away sometimes.

However I haven't managed to reproduce the issue clearly. This is probably hard to test if this is specific to dune running in watch mode. If there's interest I could try to spending more time on this. For now I just wanted to mention that there are still some issues in this area with recent versions of ppxlib (this is with 0.33).

[NathanReb]

I'll have to get a bit more familiar with the original error but if you manage to get a reproducible example I'll happily look into it!

[mbarbin]

Thanks a lot @NathanReb for offering to help!

I added a new commit to the Vcs PR where I discuss some related deriving_inline topics. I wrote down the detailed steps of what I do and the errors that I get. It's this commit: a02920b5a13c487e932e104256ffecbe2fe2ebfa Permalink

If you try going through the same steps and let me know what this does for you, with a bit of luck, you might get similar errors!

[NathanReb]

Sorry for the long wait, I was on vacation. I'll look into this shortly!

[NathanReb]

I managed to reproduce it locally, I'll try to see what's going on!

[NathanReb]

I tried it with the latest ppxlib and I'm getting different output from @all and @lint builds which can't be reconciled but no more code does not roundtrip anymore.

I'll look into it.

[NathanReb]

This is due to the quoter returning different IDs in one case and the other. The lint build only generates the deriving_inline code and therefore gets different IDs than the regular builds that also run the rest of the PPX-es.

I need to refresh my memory on how deriving_inline is supposed to work but it's a little odd that it's ran buy the regular builds AND the lint. I would expect it to only be ran by the lint builds so that it's not a dependency of the regular builds anymore.

[NathanReb]

Circling back to the quoter issue, I think it can easily be solved by adding the entire set of ppx-es to the (lint) field as well as then the IDs allocated by the quoter would match. I'll have to give this a try just to ensure that there are no more "no round-trip" errors.

That being said, I agree this won't solve your main issue which is that you can't properly use [@@deriving_inline ...] to remove a build dependency on a subset of your ppx-es. For that I'm currently working on adding a flag!

[mbarbin]

Circling back to the quoter issue, I think it can easily be solved by adding the entire set of ppx-es to the (lint) field as well as then the IDs allocated by the quoter would match.

You are talking about something inside the ppxlib implementation, or a change for the user to do in the dune config files?

you can't properly use [@@deriving_inline ...] to remove a build dependency on a subset of your ppx-es. For that I'm currently working on adding a flag!

I'm quite happy with a flag, and can justify allocating time for the testing of it if this helps down the road. Thanks a lot!

[NathanReb]

You are talking about something inside the ppxlib implementation, or a change for the user to do in the dune config files?

Sorry this wasn't very clear indeed! On the branch you sent me for reproduction, the set of ppx-es in the preprocess and lint fields differ, meaning that dune build @lint and dune build will disagree on what to promote because of the quoter allocating different IDs for the inlined code.

I mostly wrote that for reference as I don't expect a lot of users would be interested in inlining code both via the lint step and regular builds and in your case, I believe the flag to disable deriving_inline so you don't have to depend on the relevant ppx at build time is what you are looking for!

[mbarbin]

On the branch you sent me for reproduction, the set of ppx-es in the preprocess and lint fields differ,

I looked briefly again at the example, and I think it is showing a transitory state. I don't exactly remember but I think I was trying to transition some ppx from the build to the lint stage, and may have paused in a state where the same ppx is mentioned in both places. Ideally, I would not want to do that. Instead, I'd have a clear separation of which ppx are used during the build, and which are used as linting dependency only, with zero overlap. Indeed, I didn't think about a use case by which one would inline code via the regular build (I don't have strong opinion, just can't think of a situation where I would need this at the moment).

[NathanReb]

I guess it can serve as a quick and dirty way to inspect the code generated by a deriver but outside of that I can't think of a valid usecase.

I'm wondering whether the lint phase should have the whole set of ppx-es (inline + build ones) or just the set of ppx-es used for deriving_inline and similar correction based transform.

The driver has no particular idea that it's running in lint mode. Dune does invoke it with -null which discards the output and will therefore only generate the corrected file (the one that's diffed with the source to promote inline code) so it just happens to work but the driver will attempt to apply all transformations as it normally would. I'm wondering if there could be some situations where not having one of the required ppx-es linked could trigger some unwanted errors during the lint phase. It's likely that it won't in the majority of cases since the errors are usually embedded in the returned AST but that's still something to keep in mind in case it comes back to bite us.