Recover position information for undef/not_exported type errors #384

erszcz · 2022-02-15T20:09:15Z

This is another take at #379 which supersedes #380. The implementation is significantly simpler. It also covers the undef, user_type cases which were still not handled by #380.

Current master:

> gradualizer:type_check_file("test/misc/undefined_errors.erl", []).
test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type2/0 on line 0
test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type3/0 on line 0
test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type5/0 on line 0
test/misc/undefined_errors.erl: Undefined remote type undefined_errors_helper:not_exported_type/0 on line 0
test/misc/undefined_errors.erl: Undefined type undefined_type1/0 on line 0
test/misc/undefined_errors.erl: Undefined type undefined_type4/0 on line 0
test/misc/undefined_errors.erl: The atom on line 25 at column 41 is expected to have type #undefined_record1{} but it has type ok

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

test/misc/undefined_errors.erl: Call to undefined function undefined_errors_helper:undefined_call/0 on line 28 at column 41
test/misc/undefined_errors.erl: The function call on line 32 at column 20 is expected to have type #defined_record{} but it has type #undefined_record2{}

-record(defined_record, {a, b, c}).
-spec remote_record() -> #defined_record{}.
remote_record() -> undefined_errors_helper:und_rec().
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

nok
>
> gradualizer:type_check_file("priv/test/undefined_errors_helper.erl", []).
priv/test/undefined_errors_helper.erl: record undefined_record1 undefined on line 14 at column 61
priv/test/undefined_errors_helper.erl: record undefined_record2 undefined on line 16 at column 20
priv/test/undefined_errors_helper.erl: type undefined_type1() undefined on line 9 at column 14
priv/test/undefined_errors_helper.erl: type undefined_type4() undefined on line 13 at column 60
nok

This PR (some lines reordered for a simpler diff):

> gradualizer:type_check_file("test/misc/undefined_errors.erl", []).
test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type2/0 on line 15 at column 25
test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type3/0 on line 18 at column 37
test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type5/0 on line 34 at column 28
test/misc/undefined_errors.erl: Undefined remote type undefined_errors_helper:not_exported_type/0 on line 37 at column 19
test/misc/undefined_errors.erl: Undefined type undefined_type1/0 on line 12 at column 18
test/misc/undefined_errors.erl: Undefined type undefined_type4/0 on line 21 at column 36
test/misc/undefined_errors.erl: The atom on line 25 at column 41 is expected to have type #undefined_record1{} but it has type ok

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

test/misc/undefined_errors.erl: Call to undefined function undefined_errors_helper:undefined_call/0 on line 28 at column 41
test/misc/undefined_errors.erl: The function call on line 32 at column 20 is expected to have type #defined_record{} but it has type #undefined_record2{}

-record(defined_record, {a, b, c}).
-spec remote_record() -> #defined_record{}.
remote_record() -> undefined_errors_helper:und_rec().
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

nok
>
> gradualizer:type_check_file("priv/test/undefined_errors_helper.erl", []).
priv/test/undefined_errors_helper.erl: record undefined_record1 undefined on line 14 at column 61
priv/test/undefined_errors_helper.erl: record undefined_record2 undefined on line 16 at column 20
priv/test/undefined_errors_helper.erl: type undefined_type1() undefined on line 9 at column 14
priv/test/undefined_errors_helper.erl: type undefined_type4() undefined on line 13 at column 60
nok

Diff:

--- remote-types.before.log	2022-02-15 14:44:11.000000000 +0100
+++ remote-types.after.3.log	2022-02-15 21:04:15.000000000 +0100
@@ -1,10 +1,10 @@
 > gradualizer:type_check_file("test/misc/undefined_errors.erl", []).
-test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type2/0 on line 0
-test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type3/0 on line 0
-test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type5/0 on line 0
-test/misc/undefined_errors.erl: Undefined remote type undefined_errors_helper:not_exported_type/0 on line 0
-test/misc/undefined_errors.erl: Undefined type undefined_type1/0 on line 0
-test/misc/undefined_errors.erl: Undefined type undefined_type4/0 on line 0
+test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type2/0 on line 15 at column 25
+test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type3/0 on line 18 at column 37
+test/misc/undefined_errors.erl: Undefined remote type undefined_errors:undefined_type5/0 on line 34 at column 28
+test/misc/undefined_errors.erl: Undefined remote type undefined_errors_helper:not_exported_type/0 on line 37 at column 19
+test/misc/undefined_errors.erl: Undefined type undefined_type1/0 on line 12 at column 18
+test/misc/undefined_errors.erl: Undefined type undefined_type4/0 on line 21 at column 36
 test/misc/undefined_errors.erl: The atom on line 25 at column 41 is expected to have type #undefined_record1{} but it has type ok
 
 -spec remote_struct_with_record() -> undefined_errors_helper:expands_to_struct_with_undefined_record().

