Attributes in Ast_builder.Default and Ast_pattern

[zepalmer]

The modules Ast_builder.Default and Ast_pattern do not provide a facility to construct or extract attributes from expressions and other types. For instance, consider the task of writing a transformer foo which distributes attributes through subexpressions. That is,

[%foo (1+2)[@bar]]

would become

(1[@bar]) + (2[@bar])

There does not seem to be a way to match an expression or build an expression using these libraries in such a way that I could move or copy the attributes provided in the original source code without accessing the pexp_attributes field myself. I agree with the ppxlib documentation when it argues for using an abstraction layer to interact with the AST in order to maintain compatibility with future versions of the AST, but that doesn't seem to be possible when interacting with attributes given the current state of the AST modules. This seems like a natural point of potential growth for this library. :)

One intuitive approach would be to include an optional argument on constructor functions. For instance, the current signature of Ast_builder.Default.pexp_sequence is loc:Location.t -> expression -> expression -> expression. It might instead be ?attrs:attributes -> loc:Location.t -> expression -> expression -> expression. This would allow basic usage of the existing module to function in a backward-compatible way and allow attributes to be introduced when necessary.

Another approach would be to include a modification function such as

let pexp_with_attributes (attrs : attributes) (expr : expression) : expression =
  { expr with pexp_attributes = attrs }
;;

This is likely less performant as it would need to create an additional hep allocation, but it is less invasive in that it does not modify existing routines.

A dual adjustment would be appropriate for Ast_pattern to allow attributes to be retrieved when necessary.

I'm not sure which of the two approaches above is more palatable (or if there is a better alternative I haven't considered). Any thoughts?

[pitag-ha]

Hello @zepalmer, thanks for the issue! You're right: Ast_builder/Ast_pattern should provide a way to construct/descruct nodes with attributes. The current workflow to build nodes with attributes is: Whenever metaquot can't be used, we build the node without the attributes and then functionally update that node to add the attribute(s). I.e. we do exactly what you've described in your second approach mentioned above, but with custom pexp_with_attributes-like functions. Given that it's extremely unlikely that the <node_name>_attributes-field of a node will be renamed, that isn't a huge problem wrt stability, but it's true that we should provide an official way.

I also agree with your judgement for both your mentioned approaches: Adding an optional attrs argument to the builder functions would be the ideal solution, but it would be more work to implement that. Adding a pexp_with_attributes for all nodes allowing attributes that functionally updates the node to add attributes would be a worse but a reasonable lower-hanging alternative.

We currently have very little time for ppxlib. Do you happen to want to work on this? I'd be happy to give some pointers. Both Ast_builder and Ast_pattern are generated modules, so the implementation of this needs to handle the generation logic.

[zepalmer]

I'm quite interested in making these adjustments contingent upon the timeline you have in mind. May I presume that there is no huge rush?

Given the opportunity, I think I'd like to implement both of the approaches described above. The functional update is useful in cases where we want to extend or filter attributes, for instance, which would otherwise require destructing and reconstructing the node. The work I've been doing, on the other hand, has been synthesizing some data structures to which I've wanted to add deriving annotations. So I see a use case for both approaches, but I'd obviously want the blessing of the project maintainers before I added anything. :)

[pitag-ha]

I'm sorry, I've totally missed your answer.

I'm quite interested in making these adjustments contingent

That's great news, thank you!

May I presume that there is no huge rush?

Indeed, there's no rush. You can make your own timeline (and I'll try not to miss your messages anymore and answer questions with less delay).

I think I'd like to implement both of the approaches described above.

Sure, that sounds good. I'd appreciate a lot if you could then make the difference clear in the documentation, i.e. the API documentation and/or the manual, ideally both.

Do let me know if you have questions to get started!