-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fix yet more self check errors #504
Conversation
This fixes: ebin/gradualizer_prelude.beam: The tuple on line 0 is expected to have type erl_parse:abstract_form() | erl_parse:form_info() but it has type {attribute, 1, file, {[$...$v, ...], 1}} get_modules_and_forms() -> [{erlang, [{attribute, ^^^^^^^^^^^ 1, ^^ file, ^^^^^ {"src/../priv/prelude/erlang.specs.erl", 1}}, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This error stems from `erl_anno:anno()` being an opaque type. It means that literal 1 is not treated as an `erl_anno:anno()` instance, as remote opaque types cannot by compared by structural comparison. Instead, we have to create opaque type instances explicitly by calling constructors, i.e. functions that return their instances. That's what we're doing in this commit.
6afd817
to
7865417
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
erl_anno_mark(Form) -> | ||
Anno = element(2, Form), | ||
setelement(2, Form, {'$anno', Anno}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this? I don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is that the annotation is just an integer or a pair of integers. After transforming into the erl_syntax
representation it would be very hard to tell in erl_syntax_lib:map()
mapper which integer (or which pair) that we see in the tree is an annotation and which is not. By leaving a marker like this one, we can then quite safely match on a tuple like {tree, tuple, _, [{atom, _, '$anno'}, Anno]}
and finally substitute it with what we ultimately need here - an erl_anno:new()
call. Why do we need that call? See the commit message on 9a5bd85.
src/gradualizer_file_utils.erl
Outdated
epp:open([{name, File}, | ||
{location, StartLocation}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, this doesn't give us column numbers in OTP < 24.
Btw, code that depends on OTP version by erlang:function_exported/3
is an interesting and valid use case. It's the dynamic nature of dynamic loading. If we work around it here, perhaps we can add it to known problems instead?
It can't really be solved, but we can come up with some recommended way to silence it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can and I solved it in another way, but then decided it's better to remove code than to add more. I'll push the alternative solution instead, since you mention this.
BTW, I don't know why, but dropping this leads to the CI failure on 23.3.4.7 - is that connected with the missing column numbers? The other solution I have in mind passed CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe it has to do with column numbers. Some OTPs ago, there were no column numbers and then it was added gradually in different modules. (Our Peter Gömöri was involved in adding column numbers in OTP.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed the alternative approach. It's based on --specs_override_dir
and providing specs of all the functions we expect to use in various OTP versions. WDYT?
According to @zuiderkwast dropping epp:open/5 leads to column info missing in pre-OTP-24 Erlang. To sidestep this, and also to show how erlang:function_exported/3 can be used in a project relying on Gradualizer, we can solve the problem by providing specs of the functions which are present in different versions of OTP.
1f93210
to
92bd91f
Compare
Also remove unneccessary type assertions. Not sure why they're not necessary anymore, but bisecting the changes shows it's connected with this commit.
…_pass.erl This is based on the problem from src/gradualizer_lib.erl: get_type_definition({user_type, Anno, Name, Args}, Env, Opts) -> ... case maps:get({Name, length(Args)}, maps:get(types, Env#env.tenv), not_found) of not_found -> not_found; {Params, Type0} -> VarMap = maps:from_list(lists:zip(Params, Args)), Type2 = case proplists:is_defined(annotate_user_types, Opts) of true -> Module = maps:get(module, Env#env.tenv), Type1 = typelib:annotate_user_type(Module, Type0), typelib:substitute_type_vars(Type1, VarMap); false -> typelib:substitute_type_vars(Type0, VarMap) end, {ok, Type2} end ... Which results in: src/gradualizer_lib.erl: Lower bound not_found of type variable Default_typechecker_3529_43 on line 113 is not a subtype of {Default_typechecker_3529_43_typechecker_1257_51, Default_typechecker_3529_43_typechecker_1257_52}
3382b89
to
9fbb559
Compare
This is based on a problem in gradualizer_db:get_beam_map/0.
This function's spec and the structure of gradualizer_type:abstract_type() are still too hard for Gradualizer to check properly. See branch erszcz/typelib-remove-pos-checking-issues for details. However, it's tested extensively on each Gradualizer run, since type info is removed when types are loaded into gradualizer_db. Using spec (any()) -> any() makes the self-check error go away.
9fbb559
to
1324bf3
Compare
20 lines as of now and some new known problems. |
23 lines of self-check errors at this point. It has never been this low.