This catches undef/not_exported type errors on a level where position information is available from function specs and rethrows with this information supplied in the exception. In order to source this position information from specs it has to be stored in #env.fenv, so it is NOT removed from specs in `create_fenv()` anymore - this has to be done as required after fetching specs from #env.fenv.

erszcz · 2022-02-22T08:56:28Z

@zuiderkwast Do you have any remarks about this one?

src/typelib.erl

zuiderkwast · 2022-02-22T09:10:10Z

src/typechecker.erl

-      [ {{Name, NArgs}, lists:map(fun typelib:remove_pos/1,
-                                  absform:normalize_function_type_list(Types))}
+      [ {{Name, NArgs}, absform:normalize_function_type_list(Types)}


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?

This is used here:

https://github.com/erszcz/Gradualizer/blob/d78e28ea5213212f4518908d83ab26b409751d06/src/typechecker.erl#L775

https://github.com/erszcz/Gradualizer/blob/d78e28ea5213212f4518908d83ab26b409751d06/src/typechecker.erl#L783

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.

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.

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([]) ->

[].

zuiderkwast · 2022-02-22T10:08:24Z

I think this is a good approach in general. It's much better than the separate pass.

zuiderkwast · 2022-02-22T16:57:44Z

src/typechecker.erl

 get_bounded_fun_type_list(Name, Arity, Env, P) ->
    case maps:find({Name, Arity}, Env#env.fenv) of
-        {ok, Types} ->
-            typelib:remove_pos(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);


The name of this function is unfortunate, since it doesn't always return a list of bounded fun types. Sometimes it just returns the type any().

For functions without a spec, any() is set in create_fenv/2. We could replace this default type with a type on the form fun((any(), ...., any()) -> any()) of the correct arity. (This is already done in gradualizer_db:make_function_type/1 which could be moved/exported/reused.)

erszcz added 2 commits February 15, 2022 21:01

Test more undef/not_exported error cases

1250c87

erszcz mentioned this pull request Feb 15, 2022

Detect undefined remote types in a separate pass #380

Closed

2 tasks

Carry the currently checked fun's spec to avoid a catch and reraise

d78e28e

erszcz requested a review from zuiderkwast February 16, 2022 15:02

zuiderkwast reviewed Feb 22, 2022

View reviewed changes

src/typelib.erl Outdated Show resolved Hide resolved

zuiderkwast reviewed Feb 22, 2022

View reviewed changes

Don't proliferate treating type() as a list

11fe8ed

erszcz merged commit 2494099 into josefs:master Feb 22, 2022

erszcz deleted the issues/379-remote-type-position-handling-2 branch February 22, 2022 16:34

zuiderkwast reviewed Feb 22, 2022

View reviewed changes

erszcz linked an issue Feb 22, 2022 that may be closed by this pull request

remote_type position handling is incorrect #379

Closed

This was referenced Feb 22, 2022

remote_type position handling is incorrect #379

Closed

Use josefs/gradualizer @ c3ee078 esl/gradient#50

Merged

Unify single-clause and multiple-clause function representation #388

Open

erszcz mentioned this pull request Mar 8, 2022

Update Gradualizer to the current master erlang-ls/erlang_ls#1239

Merged

erszcz mentioned this pull request Mar 18, 2022

Infer non-any specs from pattern matches in function heads #394

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recover position information for undef/not_exported type errors #384

Recover position information for undef/not_exported type errors #384

erszcz commented Feb 15, 2022

erszcz commented Feb 22, 2022

zuiderkwast Feb 22, 2022

erszcz Feb 22, 2022

zuiderkwast Feb 22, 2022

erszcz Feb 22, 2022

zuiderkwast commented Feb 22, 2022

zuiderkwast Feb 22, 2022

	%% 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([]) ->
	[].

Recover position information for undef/not_exported type errors #384

Recover position information for undef/not_exported type errors #384

Conversation

erszcz commented Feb 15, 2022

erszcz commented Feb 22, 2022

zuiderkwast Feb 22, 2022

Choose a reason for hiding this comment

erszcz Feb 22, 2022

Choose a reason for hiding this comment

zuiderkwast Feb 22, 2022

Choose a reason for hiding this comment

erszcz Feb 22, 2022

Choose a reason for hiding this comment

zuiderkwast commented Feb 22, 2022

zuiderkwast Feb 22, 2022

Choose a reason for hiding this comment