Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ unreleased
location which can lead to unhelpful error messages.
(#569, @NathanReb)

- Add a new context-free rule type that replaces AST nodes that have the registered
attributes attached to them. (#574, @Skepfyr)

0.36.0 (2025-03-03)
-------------------

Expand Down
110 changes: 101 additions & 9 deletions doc/writing-ppxs.mld
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ into another one. A transformation can be:
[structure -> structure] or [signature -> signature], that can sometimes take
extra information as additional arguments. Such a transformation is applied in
the {{!driver."global-transfo-phase"}global transformation phase}, unless it
has a good reason to have been registered in another phase. While global transformations are a flexible and powerful tool in the OCaml ecosystem, they come with many {{!global_transformation}drawbacks} and should only be used when really necessary.
has a good reason to have been registered in another phase. While global transformations are a flexible and powerful tool in the OCaml ecosystem, they come with many {{!global_transformation}drawbacks} and should only be used when really necessary.

In order to register a transformation to the [ppxlib] driver, one should use the
{{!Ppxlib.Driver.V2.register_transformation}[Driver.V2.register_transformation]}. This function is used to register all
Expand All @@ -34,19 +34,20 @@ of the context-free pass. A rule contains the information about
when it should be applied in the traversal, as well as the transformation to
apply.

Currently, rules can only be defined to apply in four different contexts:
Currently, rules can only be defined to apply in five different contexts:

- on extensions points, such as [\[%ext_point payload\]]
- on some structure or signature items with an attribute, such as
- on some structure or signature items with a deriving attribute, such as
[type t = Nil \[@@deriving show\]],
- on AST nodes with attributes, such as [let x = 42 [@@attr]],
- on
{{:https://v2.ocaml.org/manual/extensionsyntax.html#ss:extension-literals}
litterals with modifiers}, such as [41g] or [43.2x],
- on function application or identifiers, such as [meta_function "99"] and [meta_constant].

In order to define rules on extensions points, we will use the {{!Ppxlib.Extension}[Extension]}
module. In order to define rules on attributed items, we will use the
{{!Ppxlib.Deriving}[Deriving]} module. For the two other rules, we will directly use the
module. In order to define deriving rules, we will use the {{!Ppxlib.Deriving}[Deriving]}
module. For the three other rules, we will directly use the
{{!Ppxlib.Context_free.Rule}[Context_free.Rule]} module.

{2 Extenders}
Expand Down Expand Up @@ -124,7 +125,7 @@ produced node will have to match the {{!ext_context}type of extension node we re

The above pattern extracts a string inside an extension node pattern. It will extract ["string"] in the the extension node [[%ext_name "string"]] and will refuse [[%ext_name 1+1]]. For other ready-to-use examples of patterns, refer to the {{!"matching-code".pattern_examples}example} section. For more in-depth explanation on the types and functions used above, see the {{!"matching-code"}Destructing AST nodes} chapter and the {{!Ppxlib.Ast_pattern}[Ast_pattern] API} .

The unit argument in [extractor] is not important. It is added so that {{:https://v2.ocaml.org/manual/polymorphism.html#ss:valuerestriction}value restriction} does not add noise to the type variables.
The unit argument in [extractor] is not important. It is added so that {{:https://v2.ocaml.org/manual/polymorphism.html#ss:valuerestriction}value restriction} does not add noise to the type variables.

{3 The Expand Function}

Expand All @@ -136,8 +137,7 @@ Building and inspecting AST nodes can be painful due to how
modules to ease this generation, such as {{!Ppxlib.Ast_builder}[Ast_builder]},
{!Ppxlib_metaquot}, {{!Ppxlib.Ast_pattern}[Ast_pattern]}, and {{!Ppxlib.Ast_traverse}[Ast_traverse]}, which are
explained in their own chapters: {{!"generating-code"}Generating AST nodes},
{{!"matching-code"}Destructing AST nodes} and {{!"ast-traversal"}Traversing AST
nodes}.
{{!"matching-code"}Destructing AST nodes} and {{!"ast-traversal"}Traversing AST nodes}.

In the example below, you can ignore the body of the function until reading
those chapters.
Expand Down Expand Up @@ -357,6 +357,98 @@ contexts. As the API shows, derivers are restricted to being applied in the foll

in both structures and signatures.

{2 Attribute-guided Rewriting}

[ppxlib] provides context-free rules that, like derivers, apply to nodes based on their
attributes but, like extenders, allow rewriting the entire AST node. These provide
lighter-weight syntax than extenders but that also means it's less obvious that they're
rewriting the syntax tree.

Before using this kind of rule, carefully consider using an extender instead. [ppxlib]
provides an opinionated syntax for preprocessors so that it's easy for users to understand
what code is being affected by the PPX. In general, these should only be used to slightly
modify the node the attribute is attached to, rather than rewrite it to something new.
The syntax of extenders highlights to users where more involved rewriting is taking place.

These are composed of:
- The name of the rewrite rule
- The list of attributes they define
- The expand function

They are defined to apply in a specific context, specifically, they can be registered to
be processed in the same contexts as extenders can occur.

{3 The List of Attributes}

A given rewrite rule can have multiple attributes that trigger it, if any of the
attributes are present on a single node then the rule is triggered and provided with the
AST node along with the payload of all the attributes registered by this rule. To declare
attributes use the {{!Ppxlib.Attribute.declare}[Attribute.declare]} function (or the other
similar functions in that module). Note that the {{!Ppxlib.Attribute.Context}[Context.t]}
must match the type of AST nodes that the rule will apply to.

{@ocaml[
# let prefix_attr = Attribute.declare "example.prefix" Expression
Ast_pattern.(single_expr_payload (estring __)) Fun.id
and suffix_attr = Attribute.declare "example.suffix" Expression
Ast_pattern.(single_expr_payload (estring __)) Fun.id ;;
val prefix_attr : (expression, string) Attribute.t = <abstr>
val suffix_attr : (expression, string) Attribute.t = <abstr>
]}

{3 The Expand Function}

The expand function takes the AST node (with this rule's attributes already stripped) and
the payloads of all the declared attributes (as a list of [payload option] to allow for
attributes that haven't been included).

{@ocaml[
# let expander
~ctxt
expression
([ prefix; suffix ] : _ Context_free.Rule.Parsed_payload_list.t)
=
match expression.pexp_desc with
| Pexp_ident { txt = Lident name; loc } ->
let prefixed = Option.value ~default:"" prefix ^ name in
let suffixed = prefixed ^ Option.value ~default:"" suffix in
{ expression with pexp_desc = Pexp_ident { txt = Lident suffixed; loc } }
| _ -> expression ;;
val expander :
ctxt:'a ->
expression ->
(string * (string * unit)) Context_free.Rule.Parsed_payload_list.t -> expression =
<fun>
]}

{3 Creating a rewriting rule}

Finally, we can create the rule using the appropriate
{{!Ppxlib.Extension.Context}[Ppxlib.Extension.Context]} and register it with the driver using
{{!Ppxlib.Context_free.Rule.attr_multiple_replace}[Context_free.Rule.attr_multiple_replace]}.
There's also a {{!Ppxlib.Context_free.Rule.attr_replace}[Context_free.Rule.attr_replace]}
function with a slightly simpler API if you only use a single attribute.

{@ocaml[
# let rewrite_rule = Context_free.Rule.attr_multiple_replace "example" Expression
[ prefix_attr; suffix_attr ] expander ;;
val rule : Context_free.Rule.t = <abstr>
# Driver.register_transformation ~rules:[rewrite_rule] "example" ;;
- : unit = ()
]}

Now, for example, the following:

{@ocaml[
let _ = foo [@prefix "p_"] [@suffix "_s"]
]}

will be rewritten to:

{@ocaml[
let _ = p_foo_s
]}

{2 Constant Rewriting}

OCaml integrates a
Expand Down Expand Up @@ -441,7 +533,7 @@ With such a rewriter registered:
Global transformations are the most general kind of transformation. As such, they allow doing virtually any modifications, but this comes with several drawbacks. There are very few PPXs that really need this powerful but dangerous feature. In fact, even if, at first sight, it seems like your transformation isn't context-free, it's likely that you can find a more suitable abstraction with which it becomes context-free. Whenever that's the case, go for context-free! The mentioned drawbacks are:

- It is harder for the user to know exactly what parts of the AST will be
changed. Your transformation becomes a scary black box.
changed. Your transformation becomes a scary black box.
- It is harder for [ppxlib] to combine several global transformations, as there is no
guarantee that the effect of one will work well with the effect of another.
- The job done by two global transformations (e.g., an AST traverse) cannot be
Expand Down
2 changes: 1 addition & 1 deletion src/attribute.ml
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ let collect =
=
let loc = Common.loc_of_attribute attr in
super#payload payload;
Attribute_table.add not_seen name loc
Attribute_table.replace not_seen name loc
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

end

let collect_unseen_errors () =
Expand Down
1 change: 1 addition & 0 deletions src/attribute.mli
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ module Context : sig
val psig_extension : signature_item t
val rtag : row_field t
val object_type_field : object_field t
val equal : 'a t -> 'b t -> bool
end

val declare :
Expand Down
Loading