Be more relaxed about ppx attributes.

[jordwalke]

In Reason, we preserve the original string content in the AST and attach them to parsed strings (which are manipulated to some extent by the parsing process).

When you write:

let%test "literal" = ..

You effectively end up with:

let%test ("literal" [@@reason.literal_value "literal"]) = ...

And the ppx_inline_test ppx reports: "Error: Attributes not allowed here"

If we'd like to use ppx_inline_test with Reason, then we will need ppx_inline_test to be a little more relaxed about allowing attributes on the string literal. Is this possible?

reasonml/reason#2134

[jordwalke]

This is likely also an issue for ppx_expect if it uses the same logic.
Confirmed the same issue happens with ppx_expect.

[ghost]

Completely removing this check would be a bit dodgy as this would mean that if the user explicitly put an attribute it would be silently ignored.

However, we could whitelist attributes generated by reason in ppxlib. We already do this for merlin for instance.

What would help is to come up with a convention for machine generated attributes that can safely be ignored. For instance using the _ prefix in attribute names.

[ghost]

/cc @let-def @trefis, what do you think about using a convention for machine generated attributes?

[jordwalke]

We currently use the attributes @@reason.literal_value and will likely use @@reason.* or @@refmt.* to store formatting attributes in AST nodes. We also make heavy use explicit_arity attributes which we believe may also be causing issues (with :MerlinTypeOf, but not causing problems with compilation). The explicit_arity may add one extra level of depth (Tuple) to patterns.

[ghost]

Ok, are you happy to prefix the name of these attributes with _? At least we can just add a general rule to ppxlib and we will have a solution ready for the next tool that needs something like this.

[let-def]

A _ prefix is fine I think.

[jordwalke]

We won't be able to go retroactively update older versions of Reason already in use that use the @@reason.* prefix or @@refmt.* prefix.

[zindel]

I'd like to support this request. We cannot use the ppx_expect tests written in Reason in fastpack because of it.

[rgrinberg]

How about temporarily hard coding reason. and refmt. in ppxlib until reason transitions to the _ prefix?

[ghost]

We can do both at once, it's the same amount of work to do either. I created this ticket to track this issue

[v-gb]

Closing this, because it looks like @jeremiedimino solved this by doing both a special case for attributes starting with "reason" or "refmt" and the general thing of ignoring attribute that start with underscores.