Add context-free rule type that replaces AST nodes with attributes

[Skepfyr]

This adds a new kind of context-free rule that allows PPX authors to write PPXs that replace AST nodes that have their attributes on them. You can already achieve something like this with extension nodes, but the syntactic overhead for that is very high for some PPXs.

I wanted this specifically for ppx_template, which uses attributes to add suffixes to identifiers, adding an extension node is too heavyweight as the attributes need to get sprinkled all over your code.

[Skepfyr]

To clarify this change: some attributes get duplicated in the AST (that is, they point to the same location) but we only want to include them once in not_seen so that remove_seen works as expected.

[NathanReb]

This sounds sane! For future reference, do you have an example of when that happens? Something we can use to reproduce it?

Might actually be worth adding a test for this.

[Skepfyr]

I do but I'm unsure how to test it. OCaml 5.00 (and below) parses let v: (unit [@attr]) = () as let v: (unit [@attr]) = ((): (unit [@attr])) so the attr is visible down both bits of the value_binding causing it to get registered twice. I can't work out how to write a test that migrates the AST in the necessary way to check this.

[ceastlund]

If we take it out, does something fail internally here at Jane Street? If so, can we turn that into an external reproduction? If not, should this fix just be a separate PR?

[NathanReb]

If I understand the problem correctly our tests would need to run on OCaml <= 5.0, parse a source file with an attribute set as you described above, and have a ppx driver run on it and "consume" the attribute. Before your fix this should trigger the bug and we should get an unused attribute warning because of the second "shadow" attribute not being marked as seen. After your fix this should go away. Am I correct?

I think you can simply restrict the compiler version under which the test is run, using dune's enabled_if stanza field, as it is done here for example.

You make it sound like it's trickier than that so I might have missed something.

Either way, I think this would be best fixed in its own PR as @ceastlund suggests!

[Skepfyr]

I would quite like this change in this PR because ppx_template fails internally without it, however I could move it out and we can maintain the patch while we work out what's going on. However, this is my current understanding:

From AST version 500 and earlier, value_binding would include the attribute in the AST twice, which would cause weirdness described. No-one hit this because nothing used Attribute_table for attributes on value bindings before.
The AST migration from 500 to 501 fixes the duplication, this means that all versions of ppxlib since then can't hit this specific version of the bug, I don't know any other places where attributes get duplicated so I can't write a test for this.
I hit this because of weirdness in our internal toolchain setup that I haven't got to the bottom of.

Given that I'd quite like to include this change, it feels conceptually correct and it fixes an issue for us internally, but I'm relatively sure that it makes no difference to ppxlib's behaviour.

[NathanReb]

I'm fine merging this as is, the change seem sane. I was suggesting we add a test to prevent reintroducing it someday but if it's too cumbersome to write a meaningful test, let's go ahead and merge!

[NathanReb]

I think this introduces a bit of confusion as to what the expanded code will look like. The clear split between attributes and extension in our rules so far also helps understand what part of the code stays, and what gets rewritten.

ppx_template does not have documentation yet. Could you give me an example of how it's used and what kind of syntactic overhead it introduces when using extension points?

[NathanReb]

Reading the opam description of the package:

This PPX rewriter is used internally at Jane Street to simulate polymorphism over various extensions to OCaml's type system, in cases where there is not yet built-in compiler support. The long-term goal is thus to delete it entirely.

I realize this is meant to be a temporary work around to current compiler limitations. I'm a bit more reluctant to add this to ppxlib if the ppx that justifies this feature is meant to be deleted in the end.

Are none of the work around available satisfactory at the moment?

One other option is to use a whole AST rewriting pass to deal with those attributes.

[Skepfyr]

Long-term there is probably at least 3 years, and it may well gain new features that keep it alive indefinitely.

ppx_template essentially adds C++ style templates in that you can write:

let%template [@mode m = (local, global)] map : 'a t @ m -> ('a @ m -> 'b @ m) -> 'b t @ m

and it will do some copy-paste + name-mangling and produce:

let map__global : 'a t @ global -> ('a @ global -> 'b @ global) -> 'b t @ global
let map__local : 'a t @ local -> ('a @ local -> 'b @ local) -> 'b t @ local

(Don't worry too much about what that means, the important bit is that it's duplicated the function and done some name-mangling.)

The motivation for this change comes at the use sites, at the moment, the code looks like one of these:

let do_a_thing =
  ... 
  let a = [%template map [@mode local]] foo in
  ...
let%template do_b_thing =
  ... 
  let b = (map [@mode local]) foo in
  ...

The first one is too noisy especially if there are a bunch of this kind of function near each other, the second form has confused and worried users internally about how large the scope of the extension node is and causes some confusion over whether the function itself is templated or whether it just uses templates.

[Skepfyr]

I don't quite understand what you mean by:

I think this introduces a bit of confusion as to what the expanded code will look like. The clear split between attributes and extension in our rules so far also helps understand what part of the code stays, and what gets rewritten.

Are you saying that currently only extension nodes are rewritten and attributes only ever add code, and that split is useful?

[ceastlund]

I think in general, it is useful for extensions to rewrite their contents and attributes to be metadata. I think for our use case and perhaps some others, attributes are more natural. In part, it's because we are rewriting the attached expression/etc. to some version of itself, rather than to some new thing. So for example module M : S [@mode portable] reads more clearly as module M : S (* where [S] has some [portable] notion to it *), whereas module M : [%mode (S @ portable)] reads as module M : _ (* where [_] is in some way generated from the names [S] and [portable] *).

I'd like us to add the ability to register attribute transformers, but perhaps we should add caveats to the documentation explaining it should be used sparingly, and only for cases where one is, essentially, transforming likes-to-likes, or a thing to a version of itself.

As for ppx_template being temporary---I think any given usage of it is probably temporary, but I think the ppx itself will live on indefinitely, as different use cases cycle in and out of it. So we do want long-term support for this.

@NathanReb, how does this sound?

[NathanReb]

Are you saying that currently only extension nodes are rewritten and attributes only ever add code, and that split is useful?

Yes, that's what I meant!

I think one of the goals of the ppx approach was to have preprocessors agree on a universal syntax to make it easier for users to read code using preprocessors without necessarily having previous knowledge of all them. I think this feature and this ppx venture out of the conventional use of attributes and thus can lead to confusing syntax.

That being said, I understand it is useful in your case and this would not be the first strange exception to the extension vs attribute rule. I just want to make sure there is no simpler alternative and that it is only used by careful and experienced ppx authors who weighted the pros and cons of introducing such syntax.

@ceastlund yes, documenting this feature properly so it is not used lightly sounds good to me!

[Skepfyr]

@NathanReb I've added another paragraph to the docs attempting to make that clearer. Are you happy with that? I could add that warning somewhere else if you want it to be even louder.

[ceastlund]

We should also add some caveat in context_free.mli.

[NathanReb]

This sounds sane! For future reference, do you have an example of when that happens? Something we can use to reproduce it?

Might actually be worth adding a test for this.

[NathanReb]

The documentation looks fine, thanks for taking the time to add this!

[Skepfyr]

Thanks for the review, and sorry for the gap, I've been busy the past 2 weeks but I should be much more responsive now.

[Skepfyr]

Do you know a solution to #574 (comment)? (I'm mostly linking it because GitHub makes it easy to lose some review comments)

[NathanReb]

Other than the copied attribute bug fix, this looks good to go. If we move it to its own PR I'm happy to merge the rest as is!

[NathanReb]

Thanks, let's merge!