Properly track unreachable defmacrops in locals tracker

Author: José Valim

lib/elixir/lib/module/locals_tracker.ex

@@ -138,8 +138,8 @@ defmodule Module.LocalsTracker do
 
   # Reattach a previously yanked node
   @doc false
-  def reattach(pid, kind, tuple, neighbours) do
-    :gen_server.cast(to_pid(pid), {:reattach, kind, tuple, neighbours})
+  def reattach(pid, tuple, kind, function, neighbours) do
+    :gen_server.cast(to_pid(pid), {:reattach, tuple, kind, function, neighbours})
   end
 
   # Collecting all conflicting imports with the given functions
@@ -162,15 +162,25 @@ defmodule Module.LocalsTracker do
   def collect_unused_locals(ref, private) do
     d = :gen_server.call(to_pid(ref), :digraph, @timeout)
     reachable = reachable_from(d, :local)
-    {unreachable(reachable, private), collect_warnings(reachable, private)}
+    reattached = :digraph.out_neighbours(d, :reattach)
+    {unreachable(reachable, reattached, private), collect_warnings(reachable, private)}
   end
 
-  defp unreachable(reachable, private) do
-    for {tuple, _, _, _} <- private,
-        not :sets.is_element(tuple, reachable),
+  defp unreachable(reachable, reattached, private) do
+    for {tuple, kind, _, _} <- private,
+        not reachable?(tuple, kind, reachable, reattached),
         do: tuple
   end
 
+  defp reachable?(tuple, :defmacrop, reachable, reattached) do
+    # All private micros are unreachable unless they have been
+    # reattached and they are reachable.
+    :lists.member(tuple, reattached) and :sets.is_element(tuple, reachable)
+  end
+  defp reachable?(tuple, :defp, reachable, _reattached) do
+    :sets.is_element(tuple, reachable)
+  end
+
   defp collect_warnings(reachable, private) do
     :lists.foldl(&collect_warnings(&1, &2, reachable), [], private)
   end
@@ -221,6 +231,7 @@ defmodule Module.LocalsTracker do
   def init([]) do
     d = :digraph.new([:protected])
     :digraph.add_vertex(d, :local)
+    :digraph.add_vertex(d, :reattach)
     {:ok, d}
   end
 
@@ -262,17 +273,29 @@ defmodule Module.LocalsTracker do
     {:noreply, d}
   end
 
-  def handle_cast({:reattach, _kind, tuple, {in_neigh, out_neigh}}, d) do
+  def handle_cast({:reattach, tuple, kind, function, {in_neigh, out_neigh}}, d) do
+    # Reattach the old function
     for from <- in_neigh do
       :digraph.add_vertex(d, from)
-      replace_edge!(d, from, tuple)
+      replace_edge!(d, from, function)
     end
 
     for to <- out_neigh do
       :digraph.add_vertex(d, to)
-      replace_edge!(d, tuple, to)
+      replace_edge!(d, function, to)
+    end
+
+    # Add the new definition
+    handle_add_definition(d, kind, tuple)
+
+    # Make a call from the old function to the new one
+    if function != tuple do
+      handle_add_local(d, function, tuple)
     end
 
+    # Finally marked the new one as reattached
+    replace_edge!(d, :reattach, tuple)
+
     {:noreply, d}
   end
 

lib/elixir/src/elixir_def.erl

@@ -58,8 +58,8 @@ fetch_definition([[Tuple] | T], File, Module, Table, All, Private) ->
 
   try ets:lookup_element(Table, {clauses, Tuple}, 2) of
     Clauses ->
-      Unwrapped =
-        {Tuple, Kind, Meta, Clauses},
+      NewAll =
+        [{Tuple, Kind, Meta, Clauses} | All],
       NewPrivate =
         case (Kind == defp) orelse (Kind == defmacrop) of
           true ->
@@ -68,7 +68,7 @@ fetch_definition([[Tuple] | T], File, Module, Table, All, Private) ->
           false ->
             Private
         end,
