Add Readability.SpecParameterNames check

.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, []},

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

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