Skip to content
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.

Sign up for GitHub

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

Detect undefined remote types in a separate pass #380

Closed
wants to merge 12 commits into from
Closed

Detect undefined remote types in a separate pass #380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4dfc3d6
Detect undefined remote types in a separate pass
erszcz Feb 8, 2022
d95fd9e
Actually remove_pos from remote_type's Mod and Name
erszcz Feb 9, 2022
c5c550d
Fix undefined_errors_test
erszcz Feb 9, 2022
c798f69
Test detecting a remote type which expands to an undefined remote type
erszcz Feb 9, 2022
29e59ab
Fix detection of a remote type which expands to an undefined remote type
erszcz Feb 9, 2022
0310bfd
Add some inline docs
erszcz Feb 9, 2022
cf0da85
Test remote types which expand to structures w/ undefined remote types
erszcz Feb 11, 2022
fa30aeb
Enumerate undefined types for easier interpretation of results
erszcz Feb 11, 2022
3295162
Test remote types which expand to structures w/ undefined local types
erszcz Feb 11, 2022
6835d36
Test remote types which expand to structures w/ undefined records
erszcz Feb 11, 2022
fc06c21
Fix remote_struct_with_local_type() and remote_struct_with_record() r…
erszcz Feb 15, 2022
81da18b
Store position info in gradualizer_db
erszcz Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions priv/test/undefined_errors_helper.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
-module(undefined_errors_helper).
-export([und_rec/0, und_ty/0, not_exp_ty/0]).
-export_type([j/0]).
-export_type([j/0,
expands_to_undefined_remote/0,
expands_to_struct_with_undefined_remote/0,
expands_to_struct_with_undefined_local/0,
expands_to_struct_with_undefined_record/0]).

-type j() :: undefined_type().
-type j() :: undefined_type1().
-type not_exported() :: ok.
-type expands_to_undefined_remote() :: undefined_errors:undefined_type2().
-type expands_to_struct_with_undefined_remote() :: {struct, undefined_errors:undefined_type3()}.
-type expands_to_struct_with_undefined_local() :: {struct, undefined_type4()}.
-type expands_to_struct_with_undefined_record() :: {struct, #undefined_record1{}}.

-spec und_rec() -> #undefined_record{}.
-spec und_rec() -> #undefined_record2{}.
und_rec() -> ok.

-spec und_ty() -> undefined_errors:undefined_type().
-spec und_ty() -> undefined_errors:undefined_type5().
und_ty() -> ok.

-spec not_exp_ty() -> not_exported().
Expand Down
83 changes: 49 additions & 34 deletions src/gradualizer_db.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@

%% API functions
-export([start_link/1,
get_spec/3,
get_type/3, get_exported_type/3, get_opaque_type/3,
get_spec/4,
get_type/4, get_exported_type/4, get_opaque_type/4,
get_record_type/2,
get_modules/0, get_types/1,
save/1, load/1,
Expand Down Expand Up @@ -54,32 +54,36 @@ start_link(Opts) ->
%% "module.erl"
-spec get_spec(M :: module(),
F :: atom(),
A :: arity()) -> {ok, [type()]} | not_found.
get_spec(M, F, A) ->
call({get_spec, M, F, A}).
A :: arity(),
Opts :: [remove_pos]) -> {ok, [type()]} | not_found.
get_spec(M, F, A, Opts) ->
call({get_spec, M, F, A, Opts}).

%% @doc Fetches an exported or unexported user-defined type. Does not expand
%% opaque types.
-spec get_type(Module :: module(),
Type :: atom(),
Params :: [type()]) -> {ok, type()} | opaque | not_found.
get_type(M, T, A) ->
call({get_type, M, T, A}).
Params :: [type()],
Opts :: [remove_pos]) -> {ok, type()} | opaque | not_found.
get_type(M, T, A, Opts) ->
call({get_type, M, T, A, Opts}).

%% @doc Fetches an exported type. Does not expand opaque types.
-spec get_exported_type(Module :: module(),
Type :: atom(),
Params :: [type()]) -> {ok, type()} | opaque |
not_exported | not_found.
get_exported_type(M, T, A) ->
call({get_exported_type, M, T, A}).
Params :: [type()],
Opts :: [remove_pos]) -> {ok, type()} | opaque |
not_exported | not_found.
get_exported_type(M, T, A, Opts) ->
call({get_exported_type, M, T, A, Opts}).

