gleam-lang / gleam

⭐️ A friendly language for building type-safe, scalable systems!
https://gleam.run
Apache License 2.0
17.42k stars 723 forks source link

Add qualifiers for type variables in generated Erlang typespecs #3221

Closed dvic closed 2 months ago

dvic commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 !

dvic commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

dvic commented 3 months ago

@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 commented 3 months ago

Thank you.

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

dvic commented 3 months ago

Thank you.

This is all very confusing to me.

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

lpil commented 3 months ago

Looks like I'm totally wrong about how they work. https://github.com/erlang/otp/issues/8533

dvic commented 3 months ago

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 commented 3 months ago

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

dvic commented 3 months ago

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 https://github.com/gleam-lang/erlang/pull/45 but I can pick this one up, wanted to get familiar with the Rust codebase anyways :)

dvic commented 3 months ago

@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 commented 3 months ago

Never mind, I have it reproduced now with:

image image
dvic commented 3 months ago

PR is submitted!