Unused functions with cyclic dependencies are now also warned as unused

Closes #1105

Author: José Valim

CHANGELOG.md

@@ -5,6 +5,7 @@
 * bug fix
   * [Dict] `Enum.to_list` and `Dict.to_list` now return the same results for dicts
   * [Kernel] Fix a bug where `unquote_splicing` did not work on the left side of a stab op
+  * [Kernel] Unused functions with cyclic dependencies are now also warned as unused
   * [Mix] Fix a bug where `mix deps.get` was not retrieving nested dependencies
   * [Record] Fix a bug where nested records cannot be defined
 

lib/elixir/include/elixir.hrl

@@ -16,6 +16,7 @@
   name_args=false,         %% when true, it means arguments should be named
   module=nil,              %% the current module
   function=nil,            %% the current function
+  function_kind=nil,       %% the current function kind
   vars=[],                 %% a dict of defined variables and their alias
   temp_vars=[],            %% a dict of all variables defined in a particular assign
   clause_vars=nil,         %% a dict of all variables defined in a particular clause

lib/elixir/lib/gen_server/behaviour.ex

@@ -104,7 +104,7 @@ defmodule GenServer.Behaviour do
         { :noreply, state }
       end
 
-      def terminate(reason, state) do
+      def terminate(_reason, _state) do
         :ok
       end
 

lib/elixir/lib/kernel/locals_tracker.ex

@@ -0,0 +1,132 @@
+# This is a module Elixir responsible for tracking
+# local calls in order to emit proper warnings when
+# any local private function is unused.
+#
+# Since this is required for bootstrap, we can't use
+# any of the GenServer.Behaviour conveniences.
+defmodule Kernel.LocalsTracker do
+  @moduledoc false
+
+  @timeout  30_000
+  @behavior :gen_server
+
+  def start_link do
+    { :ok, pid } = :gen_server.start_link(__MODULE__, [], [])
+    pid
+  end
+
+  def add_definition(pid, kind, tuple) when kind in [:def, :defp, :defmacro, :defmacrop] do
+    :gen_server.cast(pid, { :vertex, kind, tuple })
+  end
+
+  def add_dispatch(pid, from, to) do
+    :gen_server.cast(pid, { :edge, from, to })
+  end
+
+  def reachable(pid) do
+    :gen_server.call(pid, :reachable, @timeout)
+  end
+
+  def collect_unused(pid, private) do
+    # Add a vertex for each private given
+    lc { tuple, kind, _defaults } inlist private do
+      add_definition(pid, kind, tuple)
+    end
+
+    # Process all unused
+    reachable = reachable(pid)
+    :lists.foldl(collect_unused(&1, &2, reachable), [], private)
+  end
+
+  defp collect_unused({ tuple, kind, 0 }, acc, reachable) do
+    if :lists.member(tuple, reachable) do
+      acc
+    else
+      [{ :unused_def, tuple, kind }|acc]
+    end
+  end
+
+  defp collect_unused({ tuple, kind, default }, acc, reachable) when default > 0 do
+    { name, arity } = tuple
+    min = arity - default
+    max = arity
+
+    invoked = lc { n, a } inlist reachable, n == name, a in min..max, do: a
+
+    if invoked == [] do
+      [{ :unused_def, tuple, kind }|acc]
+    else
+      case :lists.min(invoked) - min do
+        0 -> acc
+        ^default -> [{ :unused_args, tuple }|acc]
+        unused_args -> [{ :unused_args, tuple, unused_args }|acc]
+      end
+    end
+  end
+
+  def stop(pid) do
+    :gen_server.cast(pid, :stop)
+  end
+
+  # Callbacks
+
+  def init([]) do
+    d = :digraph.new
+    :digraph.add_vertex(d, :root)
+    { :ok, d }
+  end
+
+  def handle_call(:reachable, _from, d) do
+    { :reply, reduce_reachable(d, :root, []), d }
+  end
+
+  def handle_call(_request, _from, d) do
+    { :noreply, d }
+  end
+
+  def handle_info(_msg, d) do
+    { :noreply, d }
+  end
+
+  def handle_cast({ :edge, from, to }, d) do
+    :digraph.add_vertex(d, to)
+    [:"$e"|_] = :digraph.add_edge(d, from, to)
+    { :noreply, d }
+  end
+
+  def handle_cast({ :vertex, public, tuple }, d) when public in [:def, :defmacro] do
+    :digraph.add_vertex(d, tuple)
+    :digraph.add_edge(d, :root, tuple)
+    { :noreply, d }
+  end
+
+  def handle_cast({ :vertex, private, tuple }, d) when private in [:defp, :defmacrop] do
+    :digraph.add_vertex(d, tuple)
+    { :noreply, d }
+  end
+
+  def handle_cast(:stop, d) do
+    { :stop, :normal, d }
+  end
+
+  def handle_cast(_msg, d) do
+    { :noreply, d }
+  end
+
+  def terminate(_reason, _d) do
+    :ok
+  end
+
+  def code_change(_old, d, _extra) do
+    { :ok, d }
+  end
+
+  # Helpers
+
+  defp reduce_reachable(d, vertex, vertices) do
+    neighbours = :digraph.out_neighbours(d, vertex)
+    remaining  = neighbours -- vertices
+    vertices   = neighbours ++ vertices
+    :lists.foldl(reduce_reachable(d, &1, &2), vertices, remaining)
+  end
+end

