Merge pull request #662 from mirego/feature/add-unused-variable-names…

…-consistency-check

Add Credo.Check.Consistency.UnusedVariableNames

.credo.exs

@@ -140,6 +140,7 @@
         # Controversial and experimental checks (opt-in, just replace `false` with `[]`)
         #
         {Credo.Check.Consistency.MultiAliasImportRequireUse, false},
+        {Credo.Check.Consistency.UnusedVariableNames, false},
         {Credo.Check.Design.DuplicatedCode, false},
         {Credo.Check.Readability.MultiAlias, false},
         {Credo.Check.Readability.Specs, false},

lib/credo/check/consistency/unused_variable_names.ex

@@ -0,0 +1,57 @@
+defmodule Credo.Check.Consistency.UnusedVariableNames do
+  @moduledoc false
+
+  @checkdoc """
+  Elixir allows us to use `_` as a name for variables that are not meant to be
+  used. But it’s a common practice to give these variables meaningful names
+  anyway (`_user` instead of `_`), but some people prefer to name them all `_`.
+
+  A single style should be present in the same codebase.
+  """
+  @explanation [check: @checkdoc]
+  @collector Credo.Check.Consistency.UnusedVariableNames.Collector
+
+  use Credo.Check, run_on_all: true, base_priority: :high
+
+  @doc false
+  def run(source_files, exec, params \\ []) when is_list(source_files) do
+    @collector.find_and_append_issues(source_files, exec, params, &issues_for/3)
+  end
+
+  defp issues_for(expected, source_file, params) do
+    issue_meta = IssueMeta.for(source_file, params)
+    issue_locations = @collector.find_locations_not_matching(expected, source_file)
+
+    Enum.map(issue_locations, fn location ->
+      format_issue(issue_meta, [
+        {:message, message_for(expected, location[:trigger])} | location
+      ])
+    end)
+  end
+
+  defp message_for(:meaningful, trigger) do
+    message = """
+    Unused variables should be named consistently.
+    It seems your strategy is to give them meaningful names (eg. `_foo`)
+    but `#{trigger}` does not follow that convention."
+    """
+
+    to_one_line(message)
+  end
+
+  defp message_for(:anonymous, trigger) do
+    message = """
+    Unused variables should be named consistently.
+    It seems your strategy is to name them anonymously (ie. `_`)
+    but `#{trigger}` does not follow that convention.
+    """
+
+    to_one_line(message)
+  end
+
+  defp to_one_line(str) do
+    str
+    |> String.split()
+    |> Enum.join(" ")
+  end
+end

lib/credo/check/consistency/unused_variable_names/collector.ex

@@ -0,0 +1,63 @@
+defmodule Credo.Check.Consistency.UnusedVariableNames.Collector do
+  @moduledoc false
+
+  use Credo.Check.Consistency.Collector
+
+  alias Credo.Code
+
+  def collect_matches(source_file, _params) do
+    unused_variable_recorder = &record_unused_variable/2
+
+    Code.prewalk(source_file, &traverse(unused_variable_recorder, &1, &2), %{})
+  end
+
+  def find_locations_not_matching(expected, source_file) do
+    location_recorder = &record_not_matching(expected, &1, &2)
+
+    source_file
+    |> Code.prewalk(&traverse(location_recorder, &1, &2), [])
+    |> Enum.reverse()
+  end
+
+  defp traverse(callback, {:=, _, params} = ast, acc) do
+    {ast, reduce_unused_variables(params, callback, acc)}
+  end
+
+  defp traverse(callback, {def, _, [{_, _, params} | _]} = ast, acc)
+       when def in [:def, :defp] do
+    {ast, reduce_unused_variables(params, callback, acc)}
+  end
+
+  defp traverse(callback, {:->, _, [params | _]} = ast, acc) do
+    {ast, reduce_unused_variables(params, callback, acc)}
+  end
+
+  defp traverse(_callback, ast, acc), do: {ast, acc}
+
+  defp reduce_unused_variables(ast, callback, acc) do
+    Enum.reduce(ast, acc, &if(unused_variable_name?(&1), do: callback.(&1, &2), else: &2))
+  end
+
+  defp unused_variable_name?({:_, _, _}), do: true
+
+  defp unused_variable_name?({name, _, _}),
+    do: String.starts_with?(Atom.to_string(name), "_")
+
+  defp unused_variable_name?(_), do: false
+
+  defp record_unused_variable({:_, _, _}, acc), do: Map.update(acc, :anonymous, 1, &(&1 + 1))
+  defp record_unused_variable(_, acc), do: Map.update(acc, :meaningful, 1, &(&1 + 1))
+
+  defp record_not_matching(expected, {name, meta, _}, acc) do
+    case {expected, Atom.to_string(name)} do
+      {:anonymous, "_" <> rest = trigger} when rest != "" ->
+        [[line_no: meta[:line], trigger: trigger] | acc]
+
+      {:meaningful, "_" = trigger} ->
+        [[line_no: meta[:line], trigger: trigger] | acc]
+
+      _ ->
+        acc
+    end
+  end
+end

test/credo/check/consistency/unused_variable_names_test.exs

@@ -0,0 +1,107 @@
+defmodule Credo.Check.Consistency.UnusedVariableNamesTest do
+  use Credo.TestHelper
+
+  @described_check Credo.Check.Consistency.UnusedVariableNames
+
+  test "it should NOT report correct behaviour" do
+    [
+      """
+      defmodule Credo.SampleOne do
+        defmodule Foo do
+          def bar(_, %{foo: foo} = _, _) do
+          end
+        end
+      end
+      """,
+      """
+      defmodule Credo.SampleTwo do
+        defmodule Foo do
+          def bar(list) do
+            Enum.map(list, fn _ -> 1 end)
+          end
+        end
+      end
+      """
+    ]
+    |> to_source_files
+    |> refute_issues(@described_check)
+  end
+
+  test "it should NOT report correct behaviour (only one unused variable)" do
+    [
+      """
+      defmodule Credo.SampleOne do
+        defmodule Foo do
+          def bar(_, _, _) do
+          end
+        end
+      end
+      """,
+      """
+      defmodule Credo.SampleTwo do
+      end
+      """
+    ]
+    |> to_source_files
+    |> refute_issues(@described_check)
+  end
+
+  test "it should report a violation for different naming schemes (expects anonymous)" do
+    [
+      """
+      defmodule Credo.SampleOne do
+        defmodule Foo do
+          def bar(_, _, _) do
+          end
+        end
+      end
+      """,
+      """
+      defmodule Credo.SampleTwo do
+        defmodule Foo do
+          def bar(list) do
+            Enum.map(list, fn _item -> 1 end)
+          end
+        end
+      end
+      """
+    ]
+    |> to_source_files
+    |> assert_issue(@described_check, fn issue ->
+      assert "_item" == issue.trigger
+      assert 4 == issue.line_no
+    end)
+  end
+
+  test "it should report a violation for different naming schemes (expects meaningful)" do
+    [
+      """
+      defmodule Credo.SampleOne do
+        defmodule Foo do
+          def bar(name, _) do
+            case name do
+              "foo" <> _name -> "FOO"
+              "bar" <> _name -> "BAR"
+              _name -> "DEFAULT"
+            end
+          end
+        end
+      end
+      """,
+      """
+      defmodule Credo.SampleTwo do
+        defmodule Foo do
+          def bar(list) do
+            Enum.map(list, fn _item -> 1 end)
+          end
+        end
+      end
+      """
+    ]
+    |> to_source_files
+    |> assert_issue(@described_check, fn issue ->
+      assert "_" == issue.trigger
+      assert 3 == issue.line_no
+    end)
+  end
+end