Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensions declared without prefixes are prone to clashes #14

Open
NathanReb opened this issue Mar 12, 2024 · 3 comments
Open

Extensions declared without prefixes are prone to clashes #14

NathanReb opened this issue Mar 12, 2024 · 3 comments
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.

Comments

@NathanReb
Copy link

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
Copy link

sim642 commented Mar 12, 2024

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
Copy link
Author

NathanReb commented Mar 13, 2024

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
Copy link
Author

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

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.
Projects
None yet
Development

No branches or pull requests

3 participants