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

Use newtype names as the underlying variable names #2277

Merged
merged 8 commits into from Mar 25, 2019

Conversation

@mrmr1993
Copy link
Contributor

commented Mar 2, 2019

This is fairly self-explanatory: if the name of a Pexp_newtype meets the criteria for a type variable name (checks associated with the error Invalid_variable_name), then we use that name for the variable we generate.

@hhugo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

Could you add a test with (type a')

@mrmr1993 mrmr1993 force-pushed the mrmr1993:named-newtype branch from 41279da to 9222a4f Mar 2, 2019
@mrmr1993

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Could you add a test with (type a')

Done, thanks!

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

This does make sense.
Note that the impact is small, because type a b c. <typexp> is processed at the syntactic level before expanding to (type a b c), and generates a type annotation with with the same variable names.

@@ -3230,7 +3230,12 @@ and type_expect_
re { exp with exp_extra =
(Texp_poly cty, loc, sexp.pexp_attributes) :: exp.exp_extra }
| Pexp_newtype({txt=name}, sbody) ->
let ty = newvar () in
let ty =
if name <> "" && name.[0] = '_' then (* Invalid variable name. *)

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Mar 5, 2019

Contributor

Typetexp (or Types,....?) could expose a val valid_tyvar_name: string -> bool (and use it internally in transl_type_param).

@mrmr1993

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Done! I changed the definition of what a valid name is slightly; I assume that the empty string shouldn't be a valid name..?

@mrmr1993

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@garrigue do you think this will make the next version? I've been suffering with the traditional generated names when module signatures don't match, it would help me a lot to have the (type ...)d names!

@trefis

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

@mrmr1993 if by next version you mean 4.08, for which we've had a couple of beta releases already, then I believe the answer is no.

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

It will be merged in trunk, which means being available for the next release, which will be 4.09.
You're free to use trunk (or a patched version of the compiler) if you need this earlier.

Just now I'm waiting for the CI check. It was failing (due to a conflict?).

@garrigue garrigue merged commit c33e46d into ocaml:trunk Mar 25, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garrigue

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.