Gradualizer does not compile with OTP 22

[gomoripeti]

The compiler now complains about the preludes trick :(

===> Compiling src/gradualizer_prelude.erl failed
src/gradualizer_prelude.erl:5: spec for function erlang:apply/2 from other module
...

this was expected to come, from OTP 22 release notes

--- POTENTIAL INCOMPATIBILITIES -------------------------------------

OTP-15563 Application(s): compiler, stdlib
Related Id(s): ERL-845, OTP-15562

Do not allow function specifications for functions
residing in other modules.

[zuiderkwast]

This is pretty serious.

Our list of overriding specs is pretty long and it makes me wonder if it's a practical approach or if we should go for something simpler. How about treating term() as any() for external functions? It's a bit radical, but term() almost never makes sense, so I think it could be a possible solution.

Otherwise, I guess it's possible to come around the problem by importing the specs in some other way. In worst case, we can put them in strings inside the code (e.g. "-spec erlang:hd([T]) -> T.") or so.

[hauleth]

Another possibility is to move all that definitions upstream.

[josefs]

@hauleth, we do plan to improve the typespecs in OTP. But we also want to be able to support a few older versions as well so we need a solution which works with the type specs in OTP right now.

[KennethL]

I suggest that you override the otp type specs by using a name convention. Put them in the same place as now but use typenames and function names like module_type and module_function and let gradualizer check if there is a type or function named list_to_atom(lists:concat([Module,TypeOrFunc)) among your overrides. By using this approach you can still put the overrides in a normal module and have it parsed and syntax checked. /Kenneth

…

On Fri, Sep 20, 2019 at 2:35 PM Josef Svenningsson ***@***.***> wrote: @hauleth <https://github.com/hauleth>, we do plan to improve the typespecs in OTP. But we also want to be able to support a few older versions as well so we need a solution which works with the type specs in OTP right now. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#187?email_source=notifications&email_token=AABFWSA6T4DZE7IZ433WPKDQKS7RFA5CNFSM4HPUJEE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7GRRFQ#issuecomment-533534870>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABFWSDND2RV3PXQBSHUPZDQKS7RFANCNFSM4HPUJEEQ> .

[zuiderkwast]

I suggest that you override the otp type specs by using a name convention. Put them in the same place as now but use typenames and function names like module_type and module_function

That's a good idea @KennethL! I guess there are no underscores in the module names we're overriding, so it should be unambiguous. Otherwise, we can use two underscores instead.

[KennethL]

There are underscores in some modules so it is better to select a more unique separator. But I guess you are only creating the name and then checking if that type exists so there might not be any problems anyway. You are not splitting typenames into Module and Type? In that case it is better with double underscore or some other unique separator. /Kenneth

…

On Fri, Sep 20, 2019, 15:37 Viktor Söderqvist ***@***.***> wrote: I suggest that you override the otp type specs by using a name convention. Put them in the same place as now but use typenames and function names like module_type and module_function That's a good idea @KennethL <https://github.com/KennethL>! I guess there are no underscores in the module names we're overriding, so it should be unambiguous. Otherwise, we can use two underscores instead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#187?email_source=notifications&email_token=AABFWSH7EOLLQJY2CMM6ICTQKTGZFA5CNFSM4HPUJEE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7GW6UY#issuecomment-533557075>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABFWSB2ACYLMTZWSQEXVVTQKTGZFANCNFSM4HPUJEEQ> .

[Zalastax]

When designing this, think about how it can be done without causing an overhead when it's not used. I think overrides should be placed in special files that only contain overrides. When those files are imported, gradualizer should make a translation from e.g.

-spec 'proplists:delete'(any(), list()) -> list().

to

-spec proplists:delete(any(), list()) -> list().

[zuiderkwast]

You are not splitting typenames into Module and Type?

@KennethL Yes, we do. We represent them as {Module, Typename, Arity}, if I remember correctly.

-spec 'proplists:delete'(any(), list()) -> list().

@Zalastax Yes, this is the way to go, I think, and only for the special module where we have these things.

[NelsonVides]

Hey there all,

I cam across this issue when I tried to use Gradualizer for the first time. As I like living on the edge (using the latest and greatest), and I don't want to give up on Gradualizer even before I started, I'm thinking, how to solve this issue.

Simple question, having a quick look at the code. You seem to be importing gradualizer_prelude on gradualizer_db by using import_beam_files. But just next to that function, you seem to be having a nicely named import_erl_files. Wouldn't just not compiling the prelude and importing its specs from the erl just do the job? I might be missing something most likely, but I'll enjoy you answer nevertheless, hope I can help somehow.

[gomoripeti]

iirc the reason why it is a compiled beam and not any other text file is that this way it can be included and retrieved from a zipped escript file. (code server supports archives, while including a priv dir is utterly inconvenient - see #74)

[zuiderkwast]

The same problem exists regardless if we import from erl or beam. The parser still complains about this.

Now, if we do the @Zalastax trick, in order to avoid the compiler error "spec for undefined function", we need an implementation for each function. The warning "function ... is unused" can be silented by a compile attribute.

-compile(nowarn_unused_function).
-spec 'erlang:apply'(function(), [any()]) -> any().
'erlang:apply'(_,_) -> error(undef).

There are quite many of these specs. Can we auto-generate such implementations with parse transform?

[gomoripeti]

I was also thinking about parse transforms but on a different way. Can't we somehow do the bulk of the import at compile time. So after transform module would look like this:

-module(gradualizer_prelude).
-export([get_prelude/0]).
get_prelude() ->
  <processed prelude>.

Where <processed prelude> can either be the output forms of epp:parse_file/2 or even the output of gradualizer_db:collect_specs/2 (for the later gradualizer_db also needs to be compiled before running the parse_trans, probably the former is much simpler). Because only the parser is run on the original file, there won't be any "spec for undefined function" warning (which I guess is emitted by a later step of the compiler)

The original file format could be:

-module(gradualizer_prelude).

-spec_module(lists).
-spec append([[T]]) -> [T].
-spec delete(T, [T])    -> [T].
...
-spec_module(proplists).
-spec delete(any(), list()) -> list().
...

Separating module from function name has the benefits of

• no need for function name trick convention
• easier to copy spec from original module

[NelsonVides]

@gomoripeti That is precisely what I was going to add. a precompile hook for rebar can generate a file that exports only that get_prelude/0 function as you describe.

[josefs]

That's a great suggestion @gomoripeti ! Do you have any cycles to have a go at implementing this?

[gomoripeti]

Unfortunately I dont have too many spare time so I only implemented a minimal parse transform to unblock compilation on OTP 22 #191

I think separating module names into separate attributes would still be beneficial, but I have to leave it for others.

[NelsonVides]

I just made a PR (#192) based on my idea with the precompiled hook, but I like @gomoripeti's PR, if anything, it's shorter, and most likely even cleaner.

[josefs]

Thanks both of you @NelsonVides and @gomoripeti for your PRs!

I like @gomoripeti's PR, if anything, it's shorter, and most likely even cleaner.

I tend to prefer it as well, it's a very simple solution that seems to be easy to maintain going forward.

[josefs]

@zuiderkwast and @Zalastax, do you have opinions on which PR you prefer?

[Zalastax]

Parse transform!

[NelsonVides]

Fair enough, I'm also for the parse transform, I just didn't know much of that possibility. Good chance to learn it anyway!

[erszcz]

There's one reason why a parse transform might not be as good as a Rebar3 pre-compile hook - Elixir deprecated using parse transforms in Erlang dependencies.

If the focus is to provide type checking for the BEAM, not just Erlang, a standalone script is preferable, since it might as easily be run by Mix as by Rebar3. On the other hand, properly supporting Elixir would probably require a separate library wrapping Gradualizer anyway, so this might not be a biggie.

Just thinking out loud.

[zuiderkwast]

The parse transform is a really good solution!

If we add -spec_module(lists) (or maybe just -module(lists)?), we can also define types belonging to different modules, e.g. lists:deep_list/1 (as we've seen in #190).

Regarding Elixir, they want to remove Erlang parse transforms for Elixir code, I suppose, because they want to move away from Erlang AST and compile directly to Core, as José says. For Erlang code, that can't be the case. If Mix claims to be able to compile Erlang code, it will still handle Erlang parse transforms for Erlang code, since that is part of the Erlang language. Also, Gradualizer works with the Erlang AST so we will only be able handle Elixir code as long as it is compiled via the Erlang AST.

[gomoripeti]

• uh, that's a strong consequence that Elixir moving to direct core-erlang compilation means no more gradualizing Elixir :( good to be aware (I guess there is still some time before that)

• the parser has no problem with multiple -modules currently, but maybe its more future proof to use a non-reserved attribute. otoh we must use -spec because only spec and callback attribute support certain syntaxes like ->

[josefs]

Concerning the support for Elixir: I never set out to support Elixir when I started working on Gradualizer. The fact that it works now is a happy accident due to the fact that I chose to work on the absform format and Elixir compiles to that.
But even though I did not design Gradualizer with Elixir in mind I'm really happy that it (sort of) works right now, and it'd be awesome if we could continue to improve on the Elixir support and make it better. However, that requires someone to step up and take responsibility of the Elixir part of Gradualizer. It would be great to have a contributor whose sole mission was to work on the Elixir support.
Though, as @gomoripeti notes, if Elixir is moving to compiling direct to core-erlang then that is not promising for future Gradualizer support for Elixir.

[josevalim]

Regarding Elixir, they want to remove Erlang parse transforms for Elixir code, I suppose, because they want to move away from Erlang AST and compile directly to Core, as José says.

We removed parse transforms because we skip the Erlang Linting to make compilation about 5% faster (as we lint everything already) but we would have to lint again if we allowed parse_transforms as they can introduce invalid nodes. Also allowing Erlang parse transform is kind of leaking implementation details.

In any case, while we have discussed about moving to Core Erlang, we have no plans to do so exactly because we would lose things like cover, debugger, gradualizer, eval, so the plan is to continue to support Erlang Abstract Format. I don't see us moving away from it unless most of those tools are changed to work on Core Erlang (which may not happen at all).

Regarding how to run Gradualizer from Elixir, the simplest would probably be to have an API that receives a bunch of paths to BEAM files (or the BEAM binaries themselves) and you would analyze them returning all errors as tuples (so we can format those errors using Elixir syntax for terms and so on). That's how almost all dialyzer integrations work too. I hope this clarifies it a bit.

[josefs]

Thanks for providing up-to-date info, @josevalim!

Regarding how to run Gradualizer from Elixir, the simplest would probably be to have an API that receives a bunch of paths to BEAM files (or the BEAM binaries themselves) and you would analyze them returning all errors as tuples (so we can format those errors using Elixir syntax for terms and so on). That's how almost all dialyzer integrations work too.

As luck would have it, we have that kind of API already.

The problem with using Gradualizer with Elixir has more to do with the discrepancies between Elixir and Erlang. For instance, Gradualizer doesn't know anything about the Elixir string type.

[josevalim]

@josefs any type we have in Elixir is built on top of typespecs, so this part should just work. For example, we warn if someone uses string() to avoid confusion, because what Elixir calls a string is not the same a Erlang. But if you do use string(), it literally means Erlang's string(). Our actual string is String.t() but that's is declared as a type t in String which is simply binary().

[josefs]

@josevalim sounds good! See #73 for some of the efforts to get gradualizer to work with Elixir.

[josefs]

Getting back on topic here, I've verified that gradualizer works with OTP 22. All tests pass for me.

Good job, people!