From 2a7d58c3f778ff5c8dfef1d2eb37a57b72d43bbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sun, 10 Jul 2022 20:09:47 +0200 Subject: [PATCH] Register functions used in local macros as exports Ideally we want to list them as requires but we don't have the infrastructure to do so. So meanwhile, we list them as exports, which is the same level used by require. --- lib/elixir/lib/kernel/lexical_tracker.ex | 101 ++++++++---------- lib/elixir/src/elixir_def.erl | 41 +++++-- lib/elixir/src/elixir_dispatch.erl | 4 +- lib/elixir/src/elixir_erl.erl | 19 +--- lib/elixir/src/elixir_lexical.erl | 27 +++-- lib/elixir/src/elixir_map.erl | 9 +- .../elixir/kernel/lexical_tracker_test.exs | 38 +++++-- lib/elixir/test/elixir/kernel/quote_test.exs | 2 +- 8 files changed, 135 insertions(+), 106 deletions(-) diff --git a/lib/elixir/lib/kernel/lexical_tracker.ex b/lib/elixir/lib/kernel/lexical_tracker.ex index b27cf19351..0b7ed7c050 100644 --- a/lib/elixir/lib/kernel/lexical_tracker.ex +++ b/lib/elixir/lib/kernel/lexical_tracker.ex @@ -30,8 +30,8 @@ defmodule Kernel.LexicalTracker do end @doc false - def add_require(pid, module) when is_atom(module) do - :gen_server.cast(pid, {:add_require, module}) + def add_export(pid, module) when is_atom(module) do + :gen_server.cast(pid, {:add_export, module}) end @doc false @@ -88,23 +88,24 @@ defmodule Kernel.LexicalTracker do @doc false def collect_unused_imports(pid) do - unused(pid, :import) + unused(pid, :unused_imports) end @doc false def collect_unused_aliases(pid) do - unused(pid, :alias) + unused(pid, :unused_aliases) end defp unused(pid, tag) do - :gen_server.call(pid, {:unused, tag}, @timeout) + :gen_server.call(pid, tag, @timeout) end # Callbacks def init(:ok) do state = %{ - directives: %{}, + aliases: %{}, + imports: %{}, references: %{}, exports: %{}, cache: %{}, @@ -116,13 +117,12 @@ defmodule Kernel.LexicalTracker do end @doc false - def handle_call({:unused, tag}, _from, state) do - directives = - for {{^tag, module_or_mfa}, marker} <- state.directives, is_integer(marker) do - {module_or_mfa, marker} - end + def handle_call(:unused_aliases, _from, state) do + {:reply, Enum.sort(state.aliases), state} + end - {:reply, Enum.sort(directives), state} + def handle_call(:unused_imports, _from, state) do + {:reply, Enum.sort(state.imports), state} end def handle_call(:references, _from, state) do @@ -148,12 +148,26 @@ defmodule Kernel.LexicalTracker do end def handle_cast({:import_dispatch, module, {function, arity}, mode}, state) do - state = add_import_dispatch(state, module, function, arity, mode) - {:noreply, state} + %{imports: imports, references: references} = state + + imports = + case imports do + %{^module => modules_and_fas} -> + modules_and_fas + |> Map.delete(module) + |> Map.delete({function, arity}) + |> then(&Map.put(imports, module, &1)) + + %{} -> + imports + end + + references = add_reference(references, module, mode) + {:noreply, %{state | imports: imports, references: references}} end - def handle_cast({:alias_dispatch, module}, state) do - {:noreply, %{state | directives: add_dispatch(state.directives, module, :alias)}} + def handle_cast({:alias_dispatch, module}, %{aliases: aliases} = state) do + {:noreply, %{state | aliases: Map.delete(aliases, module)}} end def handle_cast({:set_file, file}, state) do @@ -168,28 +182,25 @@ defmodule Kernel.LexicalTracker do {:noreply, update_in(state.compile_env, &:ordsets.add_element({app, path, return}, &1))} end - def handle_cast({:add_require, module}, state) do + def handle_cast({:add_export, module}, state) do {:noreply, put_in(state.exports[module], true)} end def handle_cast({:add_import, module, fas, line, warn}, state) do - to_remove = for {{:import, {^module, _, _}} = key, _} <- state.directives, do: key - - directives = - state.directives - |> Map.drop(to_remove) - |> add_directive(module, line, warn, :import) - - directives = - Enum.reduce(fas, directives, fn {function, arity}, directives -> - add_directive(directives, {module, function, arity}, line, warn, :import) - end) - - {:noreply, %{state | directives: directives}} + if warn do + imports = for module_or_fa <- [module | fas], do: {module_or_fa, line}, into: %{} + {:noreply, put_in(state.imports[module], imports)} + else + {:noreply, state} + end end def handle_cast({:add_alias, module, line, warn}, state) do - {:noreply, %{state | directives: add_directive(state.directives, module, line, warn, :alias)}} + if warn do + {:noreply, put_in(state.aliases[module], line)} + else + {:noreply, state} + end end @doc false @@ -221,31 +232,9 @@ defmodule Kernel.LexicalTracker do do: Map.put(references, module, :compile) defp add_reference(references, module, :runtime) when is_atom(module) do - case Map.fetch(references, module) do - {:ok, _} -> references - :error -> Map.put(references, module, :runtime) + case references do + %{^module => _} -> references + %{} -> Map.put(references, module, :runtime) end end - - defp add_import_dispatch(state, module, function, arity, mode) do - directives = - state.directives - |> add_dispatch(module, :import) - |> add_dispatch({module, function, arity}, :import) - - references = add_reference(state.references, module, mode) - %{state | directives: directives, references: references} - end - - # In the map we keep imports and aliases. - # If the value is a line, it was imported/aliased and has a pending warning - # If the value is true, it was imported/aliased and used - defp add_directive(directives, module_or_mfa, line, warn, tag) do - marker = if warn, do: line, else: true - Map.put(directives, {tag, module_or_mfa}, marker) - end - - defp add_dispatch(directives, module_or_mfa, tag) do - Map.put(directives, {tag, module_or_mfa}, true) - end end diff --git a/lib/elixir/src/elixir_def.erl b/lib/elixir/src/elixir_def.erl index d36f764b78..d23377047e 100644 --- a/lib/elixir/src/elixir_def.erl +++ b/lib/elixir/src/elixir_def.erl @@ -1,6 +1,6 @@ % Holds the logic responsible for function definitions (def(p) and defmacro(p)). -module(elixir_def). --export([setup/1, reset_last/1, local_for/4, +-export([setup/1, reset_last/1, local_for/5, external_for/5, take_definition/2, store_definition/5, store_definition/9, fetch_definitions/2, format_error/1]). -include("elixir.hrl"). @@ -16,18 +16,32 @@ reset_last({DataSet, _DataBag}) -> reset_last(Module) when is_atom(Module) -> reset_last(elixir_module:data_tables(Module)). -local_for(Module, Name, Arity, Kinds) -> +%% Finds the local definition for the current module. +local_for(Meta, Name, Arity, Kinds, E) -> + External = fun({Mod, Fun}, Args) -> + invoke_external([{from_local_macro, {Name, Arity}} | Meta], Mod, Fun, Args, E) + end, + fun_for(Meta, ?key(E, module), Name, Arity, Kinds, {value, External}). + +%% Finds the local definition for an external module. +external_for(Meta, Module, Name, Arity, Kinds) -> + fun_for(Meta, Module, Name, Arity, Kinds, none). + +fun_for(Meta, Module, Name, Arity, Kinds, External) -> Tuple = {Name, Arity}, try {Set, Bag} = elixir_module:data_tables(Module), {ets:lookup(Set, {def, Tuple}), ets:lookup(Bag, {clauses, Tuple})} of - {[{_, Kind, Meta, _, _, _}], Clauses} -> + {[{_, Kind, LocalMeta, _, _, _}], ClausesPairs} -> case (Kinds == all) orelse (lists:member(Kind, Kinds)) of - true -> elixir_erl:definition_to_anonymous(Module, Kind, Meta, - [Clause || {_, Clause} <- Clauses]); - false -> false + true -> + Local = {value, fun(Fun, Args) -> invoke_local(Meta, Module, Fun, Args, External) end}, + Clauses = [Clause || {_, Clause} <- ClausesPairs], + elixir_erl:definition_to_anonymous(Kind, LocalMeta, Clauses, Local, External); + false -> + false end; {[], _} -> false @@ -35,6 +49,21 @@ local_for(Module, Name, Arity, Kinds) -> _:_ -> false end. +invoke_local(Meta, Module, ErlName, Args, External) -> + {Name, Arity} = elixir_utils:erl_fa_to_elixir_fa(ErlName, length(Args)), + + case fun_for(Meta, Module, Name, Arity, all, External) of + false -> + {current_stacktrace, [_ | T]} = erlang:process_info(self(), current_stacktrace), + erlang:raise(error, undef, [{Module, Name, Arity, []} | T]); + Fun -> + apply(Fun, Args) + end. + +invoke_external(Meta, Mod, Name, Args, E) -> + is_map(E) andalso elixir_env:trace({require, Meta, Mod, []}, E), + apply(Mod, Name, Args). + %% Take a definition out of the table take_definition(Module, {Name, Arity} = Tuple) -> diff --git a/lib/elixir/src/elixir_dispatch.erl b/lib/elixir/src/elixir_dispatch.erl index a51b1eb614..65325f0a25 100644 --- a/lib/elixir/src/elixir_dispatch.erl +++ b/lib/elixir/src/elixir_dispatch.erl @@ -71,7 +71,7 @@ import_function(Meta, Name, Arity, E) -> Function = ?key(E, function), case (Function /= nil) andalso (Function /= Tuple) andalso - elixir_def:local_for(?key(E, module), Name, Arity, [defmacro, defmacrop]) of + elixir_def:local_for(Meta, Name, Arity, [defmacro, defmacrop], E) of false -> elixir_env:trace({local_function, Meta, Name, Arity}, E), elixir_locals:record_local(Tuple, ?key(E, module), ?key(E, function), Meta, false), @@ -142,7 +142,7 @@ expand_import(Meta, {Name, Arity} = Tuple, Args, S, E, Extra, External) -> _ -> AllowLocals = External orelse ((Function /= nil) andalso (Function /= Tuple)), Local = AllowLocals andalso - elixir_def:local_for(Module, Name, Arity, [defmacro, defmacrop]), + elixir_def:local_for(Meta, Name, Arity, [defmacro, defmacrop], E), case Dispatch of %% There is a local and an import. This is a conflict unless diff --git a/lib/elixir/src/elixir_erl.erl b/lib/elixir/src/elixir_erl.erl index 0f8757b0cb..55dfcb3008 100644 --- a/lib/elixir/src/elixir_erl.erl +++ b/lib/elixir/src/elixir_erl.erl @@ -1,6 +1,7 @@ %% Compiler backend to Erlang. + -module(elixir_erl). --export([elixir_to_erl/1, elixir_to_erl/2, definition_to_anonymous/4, compile/1, consolidate/3, +-export([elixir_to_erl/1, elixir_to_erl/2, definition_to_anonymous/5, compile/1, consolidate/3, get_ann/1, debug_info/4, scope/2, format_error/1]). -include("elixir.hrl"). -define(typespecs, 'Elixir.Kernel.Typespec'). @@ -47,24 +48,12 @@ get_ann([], Gen, Line) -> erl_anno:set_generated(Gen, erl_anno:new(Line)). %% Converts an Elixir definition to an anonymous function. -definition_to_anonymous(Module, Kind, Meta, Clauses) -> +definition_to_anonymous(Kind, Meta, Clauses, LocalHandler, ExternalHandler) -> ErlClauses = [translate_clause(Kind, 0, Clause, true) || Clause <- Clauses], Fun = {'fun', ?ann(Meta), {clauses, ErlClauses}}, - LocalHandler = fun(LocalName, LocalArgs) -> invoke_local(Module, LocalName, LocalArgs) end, - {value, Result, _Binding} = erl_eval:expr(Fun, [], {value, LocalHandler}), + {value, Result, _Binding} = erl_eval:expr(Fun, [], LocalHandler, ExternalHandler), Result. -invoke_local(Module, ErlName, Args) -> - {Name, Arity} = elixir_utils:erl_fa_to_elixir_fa(ErlName, length(Args)), - - case elixir_def:local_for(Module, Name, Arity, all) of - false -> - {current_stacktrace, [_ | T]} = erlang:process_info(self(), current_stacktrace), - erlang:raise(error, undef, [{Module, Name, Arity, []} | T]); - Fun -> - apply(Fun, Args) - end. - %% Converts Elixir quoted literals to Erlang AST. elixir_to_erl(Tree) -> elixir_to_erl(Tree, erl_anno:new(0)). diff --git a/lib/elixir/src/elixir_lexical.erl b/lib/elixir/src/elixir_lexical.erl index c655333728..2f6de52de4 100644 --- a/lib/elixir/src/elixir_lexical.erl +++ b/lib/elixir/src/elixir_lexical.erl @@ -47,10 +47,10 @@ trace({alias_expansion, _Meta, Lookup, _Result}, #{lexical_tracker := Pid}) -> ?tracker:alias_dispatch(Pid, Lookup), ok; trace({require, _Meta, Module, _Opts}, #{lexical_tracker := Pid}) -> - ?tracker:add_require(Pid, Module), + ?tracker:add_export(Pid, Module), ok; trace({struct_expansion, _Meta, Module, _Keys}, #{lexical_tracker := Pid}) -> - ?tracker:add_require(Pid, Module), + ?tracker:add_export(Pid, Module), ok; trace({alias_reference, _Meta, Module}, #{lexical_tracker := Pid} = E) -> ?tracker:remote_dispatch(Pid, Module, mode(E)), @@ -98,21 +98,20 @@ with_file(File, #{lexical_tracker := Pid} = E, Callback) -> %% ERROR HANDLING warn_unused_imports(Pid, E) -> - {ModuleImports, MFAImports} = - lists:partition(fun({M, _}) -> is_atom(M) end, ?tracker:collect_unused_imports(Pid)), - - Modules = [M || {M, _L} <- ModuleImports], - MFAImportsFiltered = [T || {{M, _, _}, _} = T <- MFAImports, not lists:member(M, Modules)], - - [begin - elixir_errors:form_warn([{line, L}], ?key(E, file), ?MODULE, {unused_import, M}) - end || {M, L} <- ModuleImports ++ MFAImportsFiltered], + [elixir_errors:form_warn([{line, Line}], ?key(E, file), ?MODULE, {unused_import, ModOrMFA}) + || {Module, Imports} <- ?tracker:collect_unused_imports(Pid), + {ModOrMFA, Line} <- unused_imports_for_module(Module, Imports)], ok. +unused_imports_for_module(Module, Imports) -> + case Imports of + #{Module := Line} -> [{Module, Line}]; + #{} -> [{{Module, Fun, Arity}, Line} || {{Fun, Arity}, Line} <- maps:to_list(Imports)] + end. + warn_unused_aliases(Pid, E) -> - [begin - elixir_errors:form_warn([{line, L}], ?key(E, file), ?MODULE, {unused_alias, M}) - end || {M, L} <- ?tracker:collect_unused_aliases(Pid)], + [elixir_errors:form_warn([{line, Line}], ?key(E, file), ?MODULE, {unused_alias, Module}) + || {Module, Line} <- ?tracker:collect_unused_aliases(Pid)], ok. format_error({unused_alias, Module}) -> diff --git a/lib/elixir/src/elixir_map.erl b/lib/elixir/src/elixir_map.erl index 4a84696278..500a61d0f3 100644 --- a/lib/elixir/src/elixir_map.erl +++ b/lib/elixir/src/elixir_map.erl @@ -141,13 +141,14 @@ wrapped_load_struct(Meta, Name, Args, Keys, E) -> InContext = lists:member(Name, [?key(E, module) | ?key(E, context_modules)]), Arity = length(Args), - Local = InContext orelse (not(ensure_loaded(Name)) andalso wait_for_struct(Name)), + External = InContext orelse (not(ensure_loaded(Name)) andalso wait_for_struct(Name)), try - case Local andalso elixir_def:local_for(Name, '__struct__', Arity, [def]) of + case External andalso elixir_def:external_for(Meta, Name, '__struct__', Arity, [def]) of false -> apply(Name, '__struct__', Args); - LocalFun -> + + ExternalFun -> %% There is an inherent race condition when using local_for. %% By the time we got to execute the function, the ETS table %% with temporary definitions for the given module may no longer @@ -157,7 +158,7 @@ wrapped_load_struct(Meta, Name, Args, Keys, E) -> %% the table has not been deleted (unless compilation of that %% module failed which should then cause this call to fail too). try - apply(LocalFun, Args) + apply(ExternalFun, Args) catch error:undef -> apply(Name, '__struct__', Args) end diff --git a/lib/elixir/test/elixir/kernel/lexical_tracker_test.exs b/lib/elixir/test/elixir/kernel/lexical_tracker_test.exs index c69a841a6e..61deac2f2c 100644 --- a/lib/elixir/test/elixir/kernel/lexical_tracker_test.exs +++ b/lib/elixir/test/elixir/kernel/lexical_tracker_test.exs @@ -22,7 +22,7 @@ defmodule Kernel.LexicalTrackerTest do end test "can add requires", config do - D.add_require(config[:pid], URI) + D.add_export(config[:pid], URI) assert D.references(config[:pid]) == {[], [URI], [], []} D.remote_dispatch(config[:pid], URI, :runtime) @@ -33,7 +33,7 @@ defmodule Kernel.LexicalTrackerTest do end test "can add module imports", config do - D.add_require(config[:pid], String) + D.add_export(config[:pid], String) D.add_import(config[:pid], String, [], 1, true) D.import_dispatch(config[:pid], String, {:upcase, 1}, :runtime) @@ -44,7 +44,7 @@ defmodule Kernel.LexicalTrackerTest do end test "can add module with {function, arity} imports", config do - D.add_require(config[:pid], String) + D.add_export(config[:pid], String) D.add_import(config[:pid], String, [upcase: 1], 1, true) D.import_dispatch(config[:pid], String, {:upcase, 1}, :compile) @@ -59,32 +59,32 @@ defmodule Kernel.LexicalTrackerTest do test "unused module imports", config do D.add_import(config[:pid], String, [], 1, true) - assert D.collect_unused_imports(config[:pid]) == [{String, 1}] + assert D.collect_unused_imports(config[:pid]) == [{String, %{String => 1}}] end test "used module imports are not unused", config do D.add_import(config[:pid], String, [], 1, true) D.import_dispatch(config[:pid], String, {:upcase, 1}, :compile) - assert D.collect_unused_imports(config[:pid]) == [] + assert D.collect_unused_imports(config[:pid]) == [{String, %{}}] end test "unused {module, function, arity} imports", config do D.add_import(config[:pid], String, [upcase: 1], 1, true) - assert D.collect_unused_imports(config[:pid]) == [{String, 1}, {{String, :upcase, 1}, 1}] + assert D.collect_unused_imports(config[:pid]) == [{String, %{String => 1, {:upcase, 1} => 1}}] end test "used {module, function, arity} imports are not unused", config do D.add_import(config[:pid], String, [upcase: 1], 1, true) D.add_import(config[:pid], String, [downcase: 1], 1, true) D.import_dispatch(config[:pid], String, {:upcase, 1}, :compile) - assert D.collect_unused_imports(config[:pid]) == [{{String, :downcase, 1}, 1}] + assert D.collect_unused_imports(config[:pid]) == [{String, %{{:downcase, 1} => 1}}] end test "overwriting {module, function, arity} import with module import", config do D.add_import(config[:pid], String, [upcase: 1], 1, true) D.add_import(config[:pid], String, [], 1, true) D.import_dispatch(config[:pid], String, {:downcase, 1}, :compile) - assert D.collect_unused_imports(config[:pid]) == [] + assert D.collect_unused_imports(config[:pid]) == [{String, %{}}] end test "imports with no warn are not unused", config do @@ -282,5 +282,27 @@ defmodule Kernel.LexicalTrackerTest do refute Kernel.LexicalTrackerTest.Defmodule in exports refute Kernel.LexicalTrackerTest.Defmodule in runtime end + + # TODO: This should not be an export dependency but all requires are exports at the moment + test "defmacro adds an export dependency" do + {{compile, exports, runtime, _}, _binding} = + Code.eval_string(""" + defmodule Kernel.LexicalTrackerTest.Defmacro do + defmacro uri() do + Macro.escape(URI.parse("/hello")) + end + + def fun() do + uri() + end + + Kernel.LexicalTracker.references(__ENV__.lexical_tracker) + end |> elem(3) + """) + + refute URI in compile + assert URI in exports + assert URI in runtime + end end end diff --git a/lib/elixir/test/elixir/kernel/quote_test.exs b/lib/elixir/test/elixir/kernel/quote_test.exs index c81aec22ed..c0b81d55ba 100644 --- a/lib/elixir/test/elixir/kernel/quote_test.exs +++ b/lib/elixir/test/elixir/kernel/quote_test.exs @@ -601,7 +601,7 @@ defmodule Kernel.QuoteTest.ImportsHygieneTest do test "lazy expand imports no conflicts" do import Kernel, except: [length: 1] - import String, only: [length: 1] + import String, only: [length: 1], warn: false assert get_list_length() == 5 assert get_list_length_with_partial() == 5