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.

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

Add crossref warning for non-exported function #1568

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions apps/els_lsp/priv/code_navigation/src/diagnostics_xref.erl
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,6 @@ existing() ->
dynamic_call(Foo, Bar) ->
Foo:bar(),
foo:Bar().

not_exported() ->
code_navigation:function_c().
2 changes: 2 additions & 0 deletions apps/els_lsp/src/els_code_action_provider.erl
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ make_code_actions(
{"function (.*) undefined", fun els_code_actions:suggest_function/4},
{"Cannot find definition for function (.*)", fun els_code_actions:suggest_function/4},
{"Cannot find module (.*)", fun els_code_actions:suggest_module/4},
{"Function (.+):(.+) is not exported.",
fun els_code_actions:export_external_function/4},
{"Unused file: (.*)", fun els_code_actions:remove_unused/4},
{"Atom typo\\? Did you mean: (.*)", fun els_code_actions:fix_atom_typo/4},
{"undefined callback function (.*) \\\(behaviour '(.*)'\\\)",
Expand Down
27 changes: 27 additions & 0 deletions apps/els_lsp/src/els_code_actions.erl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
extract_function/2,
create_function/4,
export_function/4,
export_external_function/4,
fix_module_name/4,
ignore_variable/4,
remove_macro/4,
Expand Down Expand Up @@ -84,6 +85,32 @@ export_function(Uri, _Range, _Data, [UnusedFun]) ->
]
end.

-spec export_external_function(uri(), range(), binary(), [binary()]) -> [map()].
export_external_function(_Uri, _Range, _Data, [ModBin, FABin]) ->
Mod = binary_to_atom(ModBin, utf8),
case els_utils:find_module(Mod) of
{ok, Uri} ->
{ok, Document} = els_utils:lookup_document(Uri),
case els_poi:sort(els_dt_document:pois(Document, [module, export])) of
[] ->
[];
POIs ->
#{range := #{to := {Line, _Col}}} = lists:last(POIs),
Pos = {Line + 1, 1},
[
make_edit_action(
Uri,
<<"Export ", FABin/binary>>,
?CODE_ACTION_KIND_QUICKFIX,
<<"-export([", FABin/binary, "]).\n">>,
els_protocol:range(#{from => Pos, to => Pos})
)
]
end;
{error, not_found} ->
[]
end.

-spec ignore_variable(uri(), range(), binary(), [binary()]) -> [map()].
ignore_variable(Uri, Range, _Data, [UnusedVariable]) ->
{ok, Document} = els_utils:lookup_document(Uri),
Expand Down
36 changes: 24 additions & 12 deletions apps/els_lsp/src/els_crossref_diagnostics.erl
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
source/0
]).

-type missing_reason() :: module | function | export.

