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

Recover position information for undef/not_exported type errors #384

Merged
merged 4 commits into from
Feb 22, 2022
Merged

Recover position information for undef/not_exported type errors #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ba80a9
Recover position information for undef/not_exported type errors
erszcz Feb 15, 2022
1250c87
Test more undef/not_exported error cases
erszcz Feb 15, 2022
d78e28e
Carry the currently checked fun's spec to avoid a catch and reraise
erszcz Feb 15, 2022
11fe8ed
Don't proliferate treating type() as a list
erszcz Feb 22, 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
45 changes: 36 additions & 9 deletions src/typechecker.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@
-type expr() :: gradualizer_type:abstract_expr().
-type type() :: gradualizer_type:abstract_type().

-type form() :: erl_parse:abstract_form().
-type forms() :: gradualizer_file_utils:abstract_forms().

%% Pattern macros
-define(type(T), {type, _, T, []}).
-define(type(T, A), {type, _, T, A}).
Expand Down Expand Up @@ -761,7 +764,7 @@ normalize_rec({type, _, union, Tys} = Type, Env, Unfolded) ->
Ts -> type(union, Ts)
end
end;
normalize_rec({user_type, P, Name, Args} = Type, Env, Unfolded) ->
normalize_rec({user_type, _, Name, Args} = Type, Env, Unfolded) ->
case maps:get(mta(Type, Env), Unfolded, no_type) of
{type, NormType} -> NormType;
no_type ->
Expand All @@ -772,19 +775,21 @@ normalize_rec({user_type, P, Name, Args} = Type, Env, Unfolded) ->
opaque ->
Type;
not_found ->
P = position_info_from_spec(Env#env.current_spec),
throw({undef, user_type, P, {Name, length(Args)}})
end
end;
normalize_rec(T = ?top(), _Env, _Unfolded) ->
%% Don't normalize gradualizer:top().
T;
normalize_rec({remote_type, P, [{atom, _, M}, {atom, _, N}, Args]}, Env, Unfolded) ->
normalize_rec({remote_type, _, [{atom, _, M}, {atom, _, N}, Args]}, Env, Unfolded) ->
P = position_info_from_spec(Env#env.current_spec),
case gradualizer_db:get_exported_type(M, N, Args) 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});
typelib:annotate_user_types(M, {user_type, 0, N, NormalizedArgs});
not_exported ->
throw({not_exported, remote_type, P, {M, N, length(Args)}});
not_found ->
Expand Down Expand Up @@ -3259,8 +3264,11 @@ type_check_cons_union(Env, [_ | Tys], H, T) ->