lib/elixir/lib/macro/env.ex

@@ -12,6 +12,7 @@ defmodule Macro.Env do
   * `function` - a tuple as `{ atom, integer` }, where the first
     element is the function name and the seconds its arity. Returns
     `nil` if not inside a function
+  * `function_kind` - the kind of the function (`def`, `defp`, etc)
   * `aliases` -  a list of two item tuples, where the first
     item is the aliased name and the second the actual name
   * `context` - the context of the environment. It can be nil
@@ -31,13 +32,15 @@ defmodule Macro.Env do
   @type functions :: [{ module, [name_arity] }]
   @type macros :: [{ module, [name_arity] }]
   @type context_modules :: [module]
+  @type function_kind :: :def | :defp | :defmacro | :defmacrop
 
-  fields = [:module, :file, :line, :function, :aliases,
-            :context, :requires, :functions, :macros, :context_modules]
+  fields = [:module, :file, :line, :function, :aliases, :context, :requires,
+            :functions, :macros, :context_modules, :function_kind]
 
   types  = quote do: [module: module, file: file, line: line,
     function: name_arity, aliases: aliases, requires: requires,
-    functions: functions, macros: macros, context_modules: context_modules]
+    functions: functions, macros: macros, context_modules: context_modules,
+    function_kind: function_kind]
 
   Record.deffunctions(fields, __MODULE__)
   Record.deftypes(fields, types, __MODULE__)

lib/elixir/src/elixir_compiler.erl

@@ -225,6 +225,7 @@ core_main() ->
     "lib/elixir/lib/kernel/cli.ex",
     "lib/elixir/lib/kernel/error_handler.ex",
     "lib/elixir/lib/kernel/parallel_compiler.ex",
+    "lib/elixir/lib/kernel/locals_tracker.ex",
     "lib/elixir/lib/kernel/record_rewriter.ex"
   ].
 

lib/elixir/src/elixir_def.erl

