Add Extension.V3.declare_with_path_arg

[ELLIOTTCABLE]

Seems like an oversight in the new API; came up in this Discuss thread.

make builds; make test passes. However, make test passes no matter what I do. Can't verify that my added test actually passes — but it's a trivial addition, it should be "if it compiles, it works." 🙈

(Could use a CONTRIBUTING section in the README with instructions as to how to build/test/PR, etc. hahaha.)

[panglesd]

Thanks for the PR!

However, make test passes no matter what I do

This is I think because the test you modify is only enabled in OCaml between 4.08 and 4.11 (included). So if the OCaml version in your switch is outside these bounds, the test is not run.

However, the CI ran the test. You need to register the new transformation to another extension node.

[pitag-ha]

Thanks for contributing, it's always nice to see new people on the repo!

What's the use case you have in mind? In V3, the module path the extension point lives in can be accessed by the rewriter via the expansion context, see e.g. the tests about code_path: https://github.com/ocaml-ppx/ppxlib/blob/main/test/code_path/test.ml . Have you seen that already?

[panglesd]

I was also puzzled by the fact that "with_path_arg" is not part of code_path, but it is actually something different, about only the name of the extension point: for instance [%ext_name.Additional.Path] has path_arg Additional.Path.

[pitag-ha]

about only the name of the extension point: for instance [%ext_name.Additional.Path] has path_arg Additional.Path.

Yes, but what's the use case for that? I'm assuming that the usual/intended use case for it in "V1" was to be able to pass in the module path manually given that the rewriter didn't have access to what we now call expansion context. I'm wondering what the use case of manually passing in a path argument is now that we do have the accurate code_path via the expansion context.

[ELLIOTTCABLE]

Er, the "path arg" is, imho, badly named, because code-path now exists — I think out of two possible meanings of the word 'path', the current Code_path module is a much more obvious meaning.

By contrast, the "path_arg" was a explicit module path argument, in the sense of "argument" meaning "provided by the user" — completely unrelated to Code_path, which cannot (and should not be) affected by the user.

e.g. in this example, Code_path basically encodes Abc.Xyz, whereas path_arg is explicitly Foo.Bar:

module Abc = struct
   module Xyz = struct
      let%ext.Foo.Bar = (* ... *)
   end
end

fwiw, I think now would be a fantastic time to rename the path_arg, a change I can make in this PR if y'all agree — V3's API, thanks to the omission of this feature thus far, can use a different naming convention that doesn't clash so confusingly with the new Code_path.

wdyt?

---

As for use-cases, who knows what all someone might overload that explicit "argument" to mean; but my usage (in ppx_trace) is effectively identical to that of ppx_let; e.g. their docs:

Alternatively, the extension can use values from a Let_syntax module other than the one in scope. If you write %map.A.B.C instead of %map, the expansion will use A.B.C.Let_syntax.Let_syntax.map instead of Let_syntax.map (and similarly for all extension points).

(This is a fantastic feature, and the primary reason we're still using ppx_let at @ahrefs over more "modern" syntactic monadic binds in recent OCaml versions — if you genuinely need to operate with a few monads interleaved simultaneously in scope, something like let%bind.Lwt and let%bind.Error is much, much clearer than trying to memorize the difference between let*, let+, and let% or whatnot.)

[pitag-ha]

Thanks for explaining the use case, sounds all good!

fwiw, I think now would be a fantastic time to rename the path_arg, a change I can make in this PR if y'all agree

Sounds good as well. What name would you use?

Btw, about the CI failures:

• Could you add a changelog entry (i.e. an entry to CHANGES.md)? We forgot to add an unreleased section after the 0.30.0 release. So I think now the easiest would be if you could add that unreleased section on top of Changes.md and add your entry to that section. Does that sound good?
• We use DCO, so you need to sign all your commits. You can do that at the very end if that's more convenient.
• The ocaml-ci failure is unrelated to your PR.

[ELLIOTTCABLE]

Okay, I chose, simply, "additional_arg" — although, yes, only a module-path will technically be accepted in this position, that's actually somewhat orthogonal?

Let me know if that naming pleases you. I also added notes to the now-deprecated interfaces so people can find the new one.

I forgot to ask — is there an appropriate test file that does get run on current versions? Somewhere better I can add tests for this?

[panglesd]

Let me know if that naming pleases you. I also added notes to the now-deprecated interfaces so people can find the new one.

To be honest, I think it has more disadvantages than advantages:

• It makes us have two names for the same concept
• additional_args is much less specific than path_arg, and the argument can only be a path (and additional_path_arg is too long!)

So, I would be in favour of not changing the name.

I forgot to ask — is there an appropriate test file that does get run on current versions? Somewhere better I can add tests for this?

I think the test is not run because of a change in the display of error (ocaml/ocaml#9657). But I don't think that's a good reason to not test on OCaml version > 4.12.

So we should make this test run on all versions. One possibility is to manipulate the string returned by the expect test, but that's ad-hoc for each compiler change of displayed errors, and requires maintainance time that we don't have.
The other approach is to just have different files for the different versions. You can find an example of such approach in for instance https://github.com/ocaml-ppx/ppxlib/tree/main/test/deriving
I could do the test bump myself if you prefer.

[ELLIOTTCABLE]

Okay, I think your arguments hold water — wasn't too sure myself.

Pushed a reversion of the renaming; and added 4.12+ tests, mirroring the approach elsewhere in the repo. :D

Thanks for the hand-holding. ❤️

[panglesd]

I finally had a look at the code, looks good to me! (unsurprisingly)

There are still some things to do to satisfy the CI, before merging... I can do them if you are tired of this!

• the test/driver/transformations/test_412.ml file should be added to the .ocamlformat-ignore file (or a glob for all .ml files in this directory)
• The 5.1 CI is still not happy with the test. We might need to add a test_501.ml file... (this method of having different tests for every breaking change in OCaml output is pretty annoying, but less than the other solutions in my opinion).

[ELLIOTTCABLE]

Okay! Take 5! 🤞

[panglesd]

Thanks @ELLIOTTCABLE ! (and sorry for the delay...)

[ELLIOTTCABLE]

You're totally fine, @panglesd ❤️

No pressure at all, just for my planning purposes — what's the general release-cadence likely to be for this to go up onto the public opam repo? We're already using this in production at Ahrefs (it's no big deal, we already have ... a lot of opam overrides 😅); but it's also blocking me being able to release ppx_trace to the public, and some other related ecosystem work around ocaml-trace.

[panglesd]

Until now, mostly @pitag-ha has been taking care of the releases... She is currently on holiday.

I think that with the release of ocaml 5.1, we will need to release a compatible version of ppxlib soon! So hopefully, you'll be able to release ppx_trace soon 😅.

[pitag-ha]

Yep, let's cut a release (as soon as we've merged the functor application migration fix).