-      fetch_definition(T, File, Module, Table, [Unwrapped | All], NewPrivate)
+      fetch_definition(T, File, Module, Table, NewAll, NewPrivate)
   catch
     error:badarg ->
       warn_bodyless_function(Check, Meta, File, Module, Kind, Tuple),

lib/elixir/src/elixir_locals.erl

@@ -3,7 +3,7 @@
 -export([
   setup/1, cleanup/1, cache_env/1, cache_env/2, get_cached_env/1,
   record_local/2, record_local/3, record_import/4,
-  record_definition/3, record_defaults/4,
+  record_definition/3, record_defaults/4, reattach/5,
   ensure_no_import_conflict/3, warn_unused_local/3, format_error/1
 ]).
 
@@ -25,6 +25,9 @@ setup(Module) ->
 cleanup(Module) ->
   if_tracker(Module, fun(Pid) -> unlink(Pid), ?tracker:stop(Pid), ok end).
 
+reattach(Tuple, Kind, Module, Function, Neighbours) ->
+  if_tracker(Module, fun(Pid) -> ?tracker:reattach(Pid, Tuple, Kind, Function, Neighbours) end).
+
 record_local(Tuple, Module) when is_atom(Module) ->
   if_tracker(Module, fun(Pid) -> ?tracker:add_local(Pid, Tuple), ok end).
 record_local(Tuple, _Module, Function)

lib/elixir/src/elixir_overridable.erl

@@ -52,12 +52,9 @@ store(Module, Function, Hidden) ->
       case Overridden of
         false ->
           overridable(Module, maps:put(Function, {Count, Def, Neighbours, true}, Overridable)),
-          (not elixir_compiler:get_opt(internal)) andalso
-            'Elixir.Module.LocalsTracker':reattach(Module, Kind, Function, Neighbours),
           elixir_def:store_definition(false, FinalKind, Meta, FinalName, FinalArity,
                                       File, Module, Defaults, FinalClauses),
-          elixir_locals:record_definition(Tuple, FinalKind, Module),
-          elixir_locals:record_local(Tuple, Module, Function);
+          elixir_locals:reattach(Tuple, FinalKind, Module, Function, Neighbours);
         true ->
           ok
       end,

lib/elixir/test/elixir/fixtures/dialyzer/defmacrop.ex

@@ -0,0 +1,11 @@
+defmodule Dialyzer.Defmacrop do
+  defmacrop good_macro(id) do
+    quote do
+      {:good, {:good_macro, unquote(id)}}
+    end
+  end
+
+  def run() do
+    good_macro("Not So Bad")
+  end
+end

lib/elixir/test/elixir/kernel/dialyzer_test.exs

@@ -104,6 +104,11 @@ defmodule Kernel.DialyzerTest do
     end
   end
 
+  test "no warnings on defmacrop", context do
+    copy_beam! context, Dialyzer.Defmacrop
+    assert_dialyze_no_warnings! context
+  end
+
   defp copy_beam!(context, module) do
     name = "#{module}.beam"
     File.cp! Path.join(context[:base_dir], name),

lib/elixir/test/elixir/module/locals_tracker_test.exs

@@ -60,11 +60,11 @@ defmodule Module.LocalsTrackerTest do
     D.add_local(config[:pid], {:foo, 1}, {:bar, 1})
     D.add_definition(config[:pid], :defp, {:bar, 1})
 
-    {infoo, outfoo}   = D.yank(config[:pid], {:foo, 1})
+    {infoo, outfoo} = D.yank(config[:pid], {:foo, 1})
     {inbar, outbar} = D.yank(config[:pid], {:bar, 1})
 
-    D.reattach(config[:pid], :defp, {:bar, 1}, {inbar, outbar})
-    D.reattach(config[:pid], :def, {:foo, 1}, {infoo, outfoo})
+    D.reattach(config[:pid], {:bar, 1}, :defp, {:bar, 1}, {inbar, outbar})
+    D.reattach(config[:pid], {:foo, 1}, :def, {:foo, 1}, {infoo, outfoo})
     assert {:bar, 1} in D.reachable(config[:pid])
   end