@@ -110,7 +110,7 @@ store_definition(Kind, Line, _CheckClauses, nil, _Name, _Args, _Guards, _Body, #
 
 store_definition(Kind, Line, CheckClauses, Module, Name, Args, Guards, Body, #elixir_scope{} = DS) ->
   Arity = length(Args),
-  S     = DS#elixir_scope{function={Name,Arity}, module=Module},
+  S = DS#elixir_scope{module=Module, function={Name,Arity}, function_kind=Kind},
 
   CO = elixir_compiler:get_opts(),
   Location = retrieve_file(Line, Module, S, CO),

lib/elixir/src/elixir_def_local.erl

@@ -1,20 +1,53 @@
 %% Module responsible for local invocation of macros and functions.
 -module(elixir_def_local).
 -export([
-  macro_for/3,
-  function_for/3,
-  format_error/1,
-  check_unused_local/3
+  setup/1, cleanup/1,
+  record/2, record_root/2,
+  macro_for/3, function_for/3,
+  check_unused_local/3, format_error/1
 ]).
 -include("elixir.hrl").
 
+-define(attr, '__locals_tracker').
+-define(tracker, 'Elixir.Kernel.LocalsTracker').
+
+setup(Module) ->
+  case code:ensure_loaded(?tracker) of
+    { module, _ } -> ets:insert(Module, { ?attr, ?tracker:start_link() });
+    { error, _ }  -> ok
+  end.
+
+cleanup(Module) ->
+  if_tracker(Module, fun(Pid) -> ?tracker:stop(Pid) end).
+
+record(Tuple, #elixir_scope{function=Function})
+    when Function == nil; Function == Tuple -> false;
+record(Tuple, #elixir_scope{module=Module, function=Function, function_kind=Kind}) ->
+  if_tracker(Module, fun(Pid) ->
+    ?tracker:add_definition(Pid, Kind, Function),
+    ?tracker:add_dispatch(Pid, Function, Tuple),
+    true
+  end).
+
+record_root(Module, Tuple) ->
+  if_tracker(Module, fun(Pid) ->
+    ?tracker:add_dispatch(Pid, root, Tuple),
+    true
+  end).
+
+if_tracker(Module, Callback) ->
+  case ets:lookup(Module, ?attr) of
+    [{ ?attr, Pid }] -> Callback(Pid);
+    _ -> false
+  end.
+
 %% Used by elixir_dispatch, returns false if no macro is found
 macro_for(_Tuple, _All, #elixir_scope{module=nil}) -> false;
 
 macro_for(Tuple, All, #elixir_scope{module=Module} = S) ->
   try elixir_def:lookup_definition(Module, Tuple) of
     { { Tuple, Kind, Line, _, _, _, _ }, Clauses } when Kind == defmacro; All, Kind == defmacrop ->
-      elixir_import:record_local(Tuple, S),
+      record(Tuple, S),
       get_function(Line, Module, Clauses);
     _ ->
       false
@@ -72,40 +105,18 @@ rewrite_clause(Else, _) -> Else.
 
 %% Error handling
 
-check_unused_local(File, Recorded, Private) ->
-  [check_unused_local(Fun, Kind, Line, File, Defaults, Recorded) ||
-    { Fun, Kind, Line, true, Defaults } <- Private].
-
-check_unused_local(Fun, Kind, Line, File, 0, Recorded) ->
-  not(lists:member(Fun, Recorded)) andalso
-    elixir_errors:handle_file_warning(File, { Line, ?MODULE, { unused_def, Kind, Fun } });
+check_unused_local(File, Module, Private) ->
+  if_tracker(Module, fun(Pid) ->
+    Args = [ { Fun, Kind, Defaults } ||
+             { Fun, Kind, _Line, true, Defaults } <- Private],
 
-check_unused_local({ Name, Arity } = Fun, Kind, Line, File, Defaults, Recorded) when Defaults > 0 ->
-  Min = Arity - Defaults,
-  Max = Arity,
+    Unused = ?tracker:collect_unused(Pid, Args),
 
-  Invoked = [A || { N, A } <- Recorded, A >= Min, A =< Max, N == Name],
-
-  case Invoked of
-    [] ->
-      elixir_errors:handle_file_warning(File, { Line, ?MODULE, { unused_def, Kind, Fun } });
-    _ ->
-      UnusedArgs = lists:min(Invoked) - Min,
-      if
-        UnusedArgs == 0 ->
-          ok;
-        UnusedArgs == Defaults ->
-          elixir_errors:handle_file_warning(File, { Line, ?MODULE, { unused_args, Fun } });
-        true ->
-          elixir_errors:handle_file_warning(File, { Line, ?MODULE, { unused_args, Fun, UnusedArgs } })
-      end
-  end;
-
-check_unused_local(_Fun, _Kind, _Line, _File, _Defaults, _Recorded) ->
-  ok.
-
-format_error({unused_def,defp,{Name, Arity}}) ->
-  io_lib:format("function ~ts/~B is unused", [Name, Arity]);
+    [ begin
+        { _, _, Line, _, _ } = lists:keyfind(element(2, Error), 1, Private),
+        elixir_errors:handle_file_warning(File, { Line, ?MODULE, Error })
+      end || Error <- Unused ]
+  end).
 
 format_error({unused_args,{Name, Arity}}) ->
   io_lib:format("default arguments in ~ts/~B are never used", [Name, Arity]);
@@ -116,5 +127,8 @@ format_error({unused_args,{Name, Arity},1}) ->
 format_error({unused_args,{Name, Arity},Count}) ->
   io_lib:format("the first ~B default arguments in ~ts/~B are never used", [Count, Name, Arity]);
 
-format_error({unused_def,defmacrop,{Name, Arity}}) ->
+format_error({unused_def,{Name, Arity},defp}) ->
+  io_lib:format("function ~ts/~B is unused", [Name, Arity]);
+
+format_error({unused_def,{Name, Arity},defmacrop}) ->
   io_lib:format("macro ~ts/~B is unused", [Name, Arity]).

lib/elixir/src/elixir_dispatch.erl

@@ -41,7 +41,7 @@ import_function(Meta, Name, Arity, S) ->
     { import, Receiver } ->
       require_function(Meta, Receiver, Name, Arity, S);
     nomatch ->
-      elixir_import:record_local(Tuple, S),
+      elixir_def_local:record(Tuple, S),
       { { 'fun', ?line(Meta), { function, Name, Arity } }, S }
   end.
 

lib/elixir/src/elixir_import.erl

@@ -2,17 +2,19 @@
 %% in between local functions and imports.
 %% For imports dispatch, please check elixir_dispatch.
 -module(elixir_import).
--export([import/5, record_local/2, recorded_locals/1, format_error/1,
+-export([import/5, format_error/1,
   ensure_no_import_conflict/4, ensure_no_local_conflict/4,
   ensure_all_imports_used/3,
   build_table/1, delete_table/1, record/3]).
 -include("elixir.hrl").
 
 %% This table keeps:
 %%
-%% * Invoked imports and requires in the format { { Name, Arity }, Module }
-%% * The current warn status imports and requires in the format { Module, Line :: integer }
-%% * Invoked locals in the format { { Name, Arity }, Public :: boolean }
+%% * Invoked imports and requires in the format
+%%   { { Name, Arity }, Module }
+%
+%% * The current warn status imports and requires
+%%   in the format { Module, Line :: integer }
 %%
 table(Module) -> ?atom_concat([i, Module]).
 
@@ -30,11 +32,6 @@ record(Tuple, Receiver, Module) ->
     error:badarg -> false
   end.
 
-record_local(Tuple, #elixir_scope{function=Function})
-    when Function == nil; Function == Tuple -> false;
-record_local(Tuple, #elixir_scope{module=Module}) ->
-  record(Tuple, true, Module).
-
 record_warn(_Meta, _Ref, _Opts, #elixir_scope{module=nil}) -> false;
 record_warn(Meta, Ref, Opts, S) ->
   Table = table(S#elixir_scope.module),
@@ -49,16 +46,6 @@ record_warn(Meta, Ref, Opts, S) ->
 
   Warn andalso ets:insert(Table, { Ref, ?line(Meta) }).
 
-recorded_locals(Module) ->
-  Table  = table(Module),
-  Match  = module_local_spec(),
-  Result = ets:select(Table, Match),
-  ets:select_delete(Table, Match),
-  Result.
-
-module_local_spec() ->
-  [{ { '$1', '$2' }, [{ 'orelse', {'==','$2',true}, {'==','$2',false} }], ['$1'] }].
-
 %% IMPORT HELPERS
 
 %% Update the scope to consider the imports for aliases