%%==============================================================================
%% Includes
%%==============================================================================
Expand Down Expand Up @@ -114,19 +116,23 @@ make_diagnostic({missing, Kind}, #{id := Id} = POI) ->
make_diagnostic(true, _) ->
[].

-spec range(module | function, els_poi:poi()) -> els_poi:poi_range().
-spec range(missing_reason(), els_poi:poi()) -> els_poi:poi_range().
range(module, #{data := #{mod_range := Range}}) ->
Range;
range(function, #{data := #{name_range := Range}}) ->
Range;
range(export, #{data := #{name_range := Range}}) ->
Range;
range(_, #{range := Range}) ->
Range.

-spec error_msg(module | function, els_poi:poi_id()) -> binary().
-spec error_msg(missing_reason(), els_poi:poi_id()) -> binary().
error_msg(module, {M, _F, _A}) ->
els_utils:to_binary(io_lib:format("Cannot find module ~p", [M]));
error_msg(function, Id) ->
els_utils:to_binary(io_lib:format("Cannot find definition for function ~s", [id_str(Id)])).
els_utils:to_binary(io_lib:format("Cannot find definition for function ~s", [id_str(Id)]));
error_msg(export, Id) ->
els_utils:to_binary(io_lib:format("Function ~s is not exported.", [id_str(Id)])).

-spec id_str(els_poi:poi_id()) -> string().
id_str(Id) ->
Expand All @@ -136,7 +142,7 @@ id_str(Id) ->
end.

-spec has_definition(els_poi:poi(), els_dt_document:item(), _) ->
true | {missing, function | module}.
true | {missing, missing_reason()}.
has_definition(#{data := #{imported := true}}, _Document, _Opts) ->
%% Call to a bif
true;
Expand Down Expand Up @@ -177,7 +183,9 @@ has_definition(
case function_lookup(MFA) of
true ->
true;
false ->
{missing, export} ->
true;
{missing, function} ->
case els_code_navigation:goto_definition(Uri, POI) of
{ok, _Defs} ->
true;
Expand All @@ -189,12 +197,14 @@ has_definition(#{id := {M, _F, _A} = MFA} = POI, _Document, _Opts) ->
case function_lookup(MFA) of
true ->
true;
false ->
{missing, export} ->
{missing, export};
{missing, function} ->
case els_utils:find_module(M) of
{ok, Uri} ->
case els_code_navigation:goto_definition(Uri, POI) of
{ok, _Defs} ->
true;
function_lookup(MFA);
{error, _Error} ->
{missing, function}
end;
Expand All @@ -205,13 +215,15 @@ has_definition(#{id := {M, _F, _A} = MFA} = POI, _Document, _Opts) ->
has_definition(_POI, #{uri := _Uri}, _Opts) ->
true.

-spec function_lookup(mfa()) -> boolean().
-spec function_lookup(mfa()) -> true | {missing, missing_reason()}.
function_lookup(MFA) ->
case els_db:lookup(els_dt_functions:name(), MFA) of
case els_dt_functions:lookup(MFA) of
{ok, []} ->
false;
{ok, _} ->
true
{missing, function};
{ok, [#{is_exported := true}]} ->
true;
{ok, [#{is_exported := false}]} ->
{missing, export}
end.

-spec lager_definition(atom(), integer()) -> boolean().
Expand Down
21 changes: 13 additions & 8 deletions apps/els_lsp/src/els_dt_functions.erl
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,15 @@

-record(els_dt_functions, {
mfa :: mfa() | '_' | {atom(), '_', '_'},
version :: version() | '_'
version :: version() | '_',
is_exported :: boolean() | '_'
}).
-type els_dt_functions() :: #els_dt_functions{}.
-type version() :: null | integer().
-type item() :: #{
mfa := mfa(),
version := version()
version := version(),
is_exported := boolean()
}.
-export_type([item/0]).

Expand All @@ -65,21 +67,25 @@ opts() ->
-spec from_item(item()) -> els_dt_functions().
from_item(#{
mfa := MFA,
version := Version
version := Version,
is_exported := IsExported
}) ->
#els_dt_functions{
mfa = MFA,
version = Version
version = Version,
is_exported = IsExported
}.

-spec to_item(els_dt_functions()) -> item().
to_item(#els_dt_functions{
mfa = MFA,
version = Version
version = Version,
is_exported = IsExported
}) ->
#{
mfa => MFA,
version => Version
version => Version,
is_exported => IsExported
}.

-spec insert(item()) -> ok | {error, any()}.
Expand All @@ -96,8 +102,7 @@ versioned_insert(#{mfa := MFA, version := Version} = Map) ->
els_db:conditional_write(name(), MFA, Record, Condition).

-spec lookup(mfa()) -> {ok, [item()]}.
lookup({M, _F, _A} = MFA) ->
{ok, _Uris} = els_utils:find_modules(M),
lookup(MFA) ->
{ok, Items} = els_db:lookup(name(), MFA),
{ok, [to_item(Item) || Item <- Items]}.

Expand Down
11 changes: 7 additions & 4 deletions apps/els_lsp/src/els_indexing.erl
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,17 @@ index_signature(M, Text, #{id := {F, A}, range := Range, data := #{args := Args}
-spec index_functions(atom(), uri(), [els_poi:poi()], version()) -> ok.
index_functions(M, Uri, POIs, Version) ->
ok = els_dt_functions:versioned_delete_by_uri(Uri, Version),
[index_function(M, POI, Version) || #{kind := function} = POI <- POIs],
Exports = [{F, A} || #{id := {F, A}, kind := export_entry} <- POIs],
[index_function(M, POI, Exports, Version) || #{kind := function} = POI <- POIs],
ok.

-spec index_function(atom(), els_poi:poi(), version()) -> ok.
index_function(M, #{id := {F, A}}, Version) ->
-spec index_function(atom(), els_poi:poi(), els_poi:poi_id(), version()) -> ok.
index_function(M, #{id := {F, A}}, Exports, Version) ->
IsExported = lists:member({F, A}, Exports),
els_dt_functions:versioned_insert(#{
mfa => {M, F, A},
version => Version
version => Version,
is_exported => IsExported
}).

-spec index_references(atom(), uri(), [els_poi:poi()], version()) -> ok.
Expand Down
49 changes: 49 additions & 0 deletions apps/els_lsp/test/els_code_action_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
-export([
add_underscore_to_unused_var/1,
export_unused_function/1,
export_external_function/1,
suggest_variable/1,
fix_module_name/1,
remove_unused_macro/1,
Expand Down Expand Up @@ -158,6 +159,54 @@ export_unused_function(Config) ->
?assertEqual(Expected, Result),
ok.

-spec export_external_function(config()) -> ok.
export_external_function(Config) ->
Uri = ?config(code_navigation_uri, Config),
%% Don't care
Range = els_protocol:range(#{
from => {1, 1},
to => {1, 2}
}),
Diag = #{
message => <<"Function code_action:function_c/0 is not exported.">>,
range => Range,
severity => 2,
source => <<"CrossRef">>
},
#{result := Result} = els_client:document_codeaction(Uri, Range, [Diag]),
TargetUri = ?config(code_action_uri, Config),
Expected =
[
#{
edit => #{
changes =>
#{
binary_to_atom(TargetUri, utf8) =>
[
#{
range =>
#{
'end' => #{
character => 0,
line => ?COMMENTS_LINES + 3
},
start => #{
character => 0,
line => ?COMMENTS_LINES + 3
}
},
newText => <<"-export([function_c/0]).\n">>
}
]
}
},
kind => <<"quickfix">>,
title => <<"Export function_c/0">>
}
],
?assertEqual(Expected, Result),
ok.

-spec suggest_variable(config()) -> ok.
suggest_variable(Config) ->
Uri = ?config(code_action_uri, Config),
Expand Down
11 changes: 11 additions & 0 deletions apps/els_lsp/test/els_diagnostics_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,12 @@ crossref(_Config) ->
#{
message => <<"Cannot find definition for function lists:map/3">>,
range => {{5, 8}, {5, 11}}
},

#{
message =>
<<"Function code_navigation:function_c/0 is not exported.">>,
range => {{16, 20}, {16, 30}}
}
],
Warnings = [],
Expand All @@ -802,6 +808,11 @@ crossref_compiler_enabled(_Config) ->
#{
message => <<"Cannot find definition for function lists:map/3">>,
range => {{5, 8}, {5, 11}}
},
#{
message =>
<<"Function code_navigation:function_c/0 is not exported.">>,
range => {{16, 20}, {16, 30}}
}
],
Warnings = [],
Expand Down
Loading