Opaque is ignored when used with a variant

[cemerick]

Echoing a very similar issue (#194), this doesn't appear to work with version 4.4:

type foo = A
type bar = B of foo [@opaque] | C
           [@@deriving show]

(Error: Unbound value pp_foo)

but this does:

type foo = A
type bar = B of foo [@printer fun fmt _ -> Format.pp_print_string fmt "<opaque>"] | C
           [@@deriving show]

[gasche]

The problem is that the parsing rules attach the [@opaque] attribute to the whole B of foo constructor, not to the type expression foo as you intend. You should write B of (foo [@opaque]), and that should work.

I can add logic in the show plugin to propagate more attributes in these user-tripping cases, but it somehow feels wrong to me to try to fix a grammar difficulty with workaround code.

[gasche]

I realize that this position may be partly inconsistent with what we did for #194, where a similar addition of parentheses may also have worked to fix the issue without ppx_deriving-side change. The record case (not inline record) already had a sort of "attribute cascade" built-in to handle this case, and the inline-record fix was merely synchronizing the two constructs to behave in the same way.

On one hand I think we should strive to minimize the user's surprise, and behave in the expected way -- even if that means having workaround for parsing ambiguities. On the other, currently working around this issue needs to be done in all of the plugins separately, and for all AST constructs separately. It feels like there should be a nicer way to do it.

One thing I could do is add to the ppx_deriving API (accessible to all plugins) an auxiliary function to "push down" attributes as necessary to match user expectations. Plugin authors would have to decide whether they want to adhere to the built-in strict attribute-placement rules, or be more flexible, and they could be more flexible by systematically calling this function.

A language-level solution would be to support A[@bar] of foo or (A of foo)[@bar], and emit a warning in the ambiguous case A of foo [@bar], so that users are pushed towards non-surprising attribute placements.

[cemerick]

You should write B of (foo [@opaque]), and that should work.

You are correct, it does, thank you for pointing out the error. Now that you've explained what's happening, I sort of wish the same explicitness was expected for the records cases :stuck_out_tongue:

I won't presume to have actual opinions on the complex questions of ppx infrastructure decisions, but those helpers to make common transforms easier sound good. What's really missing is user feedback about the ambiguity; sussing out that an attribute is misplaced or is not having the intended effect is incredibly difficult in general (e.g. I always need to use trial and error to get warning attributes placed properly). At least for ppx's, being able to easily see the results of ppx "macroexpansion" in one's editor is sort of a magic bullet (when it works :laughing:).