Ensure warnings are proper with yanked and reattached overridables

Author: José Valim

lib/elixir/lib/module.ex

@@ -374,10 +374,12 @@ defmodule Module do
           raise "Cannot make function #{name}/#{arity} overridable because it was not defined"
         clause ->
           :elixir_def.delete_definition(module, tuple)
+          neighbours = Module.DispatchTracker.yank(module, tuple)
 
           old    = get_attribute(module, :__overridable)
-          new    = [ { tuple, { 1, clause, false } } ]
-          merged = :orddict.merge(fn(_k, { count, _, _ }, _v2) -> { count + 1, clause, false } end, old, new)
+          merged = :orddict.update(tuple, fn({ count, _, _, _ }) ->
+            { count + 1, clause, neighbours, false }
+          end, { 1, clause, neighbours, false }, old)
 
           put_attribute(module, :__overridable, merged)
       end

lib/elixir/lib/module/dispatch_tracker.ex

@@ -88,7 +88,7 @@ defmodule Module.DispatchTracker do
   end
 
   defp to_pid(pid) when is_pid(pid),  do: pid
-  defp to_pid(mod) when is_atom(mod), do: Module.get(mod, :__locals_tracker)
+  defp to_pid(mod) when is_atom(mod), do: Module.get_attribute(mod, :__dispatch_tracker)
 
   # Internal API
 
@@ -135,7 +135,7 @@ defmodule Module.DispatchTracker do
 
   # Collect all unused imports where warn has been set to true.
   def collect_unused_imports(pid) do
-    d = :gen_server.call(pid, :digraph)
+    d = :gen_server.call(pid, :digraph, @timeout)
     warnable = :digraph.out_neighbours(d, :warn)
 
     lc mod inlist warnable, not has_imports?(d, mod), line = get_warn_line(d, mod) do
@@ -153,10 +153,22 @@ defmodule Module.DispatchTracker do
     Enum.any?(:digraph.in_neighbours(d, mod), match?({ :import, _, _ }, &1))
   end
 
+  # Yanks a local node. Returns its in and out vertices in a tuple.
+  @doc false
+  def yank(pid, local) do
+    :gen_server.call(to_pid(pid), { :yank, local }, @timeout)
+  end
+
+  def reattach(pid, kind, tuple, neighbours) do
+    pid = to_pid(pid)
+    add_definition(pid, kind, tuple)
+    :gen_server.cast(pid, { :reattach, tuple, neighbours })
+  end
+
   # Collecting all conflicting imports with the given functions
   @doc false
   def collect_imports_conflicts(pid, all_defined) do
-    d = :gen_server.call(to_pid(pid), :digraph, @timeout)
+    d = :gen_server.call(pid, :digraph, @timeout)
 
     lc { name, arity } inlist all_defined,
        n = :digraph.out_neighbours(d, { :import, name, arity }),
@@ -216,6 +228,13 @@ defmodule Module.DispatchTracker do
     { :ok, d }
   end
 
+  def handle_call({ :yank, local }, _from, d) do
+    in_vertices  = :digraph.in_neighbours(d, local)
+    out_vertices = :digraph.out_neighbours(d, local)
+    :digraph.del_vertex(d, local)
+    { :reply, { in_vertices, out_vertices }, d }
+  end
+
   def handle_call(:digraph, _from, d) do
     { :reply, d, d }
   end
@@ -230,24 +249,24 @@ defmodule Module.DispatchTracker do
 
   def handle_cast({ :add_local, from, to }, d) do
     :digraph.add_vertex(d, to)
-    replace_edge(d, from, to)
+    replace_edge!(d, from, to)
     { :noreply, d }
   end
 
   def handle_cast({ :add_import, module, { name, arity } }, d) do
     :digraph.add_vertex(d, module)
-    replace_edge(d, :import, module)
+    replace_edge!(d, :import, module)
 
     tuple = { :import, name, arity }
     :digraph.add_vertex(d, tuple)
-    replace_edge(d, tuple, module)
+    replace_edge!(d, tuple, module)
 
     { :noreply, d }
   end
 
   def handle_cast({ :add_warnable, module, warn, line }, d) do
     :digraph.add_vertex(d, module)
-    replace_edge(d, :import, module)
+    replace_edge!(d, :import, module)
 
     if warn do
       :digraph.add_edge(d, :warn, module, line)
@@ -260,7 +279,7 @@ defmodule Module.DispatchTracker do
 
   def handle_cast({ :add_definition, public, tuple }, d) when public in [:def, :defmacro] do
     :digraph.add_vertex(d, tuple)
-    replace_edge(d, :local, tuple)
+    replace_edge!(d, :local, tuple)
     { :noreply, d }
   end
 
@@ -269,6 +288,12 @@ defmodule Module.DispatchTracker do
     { :noreply, d }
   end
 
+  def handle_cast({ :reattach, tuple, { in_neigh, out_neigh } }, d) do
+    lc from inlist in_neigh, do: replace_edge(d, from, tuple)
+    lc to inlist out_neigh,  do: replace_edge(d, tuple, to)
+    { :noreply, d }
+  end
+
   def handle_cast(:stop, d) do
     { :stop, :normal, d }
   end
