janestreet / ppx_let

Monadic let-bindings
MIT License
113 stars 12 forks source link

Extensions declared without prefixes are prone to clashes #14

Open NathanReb opened 4 months ago

NathanReb commented 4 months ago

We recently tried to release a new version of ppx_deriving where the set of standard derivers it includes were ported to ppxlib's deriving API instead of the deprecated ppx_deriving one.

Doing so uncovered a clash in extension names between ppx_deriving.map and ppx_let.

Each ppx_deriving standard derivers comes with an expression extension which can be used to derive the relevant inline function for a given core_type. E.g. with ppx_deriving.show you can write:

Printf.printf "%s" ([%show: int list] [1; 2; 3])

Those extensions are declared as derive.<deriver_name>, allowing other ppx-es to declare other such extensions as well and be used alongside ppx_deriving's standard derivers.

ppx_let defines a [%map ...] extension without any prefix making it impossible for any other ppx to declare an extension named map with or without prefixes as it is then impossible to disambiguate the ppx_let's extension.

I believe declaring all ppx_let extensions as ppx_let.<extension_name> would fix this issue while being a seemless change to users.

Does this sound like an acceptable change to you? I'm happy to submit a PR if so.

sim642 commented 4 months ago

Actually, the interesting thing (which is maybe why the two could coexist some projects so far?) is that they use disjoint argument patterns:

  1. ppx_deriving.map takes a ptyp argument, e.g. [%map: ...].
  2. ppx_let takes a single_expr_payload argument, e.g. let%map ... which is [%map let ...].

I guess ppxlib cannot resolve the conflict based on the patterns, but conceptually it sounds possible. Although having ppx_let declare itself with a prefix would only be good.

NathanReb commented 4 months ago

I don't think that's why they could cohabit before. map used to be declared via ppx_deriving's api which registered to ppxlib as a whole file transform and not a context free rule, ppxlib was not in charge of interpreting ppx_deriving.map's extension and knew nothing about it.

NathanReb commented 4 months ago

Regardless of what is done on ppx_deriving's side, it's good practice to allow prefixed extension names.