Deprecate deriving abstract and add deriving dynamicKeys

[jchavarri]

Previously based on simplify-derive-abstract (#978).

As discussed on Discord, this PR deprecates deriving abstract because the PPX doesn't create an abstract type anymore, so the current name is misleading.

To provide an alternative, it adds deriving dynamicKeys. The name is a suggestion, if there are ideas for better naming please share.

The first commit deprecates abstract, the second introduces dynamicKeys.

[anmonteiro]

this location will be a lot better if you add a proper location on the attribute.

[jchavarri]

I am not sure I understand your suggestion. The deriver has no access to the original ast (which is where the attribute is), it can only insert the deprecation alert on the newly created ast nodes. I believe it's the same root cause that prevented making the type abstract originally when the deriver was introduced.

More generically, it feels that implementing deriving abstract with a pure deriver was a too limiting choice for the use case.

[anmonteiro]

I don't know what I was thinking, tbh. You're right, of course.

[anmonteiro]

I think it makes sense to deprecate abstract, indeed, since we don't make the type abstract anymore.

I'm not a big fan of dynamicKeys, though. Here are some other alternatives:

• [@@deriving constructor_fields]
• [@@deriving js_fields]
• [@@deriving js]
• [@@deriving getters_setters]
• [@@deriving maker]

Will keep thinking about some more

[jchavarri]

ReScript called this functionality "optional fields". What about optionalKeys? I think it's important to keep the naming close to the JS side (in this case "keys", because that's the part that differs from plain records). "Fields" makes me think about the record, but this deriver is not about records really.

I insist on "optional keys" or "dynamic keys" because the only real use case that remains for this deriver is the ability to create JS objects where the keys can be defined or not based on the labeled args passed to the construction function (analog to ReScript optional fields). I think making that only use case relevant through the naming will help users identify if they have to use it or not. While names like js or getters_setters don't tell anything about this use case.

Regarding casing, i chose camelcase because that's what jsConverter and newType were using. I think we should be consistent, one way or the other.

[andreypopp]

I've commented on this on Discord already so just repeating here. I think *Keys doesn't tell what this deriver is doing.

It's generating a constructor + various setters/getters for the record, the closest thing is JaneStreet's https://github.com/janestreet/ppx_fields_conv.

I think something like @@deriving record_constructor would be more clear.

[anmonteiro]

I insist on "optional keys" or "dynamic keys" because the only real use case that remains for this deriver is the ability to create JS objects where the keys can be defined or not based on the labeled args passed to the construction function (analog to ReScript optional fields).

Don't you think that the naming for this functionality should be independent of whether there are other mechanisms to achieve some overlapping use cases?

FWIW, here's what [@@deriving abstract] currently does:

• generates a constructor for the record type it is derived from
• generates getters and setters (if fields are mutable) for its fields
• allows omitting entire keys from the resulting JS object if fields are marked with [@mel.optional]

Your naming suggestion only accounts for the cases where [@mel.optional] is present. IMO it fails to convey all the other generated getters / setters and constructor functions.

[jchavarri]

The getters and setters are orthogonal to the feature we're discussing (the constructor), they were added originally because the type was abstract, but they could be implemented as a separate deriver. These getters and setters might be getting in the way throughout this conversation, so let's assume they're not part of the deriver for the rest of this comment.

The most useful functionality of the deriver (imo) is to allow producing JS objects from records that conditionally include the keys depending if the record field optional value is None or Some _. This behavior differs from the regular behavior of records in Melange, where fields with None will end up with the property in the resulting object being set, with value undefined.

This is why I think including "record" in the name is misleading, and why somehow referring to the change of behavior would help to understand what this deriver is about.

What about splitting the deriver in two?

• deriving optional_keys (or deriving construct_optional_keys)
• deriving getters_setters

Then, they could be applied as a tuple deriving optional_keys, getters_setters.

[anmonteiro]

What about splitting the deriver in two?

• deriving optional_keys (or deriving construct_optional_keys)
• deriving getters_setters

Then, they could be applied as a tuple deriving optional_keys, getters_setters.

I like this idea, here's a prototype: #987

I named them make_opt_keys and getters_setters, but we should give them more descriptive names, probably including js_ somewhere (since they're really deriving JS objects).