diff --git a/.credo.exs b/.credo.exs index 7bc855a8f..f37cd8ab7 100644 --- a/.credo.exs +++ b/.credo.exs @@ -110,6 +110,7 @@ {Credo.Check.Readability.RedundantBlankLines, []}, {Credo.Check.Readability.Semicolons, []}, {Credo.Check.Readability.SpaceAfterCommas, []}, + {Credo.Check.Readability.SpecParameterNames, []}, {Credo.Check.Readability.StringSigils, []}, {Credo.Check.Readability.TrailingBlankLine, []}, {Credo.Check.Readability.TrailingWhiteSpace, []}, diff --git a/lib/credo/check/readability/spec_parameter_names.ex b/lib/credo/check/readability/spec_parameter_names.ex new file mode 100644 index 000000000..678c17f56 --- /dev/null +++ b/lib/credo/check/readability/spec_parameter_names.ex @@ -0,0 +1,101 @@ +defmodule Credo.Check.Readability.SpecParameterNames do + use Credo.Check, + id: "EX3037", + base_priority: :low, + explanations: [ + check: """ + Parameters in `@spec` and `@callback` declarations should be named using + the `name :: type` syntax. Naming parameters makes specs self-documenting: + readers and ExDoc output see what each argument is for, not just its type. + This is especially valuable when several parameters share the same type. + + # preferred + + @spec create_user(attrs :: map(), email :: String.t()) :: {:ok, User.t()} + + @callback handle_event(event :: String.t(), params :: map(), socket :: Socket.t()) :: + {:noreply, Socket.t()} + + # NOT preferred + + @spec create_user(map(), String.t()) :: {:ok, User.t()} + + @callback handle_event(String.t(), map(), Socket.t()) :: {:noreply, Socket.t()} + + Like all `Readability` issues, this one is not a technical concern. + But you can improve the odds of others reading and liking your code by making + it easier to follow. + """ + ] + + @doc false + @impl true + def run(%SourceFile{} = source_file, params) do + ctx = Context.build(source_file, params, __MODULE__) + result = Credo.Code.prewalk(source_file, &walk/2, ctx) + result.issues + end + + defp walk({:@, _meta, [{attr_type, _attr_meta, [spec_ast]}]} = ast, ctx) + when attr_type in [:spec, :callback] do + {ast, check_spec(spec_ast, ctx)} + end + + defp walk(ast, ctx) do + {ast, ctx} + end + + # Guard-style spec: `@spec foo(args) :: return when ...` + defp check_spec( + {:when, _meta, [{:"::", _inner_meta, [fun_call, _return_type]} | _guards]}, + ctx + ) do + check_fun_args(fun_call, ctx) + end + + # Regular spec: `@spec foo(args) :: return` + defp check_spec({:"::", _meta, [fun_call, _return_type]}, ctx) do + check_fun_args(fun_call, ctx) + end + + defp check_spec(_other, ctx), do: ctx + + defp check_fun_args({_fun_name, _meta, args}, ctx) when is_list(args) do + Enum.reduce(args, ctx, &check_arg/2) + end + + defp check_fun_args({_fun_name, _meta, nil}, ctx), do: ctx + defp check_fun_args(_other, ctx), do: ctx + + # Named param: `name :: type` where name is an atom variable (nil context) + defp check_arg({:"::", _meta, [{name, _name_meta, nil}, _type]}, ctx) + when is_atom(name) do + ctx + end + + defp check_arg(arg, ctx) do + put_issue(ctx, issue_for(ctx, arg)) + end + + defp issue_for(ctx, arg) do + format_issue( + ctx, + message: "Spec parameter is missing a name. Use `name :: type` syntax.", + trigger: trigger_for(arg), + line_no: line_for(arg) + ) + end + + defp line_for({_name, meta, _arg_list}) when is_list(meta), do: meta[:line] + + defp line_for({{:., meta, _dot_args}, _call_meta, _call_args}) when is_list(meta), + do: meta[:line] + + defp line_for(_other), do: nil + + defp trigger_for({{:., _dot_meta, [{:__aliases__, _alias_meta, aliases}, fun]}, _call_meta, _call_args}), + do: Enum.map_join(aliases, ".", &to_string/1) <> ".#{fun}" + + defp trigger_for({name, _meta, _args}) when is_atom(name), do: to_string(name) + defp trigger_for(_other), do: "?" +end diff --git a/test/credo/check/readability/spec_parameter_names_test.exs b/test/credo/check/readability/spec_parameter_names_test.exs new file mode 100644 index 000000000..84691bb1e --- /dev/null +++ b/test/credo/check/readability/spec_parameter_names_test.exs @@ -0,0 +1,149 @@ +defmodule Credo.Check.Readability.SpecParameterNamesTest do + use Credo.Test.Case + + @described_check Credo.Check.Readability.SpecParameterNames + + # + # cases NOT raising issues + # + + test "it should NOT report a spec with all named parameters" do + ~S''' + defmodule CredoSampleModule do + @spec create_user(attrs :: map(), email :: String.t()) :: {:ok, term()} + def create_user(attrs, email), do: {:ok, {attrs, email}} + end + ''' + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end + + test "it should NOT report a zero-arity spec" do + ~S''' + defmodule CredoSampleModule do + @spec config() :: keyword() + def config, do: [] + end + ''' + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end + + test "it should NOT report a guard-style spec with named parameters" do + ~S''' + defmodule CredoSampleModule do + @spec foo(value :: x, opts :: keyword()) :: x when x: term() + def foo(value, _opts), do: value + end + ''' + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end + + test "it should NOT report a @callback with named parameters" do + ~S''' + defmodule CredoSampleBehaviour do + @callback handle_event(event :: String.t(), params :: map()) :: :ok + end + ''' + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end + + test "it should NOT report named parameters using remote types" do + ~S''' + defmodule CredoSampleModule do + @spec list(scope :: Scope.t(), opts :: Keyword.t()) :: [term()] + def list(_scope, _opts), do: [] + end + ''' + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end + + # + # cases raising issues + # + + test "it should report a spec with a single unnamed parameter" do + ~S''' + defmodule CredoSampleModule do + @spec greet(String.t()) :: String.t() + def greet(name), do: "hello " <> name + end + ''' + |> to_source_file + |> run_check(@described_check) + |> assert_issue(%{trigger: "String.t"}) + end + + test "it should report each unnamed parameter in a spec" do + ~S''' + defmodule CredoSampleModule do + @spec create_user(map(), String.t()) :: {:ok, term()} + def create_user(attrs, email), do: {:ok, {attrs, email}} + end + ''' + |> to_source_file + |> run_check(@described_check) + |> assert_issues(fn issues -> assert length(issues) == 2 end) + end + + test "it should report only the unnamed parameters in a mixed spec" do + ~S''' + defmodule CredoSampleModule do + @spec create_user(attrs :: map(), String.t()) :: {:ok, term()} + def create_user(attrs, email), do: {:ok, {attrs, email}} + end + ''' + |> to_source_file + |> run_check(@described_check) + |> assert_issue() + end + + test "it should report unnamed parameters in a guard-style spec" do + ~S''' + defmodule CredoSampleModule do + @spec foo(x, keyword()) :: x when x: term() + def foo(value, _opts), do: value + end + ''' + |> to_source_file + |> run_check(@described_check) + |> assert_issues(fn issues -> assert length(issues) == 2 end) + end + + test "it should report unnamed parameters in a @callback" do + ~S''' + defmodule CredoSampleBehaviour do + @callback handle_event(String.t(), map()) :: :ok + end + ''' + |> to_source_file + |> run_check(@described_check) + |> assert_issues(fn issues -> + assert length(issues) == 2 + assert Enum.any?(issues, &(&1.trigger == "map")) + end) + end + + test "it should report across multiple specs in one module" do + ~S''' + defmodule CredoSampleModule do + @spec one(integer()) :: integer() + def one(a), do: a + + @spec two(integer(), integer()) :: integer() + def two(a, b), do: a + b + end + ''' + |> to_source_file + |> run_check(@described_check) + |> assert_issues(fn issues -> assert length(issues) == 3 end) + end +end