Skip to content
Permalink
Browse files

Add Credo.Check.Consistency.UnusedVariableNames

  • Loading branch information...
remiprev committed Jun 11, 2019
1 parent ab2eea1 commit b4adda4e8e2d45a5eb2438205a37a608ec26a928
@@ -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},
@@ -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
@@ -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
@@ -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

0 comments on commit b4adda4

Please sign in to comment.
You can’t perform that action at this time.