map plugin: add a type annotation to avoid warning 41

[emillon]

Hi!

In some cases like the following, the generated code can emit instances of warning 41.

type 'a t1 = { x : 'a ; y1 : int } [@@deriving map]
type 'a t2 = { x : 'a ; y2 : int } [@@deriving map]

An alternative would be to add a constraint to the generated map function, but it does not seem to play well with polymorphism.

I also removed an intermediate List.concat since the list is always a singleton.

Let me know what you think.

Thanks!

[gasche]

Warning 41 (Ambiguous constructor or label name.) occurs when several record declarations with the used label are in scope, but the type information does not suffice to disambiguate which is meant (so we default to the declartion introduced last in scope).

It would be possible to fix the warning rather than silencing it, by adding an annotation in the generated code. (I think that would be slightly preferable if the cost is low, as it makes the code more robust to tooling changes etc.) Right now the generated code is as follows:

type 'a t1 = {
  x: 'a ;
  y1: int }[@@deriving map]
let rec map_t1 poly_a x = { x = (poly_a x.x); y1 = ((fun x -> x) x.y1) }
  [@@ocaml.warning "-39"]
type 'a t2 = {
  x: 'a ;
  y2: int }[@@deriving map]
let rec map_t2 poly_a x = { x = (poly_a x.x); y2 = ((fun x -> x) x.y2) }
  [@@ocaml.warning "-39"]

and the problematic part is the second occurrence of x.x, which may reference to either t1 or t2. Using

let rec map_t2 poly_a (x : _ t2) = ...

would suffice to silence the warning, but of course we could think of a more complete annotation.

Do some derivers already take care of producing the right annotation, whose code could be reused?

[gasche]

P.S.: I'm not convinced by the removal of the List.concat. It is a general design pattern to assume that str_of_type may return zero, one or several structure items for each type declarations, that occurs across many different deriving plugins. Right now the change looks like a simplification, but if you wanted to change the code later to generate an auxiliary declaration/module you would have to change back.

(Just for fun I grepped for lines of code containing type_decls) in my ppx directories. 34 of them also contain List.concat, and only 8 don't.)

[emillon]

_ t2 seems to be a good balance between suppressing the warning and adding a full polymorphic type annotation. I'll change to that.

For the concat, I don't think it's worth adding code "just in case". It's a private API just local to the module and it would be just as easy to add back, but in the meantime it just adds some noise to a module that's already pretty hard to read because of the domain logic. If you insist on keeping the concat call, may I suggest extracting a stri_of_type that just returns the structure item, a str_of_type that builds the list?

[emillon]

And done! It seems that core_type_of_type_decl does the right thing: the generated code is now let rec map poly_a poly_b (x : ('a, 'b) t) = ....

[gasche]

The new approach looks very nice to me, thanks!

[gasche]

I'm not sure why we delayed merging this for so long, I think I thought @emillon would merge or something. Apologies for the delay, merging now.

[emillon]

No problem! I had forgotten as well 😅 .