ppxlib icon indicating copy to clipboard operation
ppxlib copied to clipboard

Remove `Context_free.Rule.attr*`

Open ghost opened this issue 8 years ago • 7 comments

These are only used for [@@deriving ...] and it's unlikely they'll ever be used for something else. Now that [@@deriving] is part of ppxlib, I think removing them and specializing the implementation to only support [@@deriving ...] would make the code simpler.

ghost avatar Apr 12 '18 13:04 ghost

I'd like to give this a try in order to make my life easier with #61.

I'm trying to understand what the scope of this actually is.

My understanding so far is that we want to get rid of the attr_* smart constructor and replace them with something more in the line of Context_free.Rule.extension that would only allow you to declare a rule for the deriving attribute. Removing these smart constructors is gonna break the API but I assume we don't consider this as a problem since they're probably only used in Deriving.

These smart constructors appear to be called a single time, on all the registered generators at once. I'm not sure exactly what the new API should look like.

I'm also guessing this is the occasion to leak a bit less about the internal types in Context_free.Rule.

NathanReb avatar Feb 26 '19 13:02 NathanReb

My understanding so far is that we want to get rid of the attr_* smart constructor and replace them with something more in the line of Context_free.Rule.extension that would only allow you to declare a rule for the deriving attribute.

I was thinking to completely remove them and make Context_free depend on Deriving. i.e. context_free.ml would directly call functions from Deriving.

Removing these smart constructors is gonna break the API but I assume we don't consider this as a problem since they're probably only used in Deriving.

Agreed.

BTW, I remember that I tried this last year. My attempt is in the branch simplify-context-free. I don't remember exactly what's the status of this branch is, but it might be worth having a look. I couldn't easily get rid of the circular dependency between Context_free, Deriving and Driver, which was annoying and required forward references, but I don't know if we can do better though.

ghost avatar Feb 26 '19 13:02 ghost

Hmm-- I'm working on a project that requires [@@deriving]-like features, except on module declarations. As far as I can tell, [@@deriving] only currently supports types. If there is no plan to develop the attr-libraries and use only [@@deriving], what shall I do?

kwshi avatar Jul 30 '19 20:07 kwshi

I know ppx_deriving allows this. It seems fine to add the same support in ppxlib.

ghost avatar Aug 01 '19 10:08 ghost

Does it, really? The API docs seem to indicate only support for type_ext and type_decl transformers (no mention of modules whatsoever), and every single one of the examples in the README only shows @@deriving being used on types, not modules.

kwshi avatar Aug 06 '19 18:08 kwshi

This message helped me realize that the API docs for ppx_deriving were way out of date, I just updated them. The new docs, and it does mention the hooks to operate on module types (not modules), which were implemented in https://github.com/ocaml-ppx/ppx_deriving/pull/94.

gasche avatar Aug 06 '19 20:08 gasche

Thanks for updating the docs! My project, however, requires operating on a module expression, which so it seems is not provided by deriving.

kwshi avatar Aug 07 '19 14:08 kwshi