diff --git a/lib/credo/check/readability/cond_final_condition.ex b/lib/credo/check/readability/cond_final_condition.ex new file mode 100644 index 000000000..045126cda --- /dev/null +++ b/lib/credo/check/readability/cond_final_condition.ex @@ -0,0 +1,116 @@ +defmodule Credo.Check.Readability.CondFinalCondition do + use Credo.Check, + tags: [:controversial], + # Default to the value specified here: + # https://github.com/christopheradams/elixir_style_guide#true-as-last-condition + param_defaults: [value: true], + explanations: [ + check: """ + If a cond expresion ends in an "always true" statement the statement + should be the literal `true`, or the literal value specified in this + check's `value` parameter. + + Example: + + cond do + x == y -> 0 + x > y -> 0 + :else -> 1 + end + + # should be written as + + cond do + x == y -> 0 + x > y -> 0 + true -> 1 + end + + Like all `Readability` issues, this one is not a technical concern. + But you can improve the odds of other reading and liking your code by making + it easier to follow. + """, + params: [ + value: "Set the expected value for the final condition" + ] + ] + + @doc false + @impl true + def run(%SourceFile{} = source_file, params) do + issue_meta = IssueMeta.for(source_file, params) + + config_catchall_value = Params.get(params, :value, __MODULE__) + + Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta, config_catchall_value)) + end + + defp traverse({:cond, meta, arguments} = ast, issues, issue_meta, config_catchall_value) do + last_cond_clause = + arguments + |> Credo.Code.Block.do_block_for!() + |> List.wrap() + |> List.last() + + case is_catchall_clause_with_invalid_value?(last_cond_clause, config_catchall_value) do + {true, ast} -> + expression = Macro.to_string(ast) + {ast, issues ++ [issue_for(issue_meta, config_catchall_value, meta[:line], expression)]} + + _ -> + {ast, issues} + end + end + + defp traverse(ast, issues, _issue_meta, _min_pipeline_length) do + {ast, issues} + end + + defp is_catchall_clause_with_invalid_value?({:->, _meta, [[value], _args]}, value), do: false + # Integer literal catch-all clause + defp is_catchall_clause_with_invalid_value?({:->, _meta, [[integer], _args]}, _value) + when is_integer(integer), + do: {true, integer} + + # Binary literal catch-all clause + defp is_catchall_clause_with_invalid_value?({:->, _meta, [[binary], _args]}, _value) + when is_binary(binary), + do: {true, binary} + + # List literal catch-all clause + defp is_catchall_clause_with_invalid_value?({:->, _meta, [[list], _args]}, _value) + when is_list(list), + do: {true, list} + + # Map literal catch-all clause + defp is_catchall_clause_with_invalid_value?( + {:->, _meta, [[{:%{}, _meta2, _values} = ast], _args]}, + _value + ), + do: {true, ast} + + # Tuple literal catch-all clause + defp is_catchall_clause_with_invalid_value?( + {:->, _meta, [[{:{}, _meta2, _values} = ast], _args]}, + _value + ), + do: {true, ast} + + # Atom literal catch-all clause + defp is_catchall_clause_with_invalid_value?({:->, _meta, [[name], _args]}, _value) + when is_atom(name), + do: {true, name} + + # Any other cond clause expression + defp is_catchall_clause_with_invalid_value?(_clause, _value), do: false + + defp issue_for(issue_meta, expected_value, line_no, trigger) do + format_issue( + issue_meta, + message: + "Cond statements that end with an \"always true\" condition should use `#{expected_value}` instead of some other literal.", + trigger: trigger, + line_no: line_no + ) + end +end diff --git a/test/credo/check/readability/cond_final_condition_test.exs b/test/credo/check/readability/cond_final_condition_test.exs new file mode 100644 index 000000000..292e9ac91 --- /dev/null +++ b/test/credo/check/readability/cond_final_condition_test.exs @@ -0,0 +1,236 @@ +defmodule Credo.Check.Readability.CondFinalConditionTest do + use Credo.Test.Case + + @described_check Credo.Check.Readability.CondFinalCondition + + test "it should NOT report conds with a last condition of true" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + true -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check) + |> refute_issues() + end + + test "it should NOT report conds with a last condition that uses a variable" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check) + |> refute_issues() + end + + test "it should NOT report conds with a last condition that match the config" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + :else -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check, value: :else) + |> refute_issues() + end + + test "it should report conds that with a last condition that is some other literal" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + :else -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check) + |> assert_issue(fn %Credo.Issue{trigger: trigger} -> + assert ":else" == trigger + end) + end + + test "it should report conds with a last condition that is a binary literal" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + "else" -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check) + |> assert_issue(fn %Credo.Issue{trigger: trigger} -> + assert "\"else\"" == trigger + end) + end + + test "it should report conds with a last condition that is an integer literal" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + 12345 -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check) + |> assert_issue(fn %Credo.Issue{trigger: trigger} -> + assert "12345" == trigger + end) + end + + test "it should report conds with a last condition that is a list literal" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + [:else] -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check) + |> assert_issue(fn %Credo.Issue{trigger: trigger} -> + assert "[:else]" == trigger + end) + end + + test "it should report conds with a last condition that is a tuple literal" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + {:else} -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check) + |> assert_issue(fn %Credo.Issue{trigger: trigger} -> + assert "{:else}" == trigger + end) + end + + test "it should report conds with a last condition that is a map literal" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + %{} -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check) + |> assert_issue(fn %Credo.Issue{trigger: trigger} -> + assert "%{}" == trigger + end) + end + + test "it should report conds with a last condition that differ from the config" do + """ + defmodule CredoSampleModule do + def cond_true(a) do + cond do + a + 2 == 5 -> + "Nope" + + a + 3 == 5 -> + "Uh, uh" + + true -> + "OK" + end + end + end + """ + |> to_source_file() + |> run_check(@described_check, value: :else) + |> assert_issue(fn %Credo.Issue{trigger: trigger} -> + assert "true" == trigger + end) + end +end