Fix and optimize quoter

[sim642]

Thinking about #317 again, I realized that quoting had already been ported to ppxlib, but I was completely unaware of it before (it was very easy to miss in the middle of all the autogenerated documentation).

This PR ports my optimization to the quoter from ppx_deriving to ppxlib: ocaml-ppx/ppx_deriving#252.

Before doing that, I realized that the existing implementation was actually broken: it was missing application of () to the quoted name, so I fixed that first.
For identifier-only quoted expressions the two things somewhat cancel each other out, so the changes might be easier understood looking at the commits separately.

I guess nobody noticed it being broken for all these years because nobody is using Ppxlib.Quoter (also according to Sherlocode).

[sim642]

I'm not sure what to make of this failure diff with OCaml 4.14:

-            {Ppxlib__.Import.pos_fname =
-              "_no"... (* string length 6; truncated *);
-             pos_lnum = 1; pos_bol = 0; pos_cnum = -1};
+            {Ppxlib__.Import.pos_fname = "_none_"; pos_lnum = 1; pos_bol = 0;
+             pos_cnum = -1};

Structurally it's the same thing, but the testing is fragile w.r.t. unrelated changes somewhere (EDIT: I guess this: ocaml/ocaml#10565).

[pitag-ha]

Thanks for the PR, @sim642! I'll have time to review it Thursday or Friday.

[pitag-ha]

Thanks @sim642, both for the bug fix and for the optimization! Apart from the failing CI and a missing changelog entry, this looks perfect. Would you mind rebasing over main to see if that fixes the CI, and adding a changelog entry?

Also, I've read your conversation with gasche on the ppx_deriving PR, which is very interesting as well.

Btw, we should really add documentation to the quoter functions!

[sim642]

Would you mind rebasing over main to see if that fixes the CI, and adding a changelog entry?

Both done, but the CI still breaks for OCaml 4.14. I think the issue is that it's using toploop's own printing from the compiler instead of ppxlib's copy of Pprintast.

[pitag-ha]

Hey @sim642! This has taken a while, but the CI should be fixed now. Your branch needs to be rebased over main one more time, and the test needs to be promoted. Would you mind doing that?

[sim642]

Done and the CI looks happy now.

Weirdly, though, running locally in a OCaml 4.14 switch dune runtest gives a handful of other diffs like:

diff --git a/_build/default/test/base/test.ml b/_build/default/test/base/test.ml.corrected
index 7cf282a..77c27e5 100644
--- a/_build/default/test/base/test.ml
+++ b/_build/default/test/base/test.ml.corrected
@@ -106,17 +106,17 @@ let _ = convert_longident "Base.( land )"
 
 let _ = convert_longident "A(B)"
 [%%expect{|
-Exception: (Invalid_argument "Ppxlib.Longident.parse: \"A(B)\"")
+Exception: Invalid_argument "Ppxlib.Longident.parse: \"A(B)\"".
 |}]
 
 let _ = convert_longident "A.B(C)"
 [%%expect{|
-Exception: (Invalid_argument "Ppxlib.Longident.parse: \"A.B(C)\"")
+Exception: Invalid_argument "Ppxlib.Longident.parse: \"A.B(C)\"".
 |}]
 
 let _ = convert_longident ")"
 [%%expect{|
-Exception: (Invalid_argument "Ppxlib.Longident.parse: \")\"")
+Exception: Invalid_argument "Ppxlib.Longident.parse: \")\"".
 |}]
 
 let _ = Ppxlib.Code_path.(file_path @@ top_level ~file_path:"dir/main.ml")

Doesn't seem to have bothered the CI though, so I'm not sure what that's about.

EDIT: Looks like the difference between base v0.14.3 (no diffs) vs v0.15.0 (these extra diffs). I guess something additional to separately look into.

[pitag-ha]

Perfect, thanks for this improvement, @sim642!

[pitag-ha]

Weirdly, though, running locally in a OCaml 4.14 switch dune runtest gives a handful of other diffs like:

You seem to have version v.0.15.0 of sexplib0 on your 4.14 switch. The problem is that sexplib0 has re-formatted one of its exception printers in v.0.15.0. That's why the ppxlib opam file currently requires sexplib0 to be of lower version than v0.15.0 for with-test (which is why the CI installs a lower version and everything is fine on there).