@@ -285,9 +310,15 @@ defmodule Module.DispatchTracker do
     { :ok, d }
   end
 
-  defp replace_edge(d, from, to) do
+  defp replace_edge!(d, from, to) do
     unless :lists.member(to, :digraph.out_neighbours(d, from)) do
       [:"$e"|_] = :digraph.add_edge(d, from, to)
     end
   end
+
+  defp replace_edge(d, from, to) do
+    unless :lists.member(to, :digraph.out_neighbours(d, from)) do
+      :digraph.add_edge(d, from, to)
+    end
+  end
 end

lib/elixir/src/elixir_def_overridable.erl

@@ -15,13 +15,13 @@ overridable(Module, Value) ->
 is_defined(Module, Tuple) ->
   Overridable = overridable(Module),
   case orddict:find(Tuple, Overridable) of
-    { ok, { _, _, _ } } -> true;
+    { ok, { _, _, _, _ } } -> true;
     _ -> false
   end.
 
 ensure_defined(Meta, Module, Tuple, S) ->
   case is_defined(Module, Tuple) of
-    true -> [];
+    true -> ok;
     _    -> elixir_errors:form_error(Meta, S#elixir_scope.file, ?MODULE, { no_super, Module, Tuple })
   end.
 
@@ -53,24 +53,26 @@ name(Module, Function) ->
   name(Module, Function, overridable(Module)).
 
 name(_Module, { Name, _ } = Function, Overridable) ->
-  { Count, _, _ } = orddict:fetch(Function, Overridable),
+  { Count, _, _, _ } = orddict:fetch(Function, Overridable),
   ?atom_concat([Name, " (overridable ", Count, ")"]).
 
 %% Store
 
 store(Module, Function, GenerateName) ->
   Overridable = overridable(Module),
   case orddict:fetch(Function, Overridable) of
-    { _Count, _Clause, true } -> ok;
-    { Count, Clause, false } ->
-      overridable(Module, orddict:store(Function, { Count, Clause, true }, Overridable)),
+    { _Count, _Clause, _Neighbours, true } -> ok;
+    { Count, Clause, Neighbours, false } ->
+      overridable(Module, orddict:store(Function, { Count, Clause, Neighbours, true }, Overridable)),
       { { { Name, Arity }, Kind, Line, File, _Check, Location, Defaults }, Clauses } = Clause,
 
       { FinalKind, FinalName } = case GenerateName of
         true  -> { defp, name(Module, Function, Overridable) };
         false -> { Kind, Name }
       end,
 
+      'Elixir.Module.DispatchTracker':reattach(Module, Kind, { Name, Arity }, Neighbours),
+
       Def = { function, Line, FinalName, Arity, Clauses },
       elixir_def:store_each(false, FinalKind, File, Location,
         elixir_def:table(Module), elixir_def:clauses_table(Module), Defaults, Def)
@@ -79,13 +81,13 @@ store(Module, Function, GenerateName) ->
 %% Store pending declarations that were not manually made concrete.
 
 store_pending(Module) ->
-  [store(Module, X, false) || { X, { _, _, false } } <- overridable(Module),
+  [store(Module, X, false) || { X, { _, _, _, false } } <- overridable(Module),
     not 'Elixir.Module':'defines?'(Module, X)].
 
 %% Error handling
 
 format_error({ no_super, Module, { Name, Arity } }) ->
-  Bins   = [ format_fa(X) || { X, { _, _, _ } } <- overridable(Module)],
+  Bins   = [format_fa(X) || { X, { _, _, _, _ } } <- overridable(Module)],
   Joined = 'Elixir.Enum':join(Bins, <<", ">>),
   io_lib:format("no super defined for ~ts/~B in module ~ts. Overridable functions available are: ~ts",
     [Name, Arity, elixir_errors:inspect(Module), Joined]).

lib/elixir/test/elixir/kernel/warning_test.exs

@@ -75,7 +75,7 @@ defmodule Kernel.WarningTest do
     purge Sample
   end
 
-  test :unsued_default_args do
+  test :unused_default_args do
     assert capture_io(fn ->
       Code.eval_string """
       defmodule Sample1 do
@@ -171,6 +171,35 @@ defmodule Kernel.WarningTest do
     purge Sample
   end
 
+  test :unused_with_local_with_overridable do
+    assert capture_io(fn ->
+      Code.eval_string """
+      defmodule Sample do
+        def hello, do: world
+        defp world, do: :ok
+        defoverridable [hello: 0]
+        def hello, do: :ok
+      end
+      """
+    end) =~ %r"function world/0 is unused"
+  after
+    purge Sample
+  end
+
+  test :used_with_local_with_reattached_overridable do
+    assert nil? capture_io(fn ->
+      Code.eval_string """
+      defmodule Sample do
+        def hello, do: world
+        defp world, do: :ok
+        defoverridable [hello: 0, world: 0]
+      end
+      """
+    end)
+  after
+    purge Sample
+  end
+
   defp purge(list) when is_list(list) do
     Enum.each list, purge(&1)
   end