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

Add qualifiers for type variables in generated Erlang typespecs #3221

Closed
dvic opened this issue May 31, 2024 · 16 comments · Fixed by #3247
Closed

Add qualifiers for type variables in generated Erlang typespecs #3221

dvic opened this issue May 31, 2024 · 16 comments · Fixed by #3247
Labels
bug Something isn't working good first issue Good for newcomers help wanted Contributions encouraged priority:high

Comments

@dvic
Copy link
Contributor

dvic commented May 31, 2024

Starting from OTP 26 erlang/otp#6915, warnings are emitted for unbound type variables, for example:

 -spec try_call_timeout_test() -> {ok, GGA}

We need to replace this unbound type variable with term() so that no warning is emitted.

originally reported in gleam-lang/erlang#46

@dvic dvic added the bug Something isn't working label May 31, 2024
@dvic
Copy link
Contributor Author

dvic commented May 31, 2024

@lpil isn't it better to use a bound instead of term directly? Let's say this was the function (from the OTP PR mentioned above):

-spec foo(atom()) -> {ok, X} | {error, X}

Then instead of

-spec foo(atom()) -> {ok, term()} | {error, term()}

I think it's better to have

-spec foo(atom()) -> {ok, X} | {error, X} when X :: term()

In this way, the typespec correctly signals that the type should be the same in the two branches?

@lpil
Copy link
Member

lpil commented May 31, 2024

@dvic The example you give there doesn't need to be changed, it's only when a parameter is used once that it is invalid. What you've suggested it be changed to is also the same and is redundant as term() offers no constraint at all.

Thank you @dvic !

@lpil lpil changed the title Replace unbound type variable with term Replace type variable with term() it only appears once in the signature May 31, 2024
@lpil lpil added help wanted Contributions encouraged good first issue Good for newcomers priority:high labels May 31, 2024
@dvic
Copy link
Contributor Author

dvic commented May 31, 2024

@dvic The example you give there doesn't need to be changed, it's only when a parameter is used once that it is invalid.

Thank you @dvic !

Are you sure? I just double checked this locally with the example from erlang/otp#6915:

-module(foo).

-export([foo/1]).

-spec foo(atom()) -> {ok, X} | {error, X}.
foo(X) ->
    {ok, X}.
❯ erlc foo.erl
foo.erl:5:27: Warning: type variable 'X' is only used once (is unbound)
%    5| -spec foo(atom()) -> {ok, X} | {error, X}.
%     |                           ^

While

-module(foo).

-export([foo/1]).

-spec foo(atom()) -> {ok, X} | {error, X} when X :: term().
foo(X) ->
    {ok, X}.

does not emit an warning (this is on OTP 26.2.5).

@lpil
Copy link
Member

lpil commented May 31, 2024

Huh. OK I don't understand what's happening here. It says "once" but it's clearly used twice. Is this a bug in erlc?

@dvic
Copy link
Contributor Author

dvic commented May 31, 2024

Huh. OK I don't understand what's happening here. It says "once" but it's clearly used twice. Is this a bug in erlc?

Yeah the message is a bit confusing, I think it's more about the (is unbound) part. Apparently for the compiler X is being used twice in the following example, once in the return type and once in the type guard.

-spec foo(atom()) -> {ok, X} | {error, X} when X :: term().

@lpil
Copy link
Member

lpil commented May 31, 2024

I've opened an issue with Erlang/OTP to find out if this warning is emitted incorrectly, or if the text is misleading.

@dvic
Copy link
Contributor Author

dvic commented May 31, 2024

@lpil I may have confused you with the following comment
image

OTP 26.2.5 also emits a warning without the type guard, it's because of the added type guard that no warning is emitted.

I posted a clarification on the issue as well.

@lpil
Copy link
Member

lpil commented May 31, 2024

Thank you.

This is all very confusing to me. I guess we add seemingly redundant qualifiers to everything!

@lpil lpil changed the title Replace type variable with term() it only appears once in the signature Add qualifiers for type variables in generated Erlang typespecs May 31, 2024
@dvic
Copy link
Contributor Author

dvic commented May 31, 2024

Thank you.

This is all very confusing to me.

Yeah it is, I guess we're all spoiled by the errors in Gleam ;)

@lpil lpil changed the title Add qualifiers for type variables in generated Erlang typespecs Replace all type variables with term() in generated Erlang Jun 5, 2024
@lpil lpil changed the title Replace all type variables with term() in generated Erlang Add qualifiers for type variables in generated Erlang typespecs Jun 5, 2024
@lpil
Copy link
Member

lpil commented Jun 5, 2024

Looks like I'm totally wrong about how they work. erlang/otp#8533

@dvic
Copy link
Contributor Author

dvic commented Jun 5, 2024

I saw the conversation :) So for now we only add X :: term() when a type parameter is not used in the input? That should cover it, right?

@lpil
Copy link
Member

lpil commented Jun 5, 2024

I think so? It's still incorrect though unfortunately.

@dvic
Copy link
Contributor Author

dvic commented Jun 5, 2024

I think so? It's still incorrect though unfortunately.

Yeah, it's the best we can do for now.

I was busy with playing around with gleam-lang/erlang#45 but I can pick this one up, wanted to get familiar with the Rust codebase anyways :)

@dvic
Copy link
Contributor Author

dvic commented Jun 6, 2024

@lpil any idea how I can trigger the issue in a isolated test? I tried this but any() is generated:

image

It makes me think: isn't that what should have been generated in the example in this issue?

@dvic
Copy link
Contributor Author

dvic commented Jun 6, 2024

Never mind, I have it reproduced now with:

image image

@dvic
Copy link
Contributor Author

dvic commented Jun 6, 2024

PR is submitted!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Contributions encouraged priority:high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants