Repository: rrrene/credo
Pull request #652 (2019-04-29 15:26:33)

Fix DuplicatedCode not matching blocks with external calls
DuplicatedCode (and other checks which use Credo.Code.remove_metadata)
would fail to detect duplicities whenever there are any external
function calls (aliases in AST) and the duplicities don't happen to be
at exactly the same line numbers. So credo would detect if two modules
are exactly equal, but adding a "# creeedo:disabled-four-that-foul"
comment on top is enough to disable the check, which doesn't seem very
intentional. :-)

This is because Credo.Code.update_metadata doesn't recurse into

    {{:., …, …}, …, …}

nodes. Fix this by dropping the hand-crafted implementation and use
Macro.prewalk and Macro.update_meta instead.


Patch:
diff --git a/lib/credo/code.ex b/lib/credo/code.ex
index 813622e3..b869804c 100644
--- a/lib/credo/code.ex
+++ b/lib/credo/code.ex
@@ -196,50 +196,7 @@ defmodule Credo.Code do
   @doc """
   Returns an AST without its metadata.
   """
-  def remove_metadata(ast) when is_tuple(ast) do
-    update_metadata(ast, fn _ast -> [] end)
-  end
-
   def remove_metadata(ast) do
-    ast
-    |> List.wrap()
-    |> Enum.map(&update_metadata(&1, fn _ast -> [] end))
-  end
-
-  defp update_metadata({atom, _meta, list} = ast, fun) when is_list(list) do
-    {atom, fun.(ast), Enum.map(list, &update_metadata(&1, fun))}
-  end
-
-  defp update_metadata([do: tuple], fun) when is_tuple(tuple) do
-    [do: update_metadata(tuple, fun)]
-  end
-
-  defp update_metadata([do: tuple, else: tuple2], fun) when is_tuple(tuple) do
-    [do: update_metadata(tuple, fun), else: update_metadata(tuple2, fun)]
-  end
-
-  defp update_metadata({:do, tuple}, fun) when is_tuple(tuple) do
-    {:do, update_metadata(tuple, fun)}
-  end
-
-  defp update_metadata({:else, tuple}, fun) when is_tuple(tuple) do
-    {:else, update_metadata(tuple, fun)}
-  end
-
-  defp update_metadata({atom, _meta, arguments} = ast, fun) do
-    {atom, fun.(ast), arguments}
+    Macro.prewalk(ast, &Macro.update_meta(&1, fn _meta -> [] end))
   end
-
-  defp update_metadata(v, fun) when is_list(v), do: Enum.map(v, &update_metadata(&1, fun))
-
-  defp update_metadata(tuple, fun) when is_tuple(tuple) do
-    tuple
-    |> Tuple.to_list()
-    |> Enum.map(&update_metadata(&1, fun))
-    |> List.to_tuple()
-  end
-
-  defp update_metadata(v, _fun)
-       when is_atom(v) or is_binary(v) or is_float(v) or is_integer(v),
-       do: v
 end


Test patch:
diff --git a/test/credo/check/design/duplicated_code_test.exs b/test/credo/check/design/duplicated_code_test.exs
index 4097db29..ab64803d 100644
--- a/test/credo/check/design/duplicated_code_test.exs
+++ b/test/credo/check/design/duplicated_code_test.exs
@@ -75,6 +75,37 @@ defmodule Credo.Check.Design.DuplicatedCodeTest do
     |> assert_issues(@described_check, mass_threshold: 16)
   end
 
+  test "should raise an issue for duplicated code with different line numbers and external function call" do
+    s1 = """
+    defmodule M1 do
+      def myfun(p1, p2) when is_list(p2) do
+        if p1 == p2 do
+          A.f(p1)
+        else
+          p2 + p1
+        end
+      end
+    end
+    """
+
+    s2 = """
+    defmodule M2 do
+      # additional line here
+      def myfun(p1, p2) when is_list(p2) do
+        if p1 == p2 do
+          A.f(p1)
+        else
+          p2 + p1
+        end
+      end
+    end
+    """
+
+    [s1, s2]
+    |> to_source_files
+    |> assert_issues(@described_check, mass_threshold: 16)
+  end
+
   test "should NOT raise an issue for duplicated code via macros if macros are in :excluded_macros param" do
     s1 = """
     defmodule M1 do
diff --git a/test/credo/code_test.exs b/test/credo/code_test.exs
index ab5bb1ef..7bad6d99 100644
--- a/test/credo/code_test.exs
+++ b/test/credo/code_test.exs
@@ -363,4 +363,22 @@ defmodule Credo.CodeTest do
 
     assert expected == Credo.Code.remove_metadata(ast)
   end
+
+  test "returns ast with external call without metadata" do
+    ast =
+      {{:., [line: 1, column: 7],
+        [
+          {:__aliases__, [line: 1, column: 1], [:Kernel]},
+          :node
+        ]}, [line: 1, column: 7], []}
+
+    expected =
+      {{:., [],
+        [
+          {:__aliases__, [], [:Kernel]},
+          :node
+        ]}, [], []}
+
+    assert expected == Credo.Code.remove_metadata(ast)
+  end
 end