Merge pull request #55 from esl/spec-attr-check

Introduce a module for checks specific to Elixir

Author: erszcz

lib/gradient.ex

@@ -10,6 +10,7 @@ defmodule Gradient do
   alias Gradient.ElixirFileUtils
   alias Gradient.ElixirFmt
   alias Gradient.AstSpecifier
+  alias Gradient.ElixirChecker
 
   require Logger
 
@@ -25,7 +26,7 @@ defmodule Gradient do
         |> put_code_path(opts)
         |> AstSpecifier.specify()
 
-      case :gradualizer.type_check_forms(forms, opts) do
+      case ElixirChecker.check(forms, opts) ++ :gradualizer.type_check_forms(forms, opts) do
         [] ->
           :ok
 

lib/gradient/elixir_checker.ex

@@ -0,0 +1,89 @@
+defmodule Gradient.ElixirChecker do
+  @moduledoc ~s"""
+  Provide checks specific to Elixir that complement type checking delivered by Gradient.
+
+  Options:
+  - {`ex_check`, boolean()}: whether to use checks specific only to Elixir.
+  """
+
+  @spec check([:erl_parse.abstract_form()], keyword()) :: [{:file.filename(), any()}]
+  def check(forms, opts) do
+    if Keyword.get(opts, :ex_check, true) do
+      check_spec(forms)
+    else
+      []
+    end
+  end
+
+  @doc ~s"""
+  Check if all specs are exactly before the function that they specify
+  and if there is only one spec per function clause.
+
+  Correct spec locations:
+    ```
+    @spec convert(integer()) :: float()
+    def convert(int) when is_integer(int), do: int / 1
+
+    @spec convert(atom()) :: binary()
+    def convert(atom) when is_atom(atom), do: to_string(atom)
+    ```
+
+  Incorrect spec locations:
+    - More than one spec above function clause.
+    ```
+    @spec convert(integer()) :: float()
+    @spec convert(atom()) :: binary()
+    def convert(int) when is_integer(int), do: int / 1
+
+    def convert(atom) when is_atom(atom), do: to_string(atom)
+    ```
+
+    - Spec name doesn't match the function name.
+    ```
+    @spec last_two(atom()) :: atom()
+    def last_three(:ok) do
+      :ok
+    end
+    ```
+  """
+  @spec check_spec([:erl_parse.abstract_form()]) :: [{:file.filename(), any()}]
+  def check_spec([{:attribute, _, :file, {file, _}} | forms]) do
+    forms
+    |> Stream.filter(&is_fun_or_spec?/1)
+    |> Stream.map(&simplify_form/1)
+    |> Stream.concat()
+    |> Stream.filter(&has_line/1)
+    |> Enum.sort(&(elem(&1, 2) < elem(&2, 2)))
+    |> Enum.reduce({nil, []}, fn
+      {:fun, fna, _} = fun, {{:spec, {n, a} = sna, anno}, errors} when fna != sna ->
+        # Spec name doesn't match the function name
+        {fun, [{:spec_error, :wrong_spec_name, anno, n, a} | errors]}
+
+      {:spec, {n, a}, anno} = s1, {{:spec, _, _}, errors} ->
+        # Only one spec per function clause is allowed
+        {s1, [{:spec_error, :spec_after_spec, anno, n, a} | errors]}
+
+      x, {_, errors} ->
+        {x, errors}
+    end)
+    |> elem(1)
+    |> Enum.map(&{file, &1})
+  end
+
+  # Filter out __info__ generated function
+  def has_line(form), do: :erl_anno.line(elem(form, 2)) > 1
+
+  def is_fun_or_spec?({:attribute, _, :spec, _}), do: true
+  def is_fun_or_spec?({:function, _, _, _, _}), do: true
+  def is_fun_or_spec?(_), do: false
+
+  @spec simplify_form(:erl_parse.abstract_form()) ::
+          Enumerable.t({:spec | :fun, {atom(), integer()}, :erl_anno.anno()})
+  def simplify_form({:attribute, _, :spec, {{name, arity}, types}}) do
+    Stream.map(types, &{:spec, {name, arity}, elem(&1, 1)})
+  end
+
+  def simplify_form({:function, _, name, arity, clauses}) do
+    Stream.map(clauses, &{:fun, {name, arity}, elem(&1, 1)})
+  end
+end

lib/gradient/elixir_fmt.ex

@@ -69,6 +69,33 @@ defmodule Gradient.ElixirFmt do
     format_expr_type_error(expression, actual_type, expected_type, opts)
   end
 
+  def format_type_error(
+        {:spec_error, :wrong_spec_name, anno, name, arity},
+        opts
+      ) do
+    :io_lib.format(
+      "~sThe spec ~p/~p~s doesn't match the function name/arity~n",
+      [
+        format_location(anno, :brief, opts),
+        name,
+        arity,
+        format_location(anno, :verbose, opts)
+      ]
+    )
+  end
+
+  def format_type_error({:spec_error, :spec_after_spec, anno, name, arity}, opts) do
+    :io_lib.format(
+      "~sThe spec ~p/~p~s follows another spec, but only one spec per function clause is allowed~n",
+      [
+        format_location(anno, :brief, opts),
+        name,
+        arity,
+        format_location(anno, :verbose, opts)
+      ]
+    )
+  end
+
   def format_type_error({:call_undef, anno, module, func, arity}, opts) do
     :io_lib.format(
       "~sCall to undefined function ~s~p/~p~s~n",

test/examples/Elixir.CorrectSpec.beam

@@ -0,0 +1,6 @@
+defmodule SpecAfterSpec do
+  @spec convert(integer()) :: float()
+  @spec convert(atom()) :: binary()
+  def convert(int) when is_integer(int), do: int / 1
+  def convert(atom) when is_atom(atom), do: to_string(atom)
+end

test/examples/Elixir.SpecAfterSpec.beam

@@ -0,0 +1,6 @@
+defmodule CorrectSpec do
+  @spec convert(integer()) :: float()
+  def convert(int) when is_integer(int), do: int / 1
+  @spec convert(atom()) :: binary()
+  def convert(atom) when is_atom(atom), do: to_string(atom)
+end

test/examples/Elixir.SpecWrongName.beam

@@ -0,0 +1,15 @@
+defmodule SpecWrongName do
+  @spec convert(integer()) :: float()
+  def convert(int) when is_integer(int), do: int / 1
+
+  @spec convert(atom()) :: binary()
+  def last_two(list) do
+    [last, penultimate | _tail] = Enum.reverse(list)
+    [penultimate, last]
+  end
+
+  @spec last_two(atom()) :: atom()
+  def last_three(:ok) do
+    :ok
+  end
+end

test/examples/spec_after_spec.ex

@@ -1585,8 +1585,10 @@ defmodule Gradient.AstSpecifierTest do
 
     assert {:function, 3, :name, 0, [{:clause, 3, [], [], [{:atom, 4, :module_a}]}]} =
              List.last(AstSpecifier.run_mappers(astA, tokensA))
+
     assert {:function, 9, :name, 0, [{:clause, 9, [], [], [{:atom, 10, :module_b}]}]} =
              List.last(AstSpecifier.run_mappers(astB, tokensB))
+
     assert {:function, 14, :name, 0, [{:clause, 14, [], [], [{:atom, 15, :module}]}]} =
              List.last(AstSpecifier.run_mappers(ast, tokens))
   end