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

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

Open
sim642 opened this issue Apr 30, 2022 · 8 comments
Open

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

sim642 opened this issue Apr 30, 2022 · 8 comments

Comments

@sim642
Copy link
Contributor

sim642 commented Apr 30, 2022

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Contributor Author

sim642 commented May 31, 2022

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
Copy link
Contributor Author

sim642 commented Jun 1, 2022

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
Copy link
Contributor

mbarbin commented Oct 18, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

mbarbin commented Oct 21, 2024

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
Copy link
Collaborator

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

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

No branches or pull requests

5 participants