Skip to content

Conversation

@ilya-klyuchnikov
Copy link
Contributor

@ilya-klyuchnikov ilya-klyuchnikov commented Nov 24, 2020

Fixes #20

Change

Rolling back e916333. + Changing the logic of handling type variables in specs:

If a generated/translated spec has a type variable which is used exactly once (erl_lint calls such variables "unbound variables"), such a variable is replaced with the wildcard type variable (_). This makes erlc/erl_lint happy about generated code.

Snapshot tests are updated accordingly.

Subtlety

This new logic doesn't try to distinguish "original" (but also unbound for erlc) OCaml type variables and inferred type variables: if a type variable is unbound it will be replaced by _ anyway.

This makes this example less idiomatic:

let where_is : 'a -> 'message Erlang.pid option =

Before:

-spec where_is(_a) -> option:t(beam__erlang:pid(_message)).

After:

-spec where_is(_) -> option:t(beam__erlang:pid(_)).

Possible improvement(s)

  • To always prefix unbound variable with _, eg the unbound type variable _X is totally fine for erl_lint and falls into the category "I don't care".
  • To introduce some logic to distinguish "original" type variables (like in where_is) and to not replace them with _.
  • More idiomatic OCaml code in erlang/src/erl_ast_helper.ml - I did not read many parts of the project in detail, - so my code is likely not of the same style as the rest of the project.

Thanks for a really cool and interesting project!

Comment on lines +175 to +176
when List.length (List.find_all (( = ) n) occurs) < 2 ->
Type_variable (Var_name "_")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main logic. Probably, it's worth to extract the condition into a separate function something like is_var_unbound?

Copy link
Owner

Choose a reason for hiding this comment

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

Makes sense, we can do this small refactors later on.

@leostera leostera merged commit cbf62f6 into leostera:main Dec 8, 2020
@leostera
Copy link
Owner

leostera commented Dec 8, 2020

Thanks for this! It should make it easier to work with type variables going forward 😄

(Merging it now before I start doing more changes to the Erlang Parsetree and it bitrots)

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.

Safe and named fresh type variables

2 participants