Document current limitation of deriving_inline

[mbarbin]

In this draft PR, I discuss a kind of usage for deriving_inline that I would have a use for in this project.

Currently it doesn't work. This PR is meant to help support discussions about this with the ppx devs.

DO NOT MERGE.

[mbarbin]

@NathanReb I added an opam pin to ocaml-ppx/ppxlib#541 and refreshed this PR.

I added the flag -no-corrections.

The build now works! Also, the errors about the corrected code doesn't round-trip are gone. Nicely done!

However the lint does not work due to what appears to be small tweaks to ocamformating. Is it possible that the version of ocamlformat picked up and used during the deriving_inline not be correctly inferred?

You can try building the tip of this PR, specifically run dune build @lint. This results for me in:

File "lib/vcs/src/graph.ml", line 1, characters 0-0:
diff --git a/_build/default/lib/vcs/src/graph.ml b/_build/default/lib/vcs/src/graph.ml.lint-corrected
index 9cfdd5f..879889d 100644
--- a/_build/default/lib/vcs/src/graph.ml
+++ b/_build/default/lib/vcs/src/graph.ml.lint-corrected
@@ -83,29 +83,28 @@ module Node_kind = struct
             }
       [@@deriving_inline equal]
 
-      let equal =
-        (fun a__001_ ->
-           fun b__002_ ->
-           if Stdlib.( == ) a__001_ b__002_
-           then true
-           else (
-             match a__001_, b__002_ with
-             | Root _a__003_, Root _b__004_ -> Rev.equal _a__003_.rev _b__004_.rev
-             | Root _, _ -> false
-             | _, Root _ -> false
-             | Commit _a__005_, Commit _b__006_ ->
-               Stdlib.( && )
-                 (Rev.equal _a__005_.rev _b__006_.rev)
-                 (Node.equal _a__005_.parent _b__006_.parent)
-             | Commit _, _ -> false
-             | _, Commit _ -> false
-             | Merge _a__007_, Merge _b__008_ ->
-               Stdlib.( && )
-                 (Rev.equal _a__007_.rev _b__008_.rev)
-                 (Stdlib.( && )
-                    (Node.equal _a__007_.parent1 _b__008_.parent1)
-                    (Node.equal _a__007_.parent2 _b__008_.parent2)))
-         : t -> t -> bool)
+      
+let equal =
+  (fun a__001_ ->
+     fun b__002_ ->
+       if Stdlib.(==) a__001_ b__002_
+       then true
+       else
+         (match (a__001_, b__002_) with
+          | (Root _a__003_, Root _b__004_) ->
+              Rev.equal _a__003_.rev _b__004_.rev
+          | (Root _, _) -> false
+          | (_, Root _) -> false
+          | (Commit _a__005_, Commit _b__006_) ->
+              Stdlib.(&&) (Rev.equal _a__005_.rev _b__006_.rev)
+                (Node.equal _a__005_.parent _b__006_.parent)
+          | (Commit _, _) -> false
+          | (_, Commit _) -> false
+          | (Merge _a__007_, Merge _b__008_) ->
+              Stdlib.(&&) (Rev.equal _a__007_.rev _b__008_.rev)
+                (Stdlib.(&&) (Node.equal _a__007_.parent1 _b__008_.parent1)
+                   (Node.equal _a__007_.parent2 _b__008_.parent2))) :
+  t -> t -> bool)
       ;;
 
       [@@@deriving.end]

See for example the extra parens around the tuple, such as match (a__001_, b__002_) with VS match a__001_, b__002_ with, etc.

[NathanReb]

What happens if you:

1. promote the result of the linter
2. run dune build @fmt --auto-promote
3. run dune build @lint again

[mbarbin]

What happens if you:

1. promote the result of the linter

This saves the version with the paren on disk, now git status is showing a modified file.

2. run `dune build @fmt --auto-promote`

This removes the paren and revert the file back to its committed version. git status is now showing a clean state.

3. run `dune build @lint` again

This shows the same diff again.

[NathanReb]

This is annoying indeed. The driver does not try to format anything here so it's kind of expected. We'd either need dune to format corrected files before diffing which I think is something that came up a few times already so it would definitely be a useful addition.

[NathanReb]

We could change the driver so it detects that the ASTs are identical (modulo the locations) and use the source in such cases but it feels way overkill compared to properly formatting the corrected file.

[mbarbin]

Ah, I'm surprised because I thought the output of deriving_inline was already auto-formatted. I wonder how things work in other projects (e.g. base?).

This reminds me of a similar issue with the cinaps project. They ended up putting an option in the tool itself to realize the dune integration, see here. Could be an approach perhaps for ppx drivers as well?

The driver does not try to format anything here so it's kind of expected. We'd either need dune to format corrected files before diffing

I would perhaps even argue that formatting as part of the generation would be better, so you end up directly with the output you'd expect. But that's minor, it only saves you 1 dune fmt. But maybe simpler not to have to worry about changing the diffing part of things.

[NathanReb]

I think you misunderstood my proposal, the way I imagined things would be something like:

1. Dune runs the driver to get a .corrected file
2. Dune formats the .corrected file using the same rule as for the original source file
3. Dune runs the diffing/promotion on the formatted, .corrected file

In this scenario, the user would simply run dune build @lint --auto-promote and get the properly formatted version directly. Running dune build @fmt should succeed after that.

Dune's the one responsible for knowing how to format the different files accross the project so no matter how we put it, we can't simply rely on the ppx driver to know how to do it on its own.

Adding an option to the driver sounds good if we have a good scenario for dune (or other build systems) to use that option.

[mbarbin]

You are right, I do not have all the involved pieces clearly in my head yet. I appreciate the discussion, thanks!

When encountering a deriving_inline, does ppxlib always outputs a corrected file? Or is there some logic in ppxlib to decide whether a correction is needed? I'm somewhat unclear of whether things can properly work when ppxlib doesn't get involved at all in the formatting.

[NathanReb]

I'm still in the process of familiarizing myself with the deriving_inline workflow too!

Yes I think it does always generate a corrected file if it processes at least one such rule.

With the proper (and I believe relatively simple) improvement in dune, we can make this work with any code generation scheme using dune's promotion system, the driver does not even have to know about this and can simply spit out source code without having to care about formatting at all. I mentioned you on a fairly old dune issue that's exactly about this specific problem which arose a few times already. Now might be the time to finally fix this!

[mbarbin]

I read the old dune issue, and I think I have now a better understanding. Your proposal makes sense to me! Thanks for taking the extra time to detail, much appreciated!