Fix handling of module_info for arities > 1

[xxdavid]

Follow-up of #496 based on a note by @zuiderkwast.

The only issue here (as you can see from the CI output) is that apparently Gradualizer does not work with function calls to the same module using the fully qualified name. The following code (module_info is the name of the current module):

-spec module_info(number(), number()) -> number().
module_info(A, B) -> A + B.

-spec two_plus_four() -> number().
two_plus_four() -> module_info:module_info(2, 4).

causes this error: Call to undefined function module_info:module_info/2 on line 29 at column 31.

Perhaps we can catch calls to current_mod:fun in do_type_check_expr and do_type_check_expr_in and transform it to a local call to fun. Do we carry the current module somewhere (I don't see it in the Env)?

[erszcz]

Good stuff, @xxdavid!

Do we carry the current module somewhere (I don't see it in the Env)?

Indeed, we do. Check out: https://github.com/josefs/Gradualizer/blob/22a8e19f1c88a00399eaeda41cf8ddde302061bd/src/typechecker.hrl#L9 and https://github.com/josefs/Gradualizer/blob/22a8e19f1c88a00399eaeda41cf8ddde302061bd/src/gradualizer_lib.erl#L15

You can find maps:get(module, Env#env.tenv) referring to that module name here and there.

Perhaps we can catch calls to current_mod:fun in do_type_check_expr and do_type_check_expr_in and transform it to a local call to fun.

Yeah, this should work 👍 Defining and reusing the spec of erlang:get_module_info/1,2 crossed my mind, too, but that function is not documented, whereas Module:module_info/0,1 are, so that's the way to go.

[xxdavid]

@erszcz, thanks for pointing to the right direction. I've clearly overlook the tenv field. I'll update the PR tomorrow.

[xxdavid]

I've taken a deep dive into the code and I think the solution I proposed would work but it is not ideal. I think the bug is rather in the fact that we don't import files into gradualizer_db when checking files passed via the CLI args. This could be fixed by calling gradualizer_db:import_erl_files([File])/gradualizer_db:import_beam_files([File]) in gradualizer:type_check_file/2. What do you think?

[erszcz]

[...] we don't import files into gradualizer_db when checking files passed via the CLI args [...]

You're probably aware of that, but to make it completely clear, we do import files passed in on the command line, but not the files to be checked. Files to be imported are determined by -pa. Files to be checked (.erl or .beam) are specified directly without flags.

I think we could try importing them, but this requires checking for how remote / user types loaded into the type checker from gradualizer_db are represented. In general, user types without a file annotation are assumed to be local. User types with a file annotation are assumed to be normalised remote types. If we fetch a type (as part of a function spec) via gradualizer_db it might end up with the annotation, yet actually come from the currently checked module. This might break things. I'm not 100% sure, though, that's why this needs checking. I think we could try and see if tests break or not.

[xxdavid]

You're probably aware of that, but to make it completely clear, we do import files passed in on the command line, but not the files to be checked. Files to be imported are determined by -pa. Files to be checked (.erl or .beam) are specified directly without flags.

Aha, I wasn't aware of that. Thanks for pointing this out.

I think we could try importing them, but this requires checking for how remote / user types loaded into the type checker from gradualizer_db are represented. In general, user types without a file annotation are assumed to be local. User types with a file annotation are assumed to be normalised remote types. If we fetch a type (as part of a function spec) via gradualizer_db it might end up with the annotation, yet actually come from the currently checked module. This might break things. I'm not 100% sure, though, that's why this needs checking. I think we could try and see if tests break or not.

It indeed does break tests. I'm not sure what to do then. Maybe it's better not to touch the import mechanism now, as this is rather a tiny issue (you usually don't reference a local function by its fully qualified name unless you use hot code upgrades) and it works when typechecking whole project. In this case I would just leave out the test part added in this PR (module_info/2, two_plus_four/0) and keep the rest of the PR intact. Is it okay?

[erszcz]

It indeed does break tests.

Ahh, bad luck. Thanks for taking the time to check it, though!

Perhaps we can catch calls to current_mod:fun in do_type_check_expr and do_type_check_expr_in and transform it to a local call to fun.

I think this is the right approach, but we don't have to do it in this PR.

I would just leave out the test part added in this PR (module_info/2, two_plus_four/0) and keep the rest of the PR intact. Is it okay?

Would you mind moving the external self module_info call test to known_problems/should_pass? That way we'll have this case documented, though not supported yet.

[xxdavid]

Hi @erszcz and sorry for the delay.

I think this is the right approach, but we don't have to do it in this PR.

Okay! I'll try to fix it in a follow-up PR so we can finally merge this.

Would you mind moving the external self module_info call test to known_problems/should_pass? That way we'll have this case documented, though not supported yet.

Of course. :) I've force pushed to the branch.

[xxdavid]

Hi! I was waiting for an approval to merge this but I should have been probably more explicit. 😄

In the meantime I also implemented the short-curcuit clauses for calls to the current module.

What do you think about it, @erszcz? Can I merge it?

[zuiderkwast]

This looks good to me.

I'm just curious about the calls to ?MODULE:Function/N. Why was it chosen to handle them like local function calls rather than importing the currently checked module(s) in gradualizer_db? I might have missed something in the discussion. I just saw this comment from @erszcz:

I think we could try importing them, but this requires checking for how remote / user types loaded into the type checker from gradualizer_db are represented. In general, user types without a file annotation are assumed to be local. User types with a file annotation are assumed to be normalised remote types. If we fetch a type (as part of a function spec) via gradualizer_db it might end up with the annotation, yet actually come from the currently checked module. This might break things. I'm not 100% sure, though, that's why this needs checking. I think we could try and see if tests break or not.

I think it's correct to handle ?MODULE:Function/N just like remote calls, i.e. only exported functions can be called, because that's what I think the beam does. In the same way, I suppose it's correct that only exported types are visible and opaque types are opaque. When we compare remote types with local types, they are resolved, unless they are opaque. Is this a problem in some situation (such as forcing code reload by calling in this way)?

[erszcz]

Why was it chosen to handle them like local function calls rather than importing the currently checked module(s) in gradualizer_db?

I understand it leads to broken tests, but I haven't looked into the details, so I don't know what the breakage is. I just relied on @xxdavid's report. It would be the most straightforward approach if it can be pulled off.

[xxdavid]

Hmm, I also think importing the module via gradualizer_db would be a better way, as it would be in line with the Erlang semantics of remote calls (only exported functions can be called remotely).

The problem is that I don't know how to achieve that. If I do what I suggested

[…] calling gradualizer_db:import_erl_files([File])/gradualizer_db:import_beam_files([File]) in gradualizer:type_check_file/2.

I get 16 test failures similar to these two:

=INFO REPORT==== 22-Feb-2023::18:28:40.701954 ===
    application: gradualizer
    exited: stopped
    type: temporary

=INFO REPORT==== 22-Feb-2023::18:28:41.026914 ===
    application: gradualizer
    exited: stopped
    type: temporary

=INFO REPORT==== 22-Feb-2023::18:28:41.794957 ===
    application: gradualizer
    exited: stopped
    type: temporary

=INFO REPORT==== 22-Feb-2023::18:28:42.044245 ===
    application: gradualizer
    exited: stopped
    type: temporary

gradualizer_tests:9: type_check_erl_file_test_...*failed*
in function gen_server:call/3 (gen_server.erl, line 247)
in call from gradualizer:type_check_file/2 (src/gradualizer.erl, line 171)
in call from gradualizer_tests:'-type_check_erl_file_test_/0-fun-1-'/0 (test/gradualizer_tests.erl, line 9)
in call from eunit_test:run_testfun/1 (eunit_test.erl, line 71)
in call from eunit_proc:run_test/1 (eunit_proc.erl, line 522)
in call from eunit_proc:with_timeout/3 (eunit_proc.erl, line 347)
in call from eunit_proc:handle_test/2 (eunit_proc.erl, line 505)
in call from eunit_proc:tests_inorder/3 (eunit_proc.erl, line 447)
**exit:{noproc,{gen_server,call,
                    [gradualizer_db,
                     {import_erl_files,["test/should_pass/any.erl"],[]},
                     infinity]}}
  output:<<"">>

gradualizer_tests:10: type_check_erl_file_test_...*failed*
in function gen_server:call/3 (gen_server.erl, line 247)
in call from gradualizer:type_check_file/2 (src/gradualizer.erl, line 171)
in call from gradualizer_tests:'-type_check_erl_file_test_/0-fun-3-'/0 (test/gradualizer_tests.erl, line 10)
in call from eunit_test:run_testfun/1 (eunit_test.erl, line 71)
in call from eunit_proc:run_test/1 (eunit_proc.erl, line 522)
in call from eunit_proc:with_timeout/3 (eunit_proc.erl, line 347)
in call from eunit_proc:handle_test/2 (eunit_proc.erl, line 505)
in call from eunit_proc:tests_inorder/3 (eunit_proc.erl, line 447)
**exit:{noproc,{gen_server,call,
                    [gradualizer_db,
                     {import_erl_files,["test/should_pass/any.erl"],[]},
                     infinity]}}
  output:<<"">>

...

I don't get why the gradualizer_db is down. Do you?

However, I think even better would be to import the file on demand as we do it with other modules when they are referenced. Here the problem is that gradualizer_db doesn't know where to find the file corresponding to the current module. It makes some guesses but cannot guess that the desired file is somewhere under the test folder. Maybe we could somehow tell gradualizer_db where it should search for the module M before type-checking every module M. But I don't know exactly how to achieve that.

[erszcz]

@xxdavid can you share the patch you're applying and getting these errors as a result? For example by pushing it as the top commit on this branch.

[zuiderkwast]

gradualizer_db uses code:get_path/0 to find source files and beam files.

gradualizer_cli uses code:add_pathsa/1 to add paths provided using -pa or --path_add so that gradualizer_db can find them.

What if we add the directories of the file(s) to check in the same way?

[xxdavid]

@erszcz It were just these two lines of code 😄

diff --git a/src/gradualizer.erl b/src/gradualizer.erl
index 4fb83ec..6eff3d2 100644
--- a/src/gradualizer.erl
+++ b/src/gradualizer.erl
@@ -168,6 +168,7 @@ type_check_file(File, Opts) ->
         false ->
             case filename:extension(File) of
                 ".erl" ->
+                    ok = gradualizer_db:import_erl_files([File]),
                     Includes = proplists:get_all_values(i, Opts),
                     case gradualizer_file_utils:get_forms_from_erl(File, Includes) of
                         {ok, Forms} ->
@@ -176,6 +177,7 @@ type_check_file(File, Opts) ->
                             throw(Error)
                     end;
                 ".beam" ->
+                    ok = gradualizer_db:import_beam_files([File]),
                     case gradualizer_file_utils:get_forms_from_beam(File) of
                         {ok, Forms} ->
                             type_check_forms(File, Forms, Opts);

@zuiderkwast Aha, thanks for the tip. This could be a way how to do it. But code:add_pathsa/1 can deal only with directories so we would have to add a (whole) directory for each file which would be a bit magical. Do you think it's okay?

[erszcz]

@xxdavid With your patch applied, do you get the same result if you run:

$ rebar3 shell
> gradualizer:type_check_file(...).

The thing is that you're not getting an error from the type checker that the type checking fails. The error seems to happen earlier, when loading the .erl file. That shouldn't be the case as the EScript should've started the Gradualizer application already, but apparently something is not working as expected. Based on the result in the Rebar shell we might be able to tell more.

[xxdavid]

Hi @erszcz,

with the patch applied, it works well when I typecheck individual files, either from the shell or using CLI.

~/Development/erlang/Gradualizer on  fix_module_info_for_greater_arities! ⌚ 10:31:16
$ bin/gradualizer test/should_pass/module_info_higher_arity.erl

~/Development/erlang/Gradualizer on  fix_module_info_for_greater_arities! ⌚ 10:31:18
$ bin/gradualizer test/should_pass/any.erl

~/Development/erlang/Gradualizer on  fix_module_info_for_greater_arities! ⌚ 10:32:09
$ rebar3 shell
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling gradualizer
Erlang/OTP 24 [erts-12.0.2] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:1] [jit]

Eshell V12.0.2  (abort with ^G)
1> ===> Booted gradualizer

1> gradualizer:type_check_file("test/should_pass/module_info_higher_arity.erl").
ok
2> gradualizer:type_check_file("test/should_pass/any.erl").
ok

The problem is only in the tests. I've spend some time investigating it and I still don't see gradualizer_db isn't running in the moment when gradualizer_db:import_erl_files([File]) is called.

An alternative solution/workaround to this could be fixing just the module_info_higher_arity.erl test in a way other tests like remote_types.erl are made to work – by importing the file manually inside test.erl:

diff --git a/test/test.erl b/test/test.erl
index 5f00fad..c84dafd 100644
--- a/test/test.erl
+++ b/test/test.erl
@@ -20,7 +20,9 @@ gen_should_pass() ->
              gradualizer_db:import_erl_files(["test/should_pass/user_types.erl"]),
              gradualizer_db:import_erl_files(["test/should_pass/other_module.erl"]),
              %% imported.erl references any.erl
-             gradualizer_db:import_erl_files(["test/should_pass/any.erl"])
+             gradualizer_db:import_erl_files(["test/should_pass/any.erl"]),
+             %% module_info_higher_arity.erl references itself
+             gradualizer_db:import_erl_files(["test/should_pass/module_info_higher_arity.erl"])
      end,
      map_erl_files(
        fun(File) ->

I would consider it appropriate as the current behaviour (not importing the current file automatically) causes an issue only in tests (that reference itself) or when you check files (that reference itself) from CLI without specifying the -pa parameter, and that IMHO isn't a typical use case.

[xxdavid]

If no one disagrees, I'll use the workaround I mentioned in the last comment, and I'll finally merge it.

[erszcz]

I think that if we apply

diff --git a/src/gradualizer.erl b/src/gradualizer.erl
index 4fb83ec..6eff3d2 100644
--- a/src/gradualizer.erl
+++ b/src/gradualizer.erl
@@ -168,6 +168,7 @@ type_check_file(File, Opts) ->
         false ->
             case filename:extension(File) of
                 ".erl" ->
+                    ok = gradualizer_db:import_erl_files([File]),
                     Includes = proplists:get_all_values(i, Opts),
                     case gradualizer_file_utils:get_forms_from_erl(File, Includes) of
                         {ok, Forms} ->
@@ -176,6 +177,7 @@ type_check_file(File, Opts) ->
                             throw(Error)
                     end;
                 ".beam" ->
+                    ok = gradualizer_db:import_beam_files([File]),
                     case gradualizer_file_utils:get_forms_from_beam(File) of
                         {ok, Forms} ->
                             type_check_forms(File, Forms, Opts);

but gradualizer_db is down in the test, then something is not going right. It should be up because of test:setup_app/0. Then, if it's up, gradualizer:type_check_file/1 called from the anon fun in gen_should_pass/0 should properly load the file in question.

[erszcz]

I've applied your patch, @xxdavid, on https://github.com/erszcz/Gradualizer/tree/fix_module_info_for_greater_arities-1 and have run make tests - the result is at https://gist.github.com/erszcz/fbd57d0372f452083b19f57378ab0bf7.

We can see there, that the errors in tests come from test/gradualizer_tests.erl:

gradualizer_tests:9: type_check_erl_file_test_...*failed*
in function gen_server:call/3 (gen_server.erl, line 247)
in call from gradualizer:type_check_file/2 (src/gradualizer.erl, line 171)
in call from gradualizer_tests:'-type_check_erl_file_test_/0-fun-1-'/0 (test/gradualizer_tests.erl, line 9)
in call from eunit_test:run_testfun/1 (eunit_test.erl, line 71)
in call from eunit_proc:run_test/1 (eunit_proc.erl, line 531)
in call from eunit_proc:with_timeout/3 (eunit_proc.erl, line 356)
in call from eunit_proc:handle_test/2 (eunit_proc.erl, line 514)
in call from eunit_proc:tests_inorder/3 (eunit_proc.erl, line 456)
**exit:{noproc,{gen_server,call,
                    [gradualizer_db,
                     {import_erl_files,["test/should_pass/any.erl"],[]},
                     infinity]}}
  output:<<"">>

What's the context of line 9 of gradualizer_tests?

      8 type_check_erl_file_test_() ->
      9     [?_assertEqual(ok, gradualizer:type_check_file(?passing)),
     10      ?_assertEqual([], gradualizer:type_check_file(?passing, [return_errors])),
     11      ?_assertEqual(nok, gradualizer:type_check_file(?failing)),
     12      ?_assertMatch([_|_], gradualizer:type_check_file(?failing, [return_errors]))
     13     ].

Ok, we're running tests, but unlike in test/test.erl there's no setup_app or cleanup_app called for test setup/teardown. If Gradualizer (the app) is not started, then gradualizer_db (the process) is not running, hence we get noproc.

I don't think the patch proposed in https://github.com/josefs/Gradualizer/pull/500#issuecomment-1449731811 would help with that ;) However, I think something like this will (it fixes 4 out of the 16 failing tests, so all the ones generated by type_check_erl_file_test_). We just have to fix the remaining test generators in this file so that all of them use a common setup/teardown. We can put them in a top-level test generator, similarly to how test/test.erl does it, and remove the test_ suffix so that EUnit doesn't pick them up twice.

I don't see any failures from files other than gradualizer_tests, so I don't expect a need for any other fixes.

[xxdavid]

@erszcz, thanks for an exhaustive guide. 🙂 I haven't noticed that the error is in gradualizer_tests and all the time searched for an error inside test.erl. I've never worked with eunit (as I am coming from Elixir) but I managed to adjust the test so that they are used in a common generator with the setup/teardown. But now what shall we do with not_found, bad_content or beam_without_forms? These are supposed to fail and the import fails as well.

The proposed workaround (#500 (comment)) wasn't meant to be used together with the previous patch, it was an alternative solution that would fix the module_info_higher_arity.erl test. But if we succeed in importing the currently checked file automatically, we can do it this way. 🙂

[erszcz]

But now what shall we do with not_found, bad_content or beam_without_forms? These are supposed to fail and the import fails as well.

@xxdavid check #516 to see how I would do it. It's not the cleanest approach (try ... after could be replaced with EUnit setup generator), but it's sufficient. The trick is to move the import deeper in gradualizer.

[xxdavid]

The trick is to move the import deeper in gradualizer.

Aha! That was easy. Thank you for pointing in the right direction.

I've updated the pull request.

[erszcz]

Awesome, thanks @xxdavid!