Fix handling of module_info for arities > 1 #500
Conversation
|
Good stuff, @xxdavid!
Indeed, we do. Check out: Gradualizer/src/typechecker.hrl Line 9 in 22a8e19 and Gradualizer/src/gradualizer_lib.erl Line 15 in 22a8e19 You can find
Yeah, this should work 👍 Defining and reusing the spec of |
|
@erszcz, thanks for pointing to the right direction. I've clearly overlook the |
|
I've taken a deep dive into the code and I think the solution I proposed would work but it is not ideal. |
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 I think we could try importing them, but this requires checking for how remote / user types loaded into the type checker from |
Aha, I wasn't aware of that. Thanks for pointing this out.
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 ( |
Ahh, bad luck. Thanks for taking the time to check it, though!
I think this is the right approach, but we don't have to do it in this PR.
Would you mind moving the external self |
xxdavid
force-pushed
the
fix_module_info_for_greater_arities
branch
from
66a9798
to
1b14c48
Compare
|
Hi @erszcz and sorry for the delay.
Okay! I'll try to fix it in a follow-up PR so we can finally merge this.
Of course. :) I've force pushed to the branch. |
|
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? |
|
This looks good to me. I'm just curious about the calls to
I think it's correct to handle |
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. |
|
Hmm, I also think importing the module via The problem is that I don't know how to achieve that. If I do what I suggested
I get 16 test failures similar to these two: I don't get why the 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 |
|
@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. |
|
What if we add the directories of the file(s) to check in the same way? |
|
@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 |
|
@xxdavid With your patch applied, do you get the same result if you run: 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 |
|
Hi @erszcz, with the patch applied, it works well when I typecheck individual files, either from the shell or using CLI. The problem is only in the tests. I've spend some time investigating it and I still don't see An alternative solution/workaround to this could be fixing just the 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 |
|
If no one disagrees, I'll use the workaround I mentioned in the last comment, and I'll finally merge it. |
|
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 |
|
I've applied your patch, @xxdavid, on https://github.com/erszcz/Gradualizer/tree/fix_module_info_for_greater_arities-1 and have run We can see there, that the errors in tests come from What's the context of line 9 of 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 I don't think the patch proposed in #500 (comment) 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 I don't see any failures from files other than |
|
@erszcz, thanks for an exhaustive guide. 🙂 I haven't noticed that the error is in 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 |
@xxdavid check #516 to see how I would do it. It's not the cleanest approach ( |
xxdavid
force-pushed
the
fix_module_info_for_greater_arities
branch
from
bfbf1da
to
8eed829
Compare
Aha! That was easy. Thank you for pointing in the right direction. I've updated the pull request. |
This allows referencing a module (using fully qualified calls or types) from itself, without the need to add (via --pa) the directory where the file is located. It may be useful in tests as they don't add any path by default.
Co-authored-by: Radek Szymczyszyn <radoslaw.szymczyszyn@erlang-solutions.com>
xxdavid
force-pushed
the
fix_module_info_for_greater_arities
branch
from
8eed829
to
ebb322f
Compare
|
Awesome, thanks @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_infois the name of the current module):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:funindo_type_check_expranddo_type_check_expr_inand transform it to a local call tofun. Do we carry the current module somewhere (I don't see it in the Env)?