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

Gensym improvements #285

Merged
merged 7 commits into from
Sep 27, 2021
Merged

Conversation

ceastlund
Copy link
Collaborator

Improved two aspects of gen_symbol:

  1. passing one gen_symbol result as ~prefix to another no longer results in a double-suffix
  2. name_type_params_in_td produces more sensible variable names
    First added tests for both behaviors, then improved them and updated tests.

@ceastlund ceastlund requested a review from a user September 23, 2021 17:54
Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

I'm not very familiar with name_type_params_in_td. Why is it better to use a, b, ... as a prefix for the name instead of v_x or no prefix at all?

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@ceastlund
Copy link
Collaborator Author

The name_type_params_in_td function assigns names to type parameters declared as underscores, because having names for them is often handy during ppx expansion. Let's say we start with:
type (_, _) t = int
Currently, name_type_params_in_td produces something like:
type ('v_x__097_, 'v_x__098_) t = int
The code is written using "v" ^ gen_symbol (), and gen_symbol has a ?prefix argument that defaults to "_x". I kind of suspect the code meant to use "v" as a prefix, and the author didn't think about the "_x" default. I suspect the intention was:
type ('v__097_, 'v__098_) t = int
There's no real reason why "_x" or "v_x" are at all meaningful or suggestive names for types. So I wanted to change that code to use gen_symbol's ~prefix argument.

While I was at it, it occurred to me that "v" is also not a great prefix for a type. Usually, v is for "value". We usually use the beginning of the alphabet, like:
type ('a, 'b) t = int
So I made the code do something similar:
type ('a__097_, 'b__098_) t = int

@pitag-ha
Copy link
Member

Oh nice, thanks for all the explanation! Using a..., b..., ... for type parameter names does indeed sounds more intuitive than v_... or v_x.... As a side-note: it's a..., b..., ..., not 'a..., 'b..., ..., right?

@ceastlund
Copy link
Collaborator Author

What gen_symbol generates has no apostrophe. But when wrapped in Ptyp_var and rendered back as ocaml syntax, there would be an apostrophe there.

ceastlund and others added 7 commits September 27, 2021 12:48
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
add PR attribution to gen_symbol fix

Co-authored-by: Sonja Heinze <sonjaleaheinze@gmail.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
add PR attribution to name_type_params_in_td change

Co-authored-by: Sonja Heinze <sonjaleaheinze@gmail.com>
Signed-off-by: Carl Eastlund <ceastlund@janestreet.com>
@ceastlund ceastlund merged commit 53f9101 into ocaml-ppx:main Sep 27, 2021
pitag-ha added a commit to pitag-ha/opam-repository that referenced this pull request Dec 8, 2021
CHANGES:

- Add support for OCaml 4.14 (ocaml-ppx/ppxlib#304, @kit-ty-kate)

- Expand nodes before applying derivers or other inline attributes based
  transformation, allowing better interactions between extensions and
  derivers (ocaml-ppx/ppxlib#279, ocaml-ppx/ppxlib#297, @NathanReb)

- Add support for registering ppx_import as a pseudo context-free rule (ocaml-ppx/ppxlib#271, @NathanReb)

- Add `input_name` to the `Expansion_context.Extension` and `Expansion_context.Deriver` modules (ocaml-ppx/ppxlib#284, @tatchi)

- Improve `gen_symbol` to strip previous unique suffix before adding a new one (ocaml-ppx/ppxlib#285, @ceastlund)

- Improve `name_type_params_in_td` to use prefixes `a`, `b`, ... instead of `v_x`. (ocaml-ppx/ppxlib#285, @ceastlund)

- Fix a bug in `type_is_recursive` and `really_recursive` where they would
  consider a type declaration recursive if the type appeared inside an attribute
  payload (ocaml-ppx/ppxlib#299, @NathanReb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants