Getting tool_name from rewriter and/or derivers

[NathanReb]

Following ocaml/dune#1861 I'm trying to add a specific behaviour when my ppxlib based deriver is used in an ocamldep context. ocaml-migrate-parsetree seems to make it available through a config argument but I can't seem to find how I could access it from ppxlib.

I worked out an ugly solution to come around that limitation for now but if I'm indeed not missing something, it might be useful to find a way to pass it on to the actual ast -> ast function.

[NathanReb]

It turns out my ugly workaround doesn't even work so my current solution is to always silently ignore abstract types. What I'd really want is to only ignore them in an ocamldep context and produce an error otherwise. The current behaviour obviously isn't satisfactory as it would silently fail without giving any feedback to the user.

[ghost]

Indeed, we don't forward this information in ppxlib. Adding the forwarding of this information might be invasive, but given that this is a global property of the running instance, maybe we can get away with a global setting. For instance, we could add a function like this to driver.mli:

val tool_name : unit -> string

[NathanReb]

I'm usually not too fond of such globals but if you feel like that's the way to go that definitely works for me!
I guess it's the least invasive non API-breaking way of exposing tool_name here.
In the long run I assume it could be part of some abstract context value passed instead of loc and path to ast -> ast functions, as you already suggested in #60. Such a config could then easily be enriched with new fields without breaking the API.

Actually what would you think about adding such a context type and new versions of Extension.declare and Deriving.Generator.make now?

Let me know which one you'd prefer and I'll have a go at it!

[ghost]

Passing a context around does look better indeed. If you are happy to have a go then I'm all for it!

[NathanReb]

Ok great! I'm not too familiar with ppxlib's internals yet so if you have any advice/suggestion I'd gladly take it!

[ghost]

You can start by passing the context to the various functions in Transform.t in src/driver.ml. You don't need to touch the rules field since it is turned into the generic impl or intf field before the transformations are applied. Then by following type errors you should find the places that need to be updated. Feel free to ask if you need more details about some part of the code.

[NathanReb]

I had a quick look earlier and was under the impression that any transformation you register through Deriving or Extension will eventually come down to a Context_free.Rule.t. Woudn't passing the context (which might not be the best suited name btw) to impl and intf make all of its fields besides loc and path inaccessible to transformations written this way?

My first approach was then to try and modify Context_free.Rule.t's expand field so that it would take my Context.t in place of the loc and path. While this seems doable for now as my Context.t is only made of a loc and path, I'm not sure how easy it would be to add new stuff such as tool_name there.
This approach also seemed to be quite heavy as doing it in an API compatible way would require to define new attr_str_* and attr_sig_* smart constructors that would take a context: Context.t -> ... expand function.

While your suggestion seems indeed easier to implement, I'm worried that unless I'm missing something, it would require one to register a raw transformation to benefit from those changes.

[ghost]

Yes, you are right, we do need to update Context_free.Rule.t's expand field. BTW, I have been meaning to remove the Context_free.Rule.attr* functions and only handle [@@deriving ...] instead (#14). We only needed the genericity when ppx_type_conv was a separate project, but now it would simplify the code if we only supported [@@deriving]. It might help to start with that first.

[NathanReb]

I started working on this again as the changes in #67 make it much easier!