From 9739eb66d47b0fd57cc493952e0f1593d73f7a10 Mon Sep 17 00:00:00 2001
From: Christian Koch <ckoch@cars.com>
Date: Sun, 14 Aug 2022 22:03:12 -0500
Subject: [PATCH] fix(handle extra changes): mood-fn calls

filter properly for only module-function calls when making changes.
---
 lib/ex_factor.ex                |   2 +-
 lib/ex_factor/callers.ex        |  48 ++++----
 lib/ex_factor/changer.ex        | 193 ++++++++++++++++++++++++--------
 test/ex_factor/callers_test.exs |  26 ++---
 test/ex_factor/changer_test.exs |  94 +++-------------
 5 files changed, 194 insertions(+), 169 deletions(-)

diff --git a/lib/ex_factor.ex b/lib/ex_factor.ex
index 950a85b..cd26624 100644
--- a/lib/ex_factor.ex
+++ b/lib/ex_factor.ex
@@ -49,7 +49,7 @@ defmodule ExFactor do
       |> Keyword.put_new(:target_path, path(target_module))
       |> Keyword.put_new(:source_path, path(source_module))
 
-    changes = Changer.rename_module(opts)
+    changes = Changer.change(opts)
 
     format(%{additions: %ExFactor{}, changes: changes, removals: %ExFactor{}}, dry_run, opts)
   end
diff --git a/lib/ex_factor/callers.ex b/lib/ex_factor/callers.ex
index cb94ec0..74dbe63 100644
--- a/lib/ex_factor/callers.ex
+++ b/lib/ex_factor/callers.ex
@@ -4,10 +4,6 @@ defmodule ExFactor.Callers do
   """
   import ExUnit.CaptureIO
 
-  alias ExFactor.Parser
-
-  def all_fns(input), do: Parser.all_functions(input)
-
   @doc """
   use `mix xref` list all the callers of a given module.
   """
@@ -37,11 +33,31 @@ defmodule ExFactor.Callers do
     ExFactor.Traces.trace()
   end
 
-  def callers(mod, func, arity) do
-    Mix.Tasks.Xref.calls([])
-    # |> IO.inspect(label: "callers/3 XREF calls")
-    |> Enum.filter(fn x ->
-      x.callee == {cast(mod), cast(func), arity}
+  def callers(mod, func, arity, trace_function \\ get_trace_function()) do
+    entries = trace_function.()
+    mod = cast(mod)
+    func = cast(func)
+
+    callers = fn -> Mix.Tasks.Xref.run(["callers", mod]) end
+    |> capture_io()
+    |> String.split("\n")
+
+    paths = Enum.map(callers, fn line ->
+      Regex.run(~r/\S.*\.ex*\S/, line)
+      |> case do
+        [path] -> path
+        other -> other
+      end
+    end)
+
+    Enum.filter(entries, fn {{path, _module}, fn_calls} = _tuple ->
+      rel_path = Path.relative_to(path, File.cwd!())
+      funs = Enum.filter(fn_calls, fn
+        {_, _, _, ^mod, ^func, ^arity} -> true
+        _ -> false
+      end)
+
+      rel_path in paths and match?([_ | _ ], funs)
     end)
   end
 
@@ -67,20 +83,6 @@ defmodule ExFactor.Callers do
     |> Module.concat()
   end
 
-  # defp mangle_list([""]), do: []
-  # defp mangle_list(["Compiling" <> _ | tail]), do: mangle_list(tail)
-
-  # defp mangle_list(list) do
-  #   Enum.map(list, fn string ->
-  #     [path, type] = String.split(string, " ")
-
-  #     %{
-  #       file: path,
-  #       dependency_type: type
-  #     }
-  #   end)
-  # end
-
   defp get_trace_function do
     :ex_factor
     |> Application.get_env(__MODULE__, trace_function: &ExFactor.Callers.trace_function/0)
diff --git a/lib/ex_factor/changer.ex b/lib/ex_factor/changer.ex
index 4b37e84..cdc4628 100644
--- a/lib/ex_factor/changer.ex
+++ b/lib/ex_factor/changer.ex
@@ -7,33 +7,27 @@ defmodule ExFactor.Changer do
   alias ExFactor.Callers
   alias ExFactor.Parser
 
-  @doc """
-  Given all the Callers of a module, find the instances of usage of the module and refactor the
-  module reference to the new module. Respect any existing aliases.
-  """
-  def rename_module(opts) do
-    source_module = Keyword.fetch!(opts, :source_module)
-
-    source_module
-    |> Callers.callers()
-    |> update_caller_modules(opts)
-  end
+  # @doc """
+  # Given all the Callers of a module, find the instances of usage of the module and refactor the
+  # module reference to the new module. Respect any existing aliases.
+  # """
+  # def rename_module(opts) do
+  #   source_module = Keyword.fetch!(opts, :source_module)
+
+  #   source_module
+  #   |> Callers.callers()
+  #   |> update_caller_groups(opts)
+  # end
 
   @doc """
   Given all the Callers to a module, find the instances of the target function and refactor the
   function module reference to the new module. Respect any existing aliases.
   """
   def change(opts) do