get_bounded_fun_type_list(Name, Arity, Env, P) ->
case maps:find({Name, Arity}, Env#env.fenv) of
{ok, Types} ->
Types;
%% TODO: https://github.com/josefs/Gradualizer/issues/388
{ok, Types} when is_list(Types) ->
[ typelib:remove_pos(Ty) || Ty <- Types ];
{ok, Type} ->
typelib:remove_pos(Type);
error ->
case erl_internal:bif(Name, Arity) of
true ->
Expand Down Expand Up @@ -3976,15 +3984,35 @@ check_guards(Env, Guards) ->
VB = union_var_binds_symmetrical(VarBinds, Env),
VB.

type_check_function(Env, {function,_, Name, NArgs, Clauses}) ->
type_check_function(Env, {function, _, Name, NArgs, Clauses}) ->
?verbose(Env, "Checking function ~p/~p~n", [Name, NArgs]),
case maps:find({Name, NArgs}, Env#env.fenv) of
{ok, FunTy} ->
check_clauses_fun(Env, expect_fun_type(Env, FunTy), Clauses);
NewEnv = Env#env{current_spec = FunTy},
%% TODO: https://github.com/josefs/Gradualizer/issues/388
FunTyNoPos = case FunTy of
_ when is_list(FunTy) ->
[ typelib:remove_pos(Ty) || Ty <- FunTy ];
_ ->
typelib:remove_pos(FunTy)
end,
check_clauses_fun(NewEnv, expect_fun_type(NewEnv, FunTyNoPos), Clauses);
error ->
throw({internal_error, missing_type_spec, Name, NArgs})
end.

-spec position_info_from_spec(form() | forms() | none) -> erl_anno:anno().
position_info_from_spec(none) ->
%% This simplifies testing internal functions.
%% In these cases we don't go through type_check_function,
%% but call deeper into the typechecker directly.
erl_anno:new(0);
position_info_from_spec([_|_] = Forms) ->
%% TODO: https://github.com/josefs/Gradualizer/issues/388
position_info_from_spec(hd(Forms));
position_info_from_spec(Form) ->
element(2, Form).

-spec add_types_pats(Pats :: [gradualizer_type:abstract_pattern()],
Tys :: [type()],
Env :: env(),
Expand Down Expand Up @@ -4806,8 +4834,7 @@ create_fenv(Specs, Funs) ->
[ {{Name, NArgs}, type(any)}
|| {function,_, Name, NArgs, _Clauses} <- Funs
] ++
[ {{Name, NArgs}, lists:map(fun typelib:remove_pos/1,
absform:normalize_function_type_list(Types))}
[ {{Name, NArgs}, absform:normalize_function_type_list(Types)}
Comment on lines -4809 to +4837
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we keep the positions in fenv but we remove them in tenv. Don't we need them in tenv too for the same reasons?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is used here:

And set here:
https://github.com/erszcz/Gradualizer/blob/d78e28ea5213212f4518908d83ab26b409751d06/src/typechecker.erl#L3985

Which means that we use position information from the currently checked function spec when it's missing on a type to be thrown with an error. This means we throw exactly the location where the error occurs. I don't see the need to keep position information in tenv.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, we only check functions, not types. Good point. It makes sense.

I wonder where we normalize records from other modules. I can't find it. I mean normalize_rec({type, P, record, [{atom, _, Name} | Fields]}, ... where P has a filename in it, meaning it's a record in another module which comes from an expanded remote type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder where we normalize records from other modules.

I think we can only access them through gradualizer_db and there's code like this there:

Gradualizer/src/gradualizer_db.erl

Lines 506 to 535 in 5be0f94

%% Normalize Type Defs
%% -------------------
%%
%% Extracts and normalizes type definitions from a list of forms.
%%
%% Normalise record definitions into types (i.e. typed record definitions).
%% That is, if there is no typed definition of a record among the
%% forms, create one from the untyped one and normalize so that they
%% all have a default value.
%%
%% Location in field names and types are set to zero to allow comparison using
%% equality and pattern matching. This is not done for the default value (which
%% is an expression, not a type).
-spec extract_record_defs(Forms :: [tuple()]) -> Typedefs :: [{atom(), [type()]}].
extract_record_defs([{attribute, L, record, {Name, _UntypedFields}},
{attribute, L, type, {{record, Name}, Fields, []}} |
Rest]) ->
%% 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],
R = {Name, TypedFields},
[R | extract_record_defs(Rest)];
extract_record_defs([_ | Rest]) ->
%% Skip forms that are not record definitions
extract_record_defs(Rest);
extract_record_defs([]) ->
[].

|| {{Name, NArgs}, Types} <- Specs
]
).
Expand Down
3 changes: 2 additions & 1 deletion src/typechecker.hrl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
verbose = false :: boolean(),
exhaust = true :: boolean(),
%% Performance hack: Unions larger than this limit are replaced by any() in normalization.
union_size_limit :: non_neg_integer()
union_size_limit :: non_neg_integer(),
current_spec = none :: erl_parse:abstract_form() | none
}).

-endif. %% __TYPECHECKER_HRL__
16 changes: 16 additions & 0 deletions test/misc/undefined_errors.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
-module(undefined_errors).
-export([remote_type/0,
remote_remote_type/0,
remote_struct_with_remote_type/0,
remote_struct_with_local_type/0,
remote_struct_with_record/0,
remote_call/0,
remote_record/0,
normalize_remote_type/0,
Expand All @@ -8,6 +12,18 @@
-spec remote_type() -> undefined_errors_helper:j().
remote_type() -> ok.

-spec remote_remote_type() -> undefined_errors_helper:expands_to_undefined_remote().
remote_remote_type() -> ok.

-spec remote_struct_with_remote_type() -> undefined_errors_helper:expands_to_struct_with_undefined_remote().
remote_struct_with_remote_type() -> {struct, ok}.

-spec remote_struct_with_local_type() -> undefined_errors_helper:expands_to_struct_with_undefined_local().
remote_struct_with_local_type() -> {struct, ok}.

-spec remote_struct_with_record() -> undefined_errors_helper:expands_to_struct_with_undefined_record().
remote_struct_with_record() -> {struct, ok}.

-spec remote_call() -> ok.
remote_call() -> undefined_errors_helper:undefined_call().

Expand Down