%% @doc Like get_type/3 but also expands opaque types.
-spec get_opaque_type(Module :: module(),
Type :: atom(),
Params :: [type()]) -> {ok, type()} | not_found.
get_opaque_type(M, T, A) ->
call({get_opaque_type, M, T, A}).
Params :: [type()],
Opts :: [remove_pos]) -> {ok, type()} | not_found.
get_opaque_type(M, T, A, Opts) ->
call({get_opaque_type, M, T, A, Opts}).

%% @doc Fetches a record type defined in the module.
-spec get_record_type(Module :: module(),
Expand Down Expand Up @@ -174,25 +178,27 @@ init(Opts0) ->
{ok, State2}.

-spec handle_call(any(), {pid(), term()}, state()) -> {reply, term(), state()}.
handle_call({get_spec, M, F, A}, _From, State) ->
handle_call({get_spec, M, F, A, Opts}, _From, State) ->
RemovePos = proplists:get_bool(remove_pos, Opts),
State1 = autoimport(M, State),
K = {M, F, A},
case State1#state.specs of
#{K := Types} ->
Types1 = [typelib:annotate_user_types(M, Type) || Type <- Types],
Types1 = [ typelib:annotate_user_types(M, remove_pos(RemovePos, Type))
|| Type <- Types ],
{reply, {ok, Types1}, State1};
_NoMatch ->
{reply, not_found, State1}
end;
handle_call({get_exported_type, M, T, Args}, _From, State) ->
handle_call({get_exported_type, M, T, Args, Opts}, _From, State) ->
State1 = autoimport(M, State),
handle_get_type(M, T, Args, true, false, State1);
handle_call({get_type, M, T, Args}, _From, State) ->
handle_get_type(M, T, Args, true, false, Opts, State1);
handle_call({get_type, M, T, Args, Opts}, _From, State) ->
State1 = autoimport(M, State),
handle_get_type(M, T, Args, false, false, State1);
handle_call({get_opaque_type, M, T, Args}, _From, State) ->
handle_get_type(M, T, Args, false, false, Opts, State1);
handle_call({get_opaque_type, M, T, Args, Opts}, _From, State) ->
State1 = autoimport(M, State),
handle_get_type(M, T, Args, false, true, State1);
handle_get_type(M, T, Args, false, true, Opts, State1);
handle_call({get_record_type, M, Name}, _From, State) ->
State1 = autoimport(M, State),
K = {M, Name},
Expand Down Expand Up @@ -327,10 +333,17 @@ call(Request, Timeout) ->
gen_server:call(?name, Request, Timeout).

%% helper for handle_call for get_type, get_exported_type, get_opaque_type.
-spec handle_get_type(module(), Name :: atom(), Params :: [type()],
RequireExported :: boolean(), ExpandOpaque :: boolean(),
state()) -> {reply, {ok, type()} | atom(), state()}.
handle_get_type(M, T, Args, RequireExported, ExpandOpaque, State) ->
-spec handle_get_type(M, T, Args, RequireExported, ExpandOpaque, Opts, State) -> R when
M :: module(),
T :: atom(),
Args :: [type()],
RequireExported :: boolean(),
ExpandOpaque :: boolean(),
Opts :: [remove_pos],
State :: state(),
R :: {reply, {ok, type()} | atom(), state()}.
handle_get_type(M, T, Args, RequireExported, ExpandOpaque, Opts, State) ->
RemovePos = proplists:get_bool(remove_pos, Opts),
K = {M, T, length(Args)},
case State#state.types of
#{K := TypeInfo} ->
Expand All @@ -342,7 +355,7 @@ handle_get_type(M, T, Args, RequireExported, ExpandOpaque, State) ->
#typeinfo{params = Vars,
body = Type0} ->
VarMap = maps:from_list(lists:zip(Vars, Args)),
Type1 = typelib:annotate_user_types(M, Type0),
Type1 = typelib:annotate_user_types(M, remove_pos(RemovePos, Type0)),
Type2 = typelib:substitute_type_vars(Type1, VarMap),
{reply, {ok, Type2}, State}
end;
Expand Down Expand Up @@ -491,7 +504,7 @@ collect_types(Module, Forms) ->
Info = #typeinfo{exported = Exported,
opaque = (Attr == opaque),
params = Params,
body = typelib:remove_pos(Body)},
body = Body},
{Id, Info}
end || Form = {attribute, _, Attr, {Name, Body, Vars}} <- Forms,
Attr == type orelse Attr == opaque,
Expand Down Expand Up @@ -523,9 +536,8 @@ extract_record_defs([{attribute, L, record, {Name, _UntypedFields}},
%% This representation is only used in OTP < 19
extract_record_defs([{attribute, L, record, {Name, Fields}} | Rest]);
extract_record_defs([{attribute, _L, record, {Name, Fields}} | Rest]) ->
TypedFields = [gradualizer_lib:remove_pos_typed_record_field(
absform:normalize_record_field(Field))
|| Field <- Fields],
TypedFields = [ absform:normalize_record_field(Field)
|| Field <- Fields ],
R = {Name, TypedFields},
[R | extract_record_defs(Rest)];
extract_record_defs([_ | Rest]) ->
Expand Down Expand Up @@ -562,8 +574,7 @@ collect_specs(Module, Forms) ->
{F, A} <- Exports,
not sets:is_element({F, A},
SpecedFunsSet)],
[{Key, lists:map(fun typelib:remove_pos/1,
absform:normalize_function_type_list(Types))}
[{Key, absform:normalize_function_type_list(Types)}
|| {Key, Types} <- Specs ++ ImplicitSpecs].

normalize_spec({{Func, Arity}, Types}, Module) ->
Expand Down Expand Up @@ -623,3 +634,7 @@ beam_file_regexp() ->
erl_file_regexp() ->
{ok, RE} = re:compile(<<"([^/.]*)\.erl$">>),
RE.

-spec remove_pos(boolean(), type()) -> type().
remove_pos(true, Type) -> typelib:remove_pos(Type);
remove_pos( _, Type) -> Type.
9 changes: 5 additions & 4 deletions src/gradualizer_lib.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,15 +102,16 @@ reverse_graph(G) ->
-spec get_type_definition(UserTy, Env, Opts) -> {ok, Ty} | opaque | not_found when
UserTy :: gradualizer_type:abstract_type(),
Env :: typechecker:env(),
Opts :: [annotate_user_types],
Opts :: [Opt],
Opt :: annotate_user_types | remove_pos,
Ty :: gradualizer_type:abstract_type().
get_type_definition({remote_type, _Anno, [{atom, _, Module}, {atom, _, Name}, Args]}, _Env, _Opts) ->
gradualizer_db:get_type(Module, Name, Args);
get_type_definition({remote_type, _Anno, [{atom, _, Module}, {atom, _, Name}, Args]}, _Env, Opts) ->
gradualizer_db:get_type(Module, Name, Args, Opts);
get_type_definition({user_type, Anno, Name, Args}, Env, Opts) ->
%% Let's check if the type is a known remote type.
case typelib:get_module_from_annotation(Anno) of
{ok, Module} ->
gradualizer_db:get_type(Module, Name, Args);
gradualizer_db:get_type(Module, Name, Args, Opts);
none ->
%% Let's check if the type is defined in the context of this module.
case maps:get({Name, length(Args)}, maps:get(types, Env#env.tenv), not_found) of
Expand Down
86 changes: 78 additions & 8 deletions src/typechecker.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,7 @@ normalize_rec({user_type, P, Name, Args} = Type, Env, Unfolded) ->
{type, NormType} -> NormType;
no_type ->
UnfoldedNew = maps:put(mta(Type, Env), {type, Type}, Unfolded),
case gradualizer_lib:get_type_definition(Type, Env, []) of
case gradualizer_lib:get_type_definition(Type, Env, [remove_pos]) of
{ok, T} ->
normalize_rec(T, Env, UnfoldedNew);
opaque ->
Expand All @@ -779,15 +779,17 @@ normalize_rec(T = ?top(), _Env, _Unfolded) ->
%% Don't normalize gradualizer:top().
T;
normalize_rec({remote_type, P, [{atom, _, M}, {atom, _, N}, Args]}, Env, Unfolded) ->
case gradualizer_db:get_exported_type(M, N, Args) of
case gradualizer_db:get_exported_type(M, N, Args, [remove_pos]) of
{ok, T} ->
normalize_rec(T, Env, Unfolded);
opaque ->
NormalizedArgs = lists:map(fun (Ty) -> normalize_rec(Ty, Env, Unfolded) end, Args),
typelib:annotate_user_types(M, {user_type, P, N, NormalizedArgs});
not_exported ->
%% See check_undefined_types/1 for handling of these cases.
throw({not_exported, remote_type, P, {M, N, length(Args)}});
not_found ->
%% See check_undefined_types/1 for handling of these cases.
throw({undef, remote_type, P, {M, N, length(Args)}})
end;
normalize_rec({op, _, _, _Arg} = Op, _Env, _Unfolded) ->
Expand Down Expand Up @@ -1722,7 +1724,7 @@ do_type_check_expr(Env, {'fun', P, {function, Name, Arity}}) ->
do_type_check_expr(Env, {'fun', P, {function, M, F, A}}) ->
case {get_atom(Env, M), get_atom(Env, F), A} of
{{atom, _, Module}, {atom, _, Function}, {integer, _, Arity}} ->
case gradualizer_db:get_spec(Module, Function, Arity) of
case gradualizer_db:get_spec(Module, Function, Arity, [remove_pos]) of
{ok, BoundedFunTypeList} ->
Ty = bounded_type_list_to_type(Env, BoundedFunTypeList),
{Ty, #{}, constraints:empty()};
Expand Down Expand Up @@ -2524,7 +2526,7 @@ do_type_check_expr_in(Env, ResTy, Expr = {'fun', P, {function, Name, Arity}}) ->
do_type_check_expr_in(Env, ResTy, Expr = {'fun', P, {function, M, F, A}}) ->
case {get_atom(Env, M), get_atom(Env, F), A} of
{{atom, _, Module}, {atom, _, Function}, {integer, _,Arity}} ->
case gradualizer_db:get_spec(Module, Function, Arity) of
case gradualizer_db:get_spec(Module, Function, Arity, [remove_pos]) of
{ok, BoundedFunTypeList} ->
FunTypeList =
unfold_bounded_type_list(Env, BoundedFunTypeList),
Expand Down Expand Up @@ -3088,7 +3090,7 @@ type_check_fun(Env, {atom, P, Name}, Arity) ->
{Types, #{}, constraints:empty()};
type_check_fun(_Env, {remote, P, {atom,_,Module}, {atom,_,Fun}}, Arity) ->
% Module:function call
case gradualizer_db:get_spec(Module, Fun, Arity) of
case gradualizer_db:get_spec(Module, Fun, Arity, [remove_pos]) of
{ok, Types} -> {Types, #{}, constraints:empty()};
not_found -> throw({call_undef, P, Module, Fun, Arity})
end;
Expand Down Expand Up @@ -3264,7 +3266,7 @@ get_bounded_fun_type_list(Name, Arity, Env, P) ->
error ->
case erl_internal:bif(Name, Arity) of
true ->
{ok, Types} = gradualizer_db:get_spec(erlang, Name, Arity),
{ok, Types} = gradualizer_db:get_spec(erlang, Name, Arity, [remove_pos]),
Types;
false ->
%% If it's not imported, the file doesn't compile.
Expand All @@ -3278,7 +3280,7 @@ get_bounded_fun_type_list(Name, Arity, Env, P) ->
get_imported_bounded_fun_type_list(Name, Arity, Env, P) ->
case maps:find({Name, Arity}, Env#env.imported) of
{ok, Module} ->
case gradualizer_db:get_spec(Module, Name, Arity) of
case gradualizer_db:get_spec(Module, Name, Arity, [remove_pos]) of
{ok, BoundedFunTypeList} ->
{ok, BoundedFunTypeList};
not_found ->
Expand Down Expand Up @@ -4740,7 +4742,8 @@ type_check_forms(Forms, Opts) ->
collect_specs_types_opaques_and_functions(Forms),
Env = create_env(ParseData, Opts),
?verbose(Env, "Checking module ~p~n", [ParseData#parsedata.module]),
AllErrors =
UndefinedTypeErrors = check_undefined_types(Forms),
TypeCheckErrors =
lists:foldr(
fun (Function, Errors) when Errors =:= [];
not StopOnFirstError ->
Expand Down Expand Up @@ -4772,6 +4775,10 @@ type_check_forms(Forms, Opts) ->
(_Function, Errors) ->
Errors
end, [], ParseData#parsedata.functions),
%% `TypeCheckErrors' will contain undefined remote type errors missing position information.
%% We don't want to report them, as they're not useful to the user - we report
%% `UndefinedTypeErrors' instead.
AllErrors = filter_undefined_type_errors(Env, TypeCheckErrors) ++ UndefinedTypeErrors,
lists:reverse(AllErrors).

-spec create_env(#parsedata{}, proplists:proplist()) -> env().
Expand Down Expand Up @@ -4812,6 +4819,69 @@ create_fenv(Specs, Funs) ->
]
).

%% @doc Check for undefined remote types in a separate pass over `Forms'.
%%
%% The main typechecker pass defined in `type_check_forms/2' removes position information from types
%% as soon as they're encountered. It's done to make types occuring on different source
%% lines represented the same, therefore easy to compare.
%%
%% However, in case of references to undefined types (that is, programmer mistakes) it means that
%% an error which ought to carry this position information to point the user at the source line
%% to fix is missing.
%%
%% This pass over the AST does not remove position information, so invalid remote type references
%% can be reported properly.
%%
%% See https://github.com/josefs/Gradualizer/issues/379 for more information.
%% @end
-spec check_undefined_types(Forms) -> Errors when
Forms :: gradualizer_file_utils:abstract_forms(),
Errors :: [ Error ],
Error :: {not_exported, remote_type, erl_anno:anno(), {module(), atom(), non_neg_integer()}}
| {undef, remote_type, erl_anno:anno(), {module(), atom(), non_neg_integer()}}.
check_undefined_types(Forms) ->
gradualizer_lib:fold_ast(fun check_remote_type/2, [], Forms).
Comment on lines +4842 to +4843
Copy link
Collaborator

@zuiderkwast zuiderkwast Feb 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but I can't see that we handle the following:

  • remote types which expand to structures containing user types or records which are undefined
  • remote types which expand to structures containing more remote types Handled
  • user types and records belong to a remote module (i.e. they come from expanding a remote type) which refer to other remote types, user types and records
  • cycles of the above

Some of it could be solved if gradualizer_db would perform linting when parsing a module, but I don't think it does (yet). (Btw, that's another topic: Unify the gradualizer_db and typechecker code to parse and extracts stuff from a parse tree.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the difference between:

remote types which expand to structures containing user types or records which are undefined

and

user types and records belong to a remote module (i.e. they come from expanding a remote type) which refer to other remote types, user types and records

be? Could you give an example?

I've added a number of tests - could you confirm if they capture what you had in mind above? Apparently, there was no need to change the check itself.

Copy link
Collaborator

@zuiderkwast zuiderkwast Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remote types which expand to structures containing user types or records which are undefined

-module(foo).
-export_type([t/0]).
-type t() :: {undef(), #undef{}}.

user types and records belong to a remote module (i.e. they come from expanding a remote type) which refer to other remote types, user types and records

-module(bar).
-export_type([t/0]).
-type t() :: [w() | #r{}].
-type w() :: {foo:undef()}.
-record(r, {f :: foo:undef()}).

If these are covered by tests, then I suspect the line and column are either zero (because I can't see that they're covered by the new pass, but only by the regular typechecking pass) or if they're non-zero, maybe line and column refer to the location in foo or bar where the error is rather than the module being type checked. We can probably report the line and column where foo:t() or bar:t() is used in the module being checked rather than where the error is in the foo and bar modules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right, this work so far only improved one particular situation when the reference was to a remote type which was not exported. All the other reports were still based on type representation fetched from gradualizer_db where position information was already lost, so the reported lines were 0.


check_remote_type({attribute, _, SpecType, {_, Forms}}, Acc)
when SpecType =:= spec;
SpecType =:= type ->
check_undefined_types(Forms) ++ Acc;
check_remote_type(?top(), Acc) ->
Acc;
%% Check remote calls, as the remote function specs can also refer to undefined remote types.
check_remote_type({call, _, {remote, _, {atom, _, Module}, {atom, _, Fun}}, Args}, Acc) ->
RemovePosOff = [],
case gradualizer_db:get_spec(Module, Fun, length(Args), RemovePosOff) of
{ok, Types} ->
check_undefined_types(Types) ++ Acc;
not_found ->
Acc
end;
check_remote_type({remote_type, P, [{atom, _, M}, {atom, _, N}, Args]}, Acc) ->
RemovePosOff = [],
case gradualizer_db:get_exported_type(M, N, Args, RemovePosOff) of
{ok, Type} ->
%% A remote type might expand to another remote type, so let's check that, too.
check_undefined_types([Type]) ++ Acc;
opaque ->
Acc;
not_exported ->
[{not_exported, remote_type, P, {M, N, length(Args)}} | Acc];
not_found ->
[{undef, remote_type, P, {M, N, length(Args)}} | Acc]
end;
check_remote_type(_Form, Acc) ->
Acc.

filter_undefined_type_errors(Env, Errors) ->
{Discard, Keep} = lists:partition(fun
({not_exported, remote_type, _, _}) -> true;
({undef, remote_type, _, _}) -> true;
(_) -> false
end, Errors),
verbose(Env, "Discarding errors: ~p\n", [Discard]),
Keep.

%% Collect the top level parse tree stuff returned by epp:parse_file/2.
-spec collect_specs_types_opaques_and_functions(Forms :: list()) -> #parsedata{}.
collect_specs_types_opaques_and_functions(Forms) ->
Expand Down
4 changes: 2 additions & 2 deletions src/typelib.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ pp_type(Type) ->

%% Looks up and prints the type M:N(P1, ..., Pn).
debug_type(M, N, P) ->
case gradualizer_db:get_type(M, N, P) of
case gradualizer_db:get_type(M, N, P, _RemovePosOff = []) of
{ok, T} ->
Params = lists:join($,, lists:map(fun pp_type/1, P)),
io:format("~w:~w(~s) :: ~s.~n",
Expand Down Expand Up @@ -118,7 +118,7 @@ remove_pos({type, _, Assoc, Tys}) when Assoc == map_field_exact;
{type, erl_anno:new(0), Assoc, lists:map(fun remove_pos/1, Tys)};
remove_pos({remote_type, _, [Mod, Name, Params]}) ->
Params1 = lists:map(fun remove_pos/1, Params),
{remote_type, erl_anno:new(0), [Mod, Name, Params1]};
{remote_type, erl_anno:new(0), [remove_pos(Mod), remove_pos(Name), Params1]};
remove_pos({ann_type, _, [_Var, Type]}) ->
%% Also remove annotated types one the form Name :: Type
remove_pos(Type);
Expand Down
Loading