-    # Mix.Tasks.Compile.Elixir.run([])
-    # :timer.sleep(100)
     source_module = Keyword.fetch!(opts, :source_module)
-    source_function = Keyword.fetch!(opts, :source_function)
-    arity = Keyword.fetch!(opts, :arity)
 
     source_module
-    |> Callers.callers(source_function, arity)
-    # |> IO.inspect(label: "callers")
-    # |> Enum.group_by(& &1.file)
+    |> Callers.callers()
     |> update_caller_groups(opts)
   end
 
@@ -51,55 +45,155 @@ defmodule ExFactor.Changer do
     ]
   end
 
-  defp update_caller_groups(callers, opts) do
-    dry_run = Keyword.get(opts, :dry_run, false)
+  # defp update_caller_groups(callers, opts) do
+  #   dry_run = Keyword.get(opts, :dry_run, false)
 
-    Enum.map(callers, fn {file, [first | _] = grouped_callers} ->
-      file_list =
-        File.read!(file)
-        |> String.split("\n")
+  #   Enum.map(callers, fn {file, [first | _] = grouped_callers} ->
+  #     file_list =
+  #       File.read!(file)
+  #       |> String.split("\n")
 
-      grouped_callers
-      |> Enum.reduce({[:unchanged], file_list}, fn %{line: line}, acc ->
-        find_and_replace_function(acc, opts, line)
-      end)
-      |> maybe_add_import(opts)
-      |> maybe_add_alias(opts)
-      |> write_file(first.caller_module, file, dry_run)
-    end)
-  end
+  #     grouped_callers
+  #     |> Enum.reduce({[:unchanged], file_list}, fn %{line: line}, acc ->
+  #       find_and_replace(acc, opts, line)
+  #     end)
+  #     |> maybe_add_import(opts)
+  #     |> maybe_add_alias(opts)
+  #     |> write_file(first.caller_module, file, dry_run)
+  #   end)
+  # end
 
-  defp update_caller_modules(callers, opts) do
+  defp update_caller_groups(callers, opts) do
     dry_run = Keyword.get(opts, :dry_run, false)
     source_module = Keyword.fetch!(opts, :source_module)
-    # source_path = Keyword.get(opts, :source_path)
-
+    source_function = Keyword.get(opts, :source_function)
+    |> IO.inspect(label: "source_function")
     mod = Callers.cast(source_module)
-    Enum.map(callers, fn {{file_path, module}, fn_calls} ->
+
+    case source_function do
+      nil -> callers
+        Enum.filter(callers, fn {{file_path, module}, fn_calls} ->
+          Enum.find(fn_calls, fn
+          {_, _, _, ^mod, _, _} -> true
+          _ -> false
+          end)
+        end)
+      fun ->
+        # fun |> IO.inspect(label: "function")
+        # callers |> IO.inspect(label: "callers")
+        fun_atom = Callers.cast(fun)
+        Enum.filter(callers, fn {{file_path, module}, fn_calls} ->
+          Enum.find(fn_calls, fn
+          {_, _, _, ^mod, ^fun_atom, _} -> true
+          _ -> false
+          end)
+        end)
+        # arity = Keyword.get(opts, :arity)
+        # |> IO.inspect(label: "arity")
+        # matching_functions = Enum.filter(fn_calls, fn
+          # {_, _, _, ^mod, ^fun_atom, _} -> true
+          # _ -> false
+    end
+    |> IO.inspect(label: "before mapping something?")
+    |> Enum.map(fn {{file_path, module}, fn_calls} ->
       file_list =
         file_path
         |> File.read!()
         |> String.split("\n")
-
-      matching_functions = Enum.filter(fn_calls, fn
-        {_, _, _, ^mod, _, _} -> true
-        _ -> false
-      end)
-      |> Enum.reduce({[:unchanged], file_list}, fn {_, line, _, _, _, _}, acc ->
-        find_and_replace_module(acc, opts, line)
+      # |> IO.inspect(label: "matching functions")
+      Enum.reduce(fn_calls, {[:unchanged], file_list}, fn
+        {_, line, _, ^mod, _, _}, acc ->
+          find_and_replace(acc, opts, line)
+        _, acc -> acc
       end)
       # |> IO.inspect(label: "FUNS")
       |> maybe_add_import(opts)
       |> maybe_add_alias(opts)
       |> write_file(module, file_path, dry_run)
-
     end)
   end
 
-  defp find_and_replace_function({state, file_list}, opts, line) do
+  defp find_and_replace({state, file_list} = tuple, opts, line) do
+    # opts values
+    case Keyword.get(opts, :source_function) do
+      nil -> update_module(tuple, opts, line)
+      _source_function -> update_module_and_function(tuple, opts, line)
+    end
+    # source_module = Keyword.fetch!(opts, :source_module)
+    # target_module = Keyword.fetch!(opts, :target_module)
+
+    # # modified values
+    # source_string = to_string(source_module)
+    # source_modules = String.split(source_module, ".")
+    # source_alias = Enum.at(source_modules, -1)
+    # target_alias = preferred_alias(file_list, target_module)
+    # source_alias_alt = find_alias_as(file_list, source_module)
+    # fn_line = Enum.at(file_list, line - 1)
+
+    # {new_state, new_line} =
+    #   cond do
+    #     # match full module name
+    #     String.match?(fn_line, ~r/#{source_string}\.#{source_function}/) ->
+    #       fn_line = String.replace(fn_line, source_module, target_alias)
+    #       {set_state(state, :changed), fn_line}
+
+    #     # match aliased module name
+    #     String.match?(fn_line, ~r/#{source_alias}\.#{source_function}/) ->
+    #       fn_line = String.replace(fn_line, source_alias, target_alias)
+    #       {set_state(state, :changed), fn_line}
+
+    #     # match module name aliased :as
+    #     String.match?(fn_line, ~r/#{source_alias_alt}\.#{source_function}/) ->
+    #       fn_line = String.replace(fn_line, source_alias_alt, target_alias)
+    #       {set_state(state, :changed), fn_line}
+
+    #     true ->
+    #       {state, fn_line}
+    #   end
+
+    # {new_state, List.replace_at(file_list, line - 1, new_line)}
+  end
+
+  # defp find_and_replace_module({state, file_list}, opts, line) do
+  #   # opts values
+  #   source_module = Keyword.fetch!(opts, :source_module)
+  #   target_module = Keyword.fetch!(opts, :target_module)
+
+  #   # modified values
+  #   source_string = to_string(source_module)
+  #   source_modules = String.split(source_module, ".")
+  #   source_alias = Enum.at(source_modules, -1)
+  #   target_alias = preferred_alias(file_list, target_module)
+  #   source_alias_alt = find_alias_as(file_list, source_module)
+  #   fn_line = Enum.at(file_list, line - 1)
+
+  #   {new_state, new_line} =
+  #     cond do
+  #       # match full module name
+  #       String.match?(fn_line, ~r/#{source_string}/) ->
+  #         fn_line = String.replace(fn_line, source_module, target_alias)
+  #         {set_state(state, :changed), fn_line}
+
+  #       # match aliased module name
+  #       String.match?(fn_line, ~r/#{source_alias}/) ->
+  #         fn_line = String.replace(fn_line, source_alias, target_alias)
+  #         {set_state(state, :changed), fn_line}
+
+  #       # match module name aliased :as
+  #       String.match?(fn_line, ~r/#{source_alias_alt}/) ->
+  #         fn_line = String.replace(fn_line, source_alias_alt, target_alias)
+  #         {set_state(state, :changed), fn_line}
+
+  #       true ->
+  #         {state, fn_line}
+  #     end
+
+  #   {new_state, List.replace_at(file_list, line - 1, new_line)}
+  # end
+
+  defp update_module_and_function({state, file_list}, opts, line) do
     # opts values
     source_module = Keyword.fetch!(opts, :source_module)
-    source_function = Keyword.fetch!(opts, :source_function)
     target_module = Keyword.fetch!(opts, :target_module)
 
     # modified values
@@ -108,6 +202,7 @@ defmodule ExFactor.Changer do
     source_alias = Enum.at(source_modules, -1)
     target_alias = preferred_alias(file_list, target_module)
     source_alias_alt = find_alias_as(file_list, source_module)
+    {:ok, source_function} = Keyword.fetch(opts, :source_function)
     fn_line = Enum.at(file_list, line - 1)
 
     {new_state, new_line} =
@@ -134,7 +229,7 @@ defmodule ExFactor.Changer do
     {new_state, List.replace_at(file_list, line - 1, new_line)}
   end
 
-  defp find_and_replace_module({state, file_list}, opts, line) do
+  defp update_module({state, file_list}, opts, line) do
     # opts values
     source_module = Keyword.fetch!(opts, :source_module)
     target_module = Keyword.fetch!(opts, :target_module)
@@ -222,6 +317,7 @@ defmodule ExFactor.Changer do
     Enum.join(contents_list, "\n")
   end
 
+  defp maybe_add_alias({[:unchanged], _} = resp, _), do: resp
   defp maybe_add_alias({state, contents_list}, opts) do
     target_module = Keyword.fetch!(opts, :target_module)
     target_string = to_string(target_module)
@@ -274,6 +370,7 @@ defmodule ExFactor.Changer do
     end
   end
 
+  defp maybe_add_import({[:unchanged], _} = resp, _), do: resp
   defp maybe_add_import({state, contents_list}, opts) do
     source_module = Keyword.fetch!(opts, :source_module)
     target_module = Keyword.fetch!(opts, :target_module)
diff --git a/test/ex_factor/callers_test.exs b/test/ex_factor/callers_test.exs
index 5227f49..2172b85 100644
--- a/test/ex_factor/callers_test.exs
+++ b/test/ex_factor/callers_test.exs
@@ -2,7 +2,7 @@ defmodule ExFactor.CallersTest do
   use ExUnit.Case
   alias ExFactor.Callers
 
-  describe "callers/1" do
+  describe "callers/2" do
     test "it should report callers of a module using a 0-arity trace function" do
       callers = Callers.callers(ExFactor.Parser, fn -> ExFactor.Support.Trace.trace_function() end)
 
@@ -30,27 +30,17 @@ defmodule ExFactor.CallersTest do
     end
   end
 
-  describe "callers/3" do
+  describe "callers/4" do
     test "it should report callers of a module" do
-      [one, _two, _three] =
-        ExFactor.Parser
-        |> Callers.callers(:all_functions, 1)
-        |> Enum.sort_by(& &1.file)
-
-      assert one.caller_module == ExFactor.Callers
-      assert one.file == "lib/ex_factor/callers.ex"
-      assert one.line == 9
+      callers = Callers.callers(ExFactor.Parser, :all_functions, 1)
+      assert Enum.find(callers, fn {{path, _module}, _funs} -> path == "lib/ex_factor/remover.ex" end)
+      assert Enum.find(callers, fn {{_path, module}, _funs} -> module == ExFactor.Remover end)
     end
 
     test "it converts strings to atoms" do
-      [one, _two, _three] =
-        "ExFactor.Parser"
-        |> Callers.callers("all_functions", 1)
-        |> Enum.sort_by(& &1.file)
-
-      assert one.caller_module == ExFactor.Callers
-      assert one.file == "lib/ex_factor/callers.ex"
-      assert one.line == 9
+      callers = Callers.callers("ExFactor.Parser", "all_functions", 1)
+      assert Enum.find(callers, fn {{path, _module}, _funs} -> path == "lib/ex_factor/remover.ex" end)
+      assert Enum.find(callers, fn {{_path, module}, _funs} -> module == ExFactor.Remover end)
     end
 
     test "when the module is invalid" do
diff --git a/test/ex_factor/changer_test.exs b/test/ex_factor/changer_test.exs
index f9768c8..0e8375b 100644
--- a/test/ex_factor/changer_test.exs
+++ b/test/ex_factor/changer_test.exs
@@ -8,7 +8,7 @@ defmodule ExFactor.ChangerTest do
     :ok
   end
 
-  describe "rename_module/1" do
+  describe "change/1" do
     test "it renames all the instances of a module" do
       opts = [
         target_module: "ExFactor.Modified.Parser",
@@ -16,95 +16,31 @@ defmodule ExFactor.ChangerTest do
         dry_run: true
       ]
 
-      changes = Changer.rename_module(opts)
+      changes = Changer.change(opts)
 
       assert %ExFactor{} = change = Enum.find(changes, &(&1.module == ExFactor.Remover))
       assert [{ExFactor.Remover, _bin}] = Code.compile_string(change.file_contents)
     end
-  end
 
-  describe "change/1" do
     test "it finds all the callers of a module, function, and arity, and updates the calls to the new module" do
-      content = """
-        defmodule ExFactor.Tmp.SourceMod do
-          @moduledoc "
-          This is a multiline moduedoc
-          "
-          @doc "this is some documentation for refactor1/1"
-          def refactor1([]) do
-            :empty
-          end
-          def refactor1(arg1) do
-            {:ok, arg1}
-          end
-        end
-      """
-
-      File.write("lib/ex_factor/tmp/source_module.ex", content)
-
-      content = """
-      defmodule ExFactor.Tmp.CallerModule do
-        @moduledoc \"\"\"
-        This is a multiline moduedoc.
-        Its in the caller module
-        \"\"\"
-        alias ExFactor.Tmp.SourceMod
-        alias ExFactor.Tmp.SourceMod.Other
-        def pub1(arg_a) do
-          SourceMod.refactor1(arg_a)
-        end
-        def pub2, do: Other
-
-        def pub3(arg_a) do
-          SourceMod.refactor1(arg_a)
-        end
-      end
-      """
-
-      File.write("lib/ex_factor/tmp/caller_module.ex", content)
-
-      content = """
-      defmodule ExFactor.Tmp.CallerTwoModule do
-        alias ExFactor.Tmp.SourceMod
-        def pub1(arg_a) do
-          SourceMod.refactor1(arg_a)
-        end
-        def pub2, do: Enum
-      end
-      """
-
-      File.write("lib/ex_factor/tmp/caller_two_module.ex", content)
-
       opts = [
-        target_module: "ExFactor.Tmp.TargetModule",
-        source_module: "ExFactor.Tmp.SourceMod",
-        source_function: :refactor1,
-        arity: 1
+        target_module: "ExFactor.Modified.Parser",
+        source_module: "ExFactor.Parser",
+        source_function: "read_file",
+        arity: 1,
+        dry_run: true
       ]
 
       changes = Changer.change(opts)
 
-      caller = File.read!("lib/ex_factor/tmp/caller_module.ex")
-      assert caller =~ "alias ExFactor.Tmp.TargetModule"
-      # ensure we don't match dumbly
-      assert caller =~ "alias ExFactor.Tmp.SourceMod.Other"
-      refute caller =~ "alias ExFactor.Tmp.TargetModule.Other"
-      # assert the alias doesn't get spliced into the moduledoc
-      refute caller =~ "Its in the caller module\nalias ExFactor.Tmp.TargetModule\n  \""
-      assert caller =~ "TargetModule.refactor1(arg_a)"
-      # asser the function uses the alias
-      refute caller =~ "ExFactor.Tmp.TargetModule.refactor1(arg_a)"
-      assert caller =~ "def pub3(arg_a) do\n    TargetModule.refactor1(arg_a)"
-
-      caller_two = File.read!("lib/ex_factor/tmp/caller_two_module.ex")
-      assert caller_two =~ "alias ExFactor.Tmp.TargetModule"
-      # ensure we don't match dumbly
-      assert caller_two =~ "TargetModule.refactor1(arg_a)"
-      # asser the function uses the alias
-      refute caller_two =~ "ExFactor.Tmp.TargetModule.refactor1(arg_a)"
-
-      assert Enum.find(changes, &(&1.path == "lib/ex_factor/tmp/caller_module.ex"))
-      assert Enum.find(changes, &(&1.path == "lib/ex_factor/tmp/caller_two_module.ex"))
+      changed_modules = changes
+      |> Enum.map(& &1.module)
+      |> Enum.sort()
+
+      assert changed_modules == [ExFactor.Extractor, ExFactor.Remover]
+
+      assert %ExFactor{} = change = Enum.find(changes, &(&1.module == ExFactor.Remover))
+      assert [{ExFactor.Remover, _bin}] = Code.compile_string(change.file_contents)
     end
 
     test "only add alias entry if it's missing" do