diff --git a/lib/gradient.ex b/lib/gradient.ex index 13008752..7670954b 100644 --- a/lib/gradient.ex +++ b/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 diff --git a/lib/gradient/elixir_checker.ex b/lib/gradient/elixir_checker.ex new file mode 100644 index 00000000..19aae0da --- /dev/null +++ b/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 diff --git a/lib/gradient/elixir_fmt.ex b/lib/gradient/elixir_fmt.ex index 40c6f9bd..0be7abe7 100644 --- a/lib/gradient/elixir_fmt.ex +++ b/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", diff --git a/test/examples/Elixir.CorrectSpec.beam b/test/examples/Elixir.CorrectSpec.beam new file mode 100644 index 00000000..10add2bc Binary files /dev/null and b/test/examples/Elixir.CorrectSpec.beam differ diff --git a/test/examples/Elixir.SpecAfterSpec.beam b/test/examples/Elixir.SpecAfterSpec.beam new file mode 100644 index 00000000..4792ed14 Binary files /dev/null and b/test/examples/Elixir.SpecAfterSpec.beam differ diff --git a/test/examples/Elixir.SpecWrongName.beam b/test/examples/Elixir.SpecWrongName.beam new file mode 100644 index 00000000..87e4030f Binary files /dev/null and b/test/examples/Elixir.SpecWrongName.beam differ diff --git a/test/examples/spec_after_spec.ex b/test/examples/spec_after_spec.ex new file mode 100644 index 00000000..4d8066e8 --- /dev/null +++ b/test/examples/spec_after_spec.ex @@ -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 diff --git a/test/examples/spec_correct.ex b/test/examples/spec_correct.ex new file mode 100644 index 00000000..77b17f83 --- /dev/null +++ b/test/examples/spec_correct.ex @@ -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 diff --git a/test/examples/spec_wrong_name.ex b/test/examples/spec_wrong_name.ex new file mode 100644 index 00000000..7fb51c69 --- /dev/null +++ b/test/examples/spec_wrong_name.ex @@ -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 diff --git a/test/gradient/ast_specifier_test.exs b/test/gradient/ast_specifier_test.exs index 6990a9f1..9ebf53de 100644 --- a/test/gradient/ast_specifier_test.exs +++ b/test/gradient/ast_specifier_test.exs @@ -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 diff --git a/test/gradient/elixir_checker_test.exs b/test/gradient/elixir_checker_test.exs new file mode 100644 index 00000000..75a223ca --- /dev/null +++ b/test/gradient/elixir_checker_test.exs @@ -0,0 +1,38 @@ +defmodule Gradient.ElixirCheckerTest do + use ExUnit.Case + doctest Gradient.ElixirChecker + + alias Gradient.ElixirChecker + + import Gradient.TestHelpers + + test "checker options" do + ast = load("Elixir.SpecWrongName.beam") + + assert [] = ElixirChecker.check(ast, ex_check: false) + assert [] != ElixirChecker.check(ast, ex_check: true) + end + + test "all specs are correct" do + ast = load("Elixir.CorrectSpec.beam") + + assert [] = ElixirChecker.check(ast, ex_check: true) + end + + test "spec name doesn't match the function name" do + ast = load("Elixir.SpecWrongName.beam") + + assert [ + {_, {:spec_error, :wrong_spec_name, 11, :last_two, 1}}, + {_, {:spec_error, :wrong_spec_name, 5, :convert, 1}} + ] = ElixirChecker.check(ast, []) + end + + test "more than one spec per function clause is not allowed" do + ast = load("Elixir.SpecAfterSpec.beam") + + assert [ + {_, {:spec_error, :spec_after_spec, 3, :convert_a, 1}} + ] = ElixirChecker.check(ast, []) + end +end diff --git a/test/gradient/elixir_fmt_test.exs b/test/gradient/elixir_fmt_test.exs index 4757381a..2e10c3e2 100644 --- a/test/gradient/elixir_fmt_test.exs +++ b/test/gradient/elixir_fmt_test.exs @@ -239,6 +239,27 @@ defmodule Gradient.ElixirFmtTest do end end + describe "spec" do + test "name doesn't match the function name" do + msg = + {:spec_error, :wrong_spec_name, 3, :convert, 1} + |> ElixirFmt.format_error([]) + |> :erlang.iolist_to_binary() + + assert "The spec convert/1 on line 3 doesn't match the function name/arity\n" = msg + end + + test "follows another spec" do + msg = + {:spec_error, :spec_after_spec, 3, :convert, 1} + |> ElixirFmt.format_error([]) + |> :erlang.iolist_to_binary() + + assert "The spec convert/1 on line 3 follows another spec, but only one spec per function clause is allowed\n" = + msg + end + end + @tag :skip test "format_expr_type_error/4" do opts = [forms: basic_erlang_forms()] diff --git a/test/gradient/tokens_test.exs b/test/gradient/tokens_test.exs index 8b0d7386..a9f7f453 100644 --- a/test/gradient/tokens_test.exs +++ b/test/gradient/tokens_test.exs @@ -67,8 +67,7 @@ defmodule Gradient.TokensTest do end test "unless" do - {tokens, _ast} = - load("conditional/Elixir.Conditional.Unless.beam", "conditional/unless.ex") + {tokens, _ast} = load("conditional/Elixir.Conditional.Unless.beam", "conditional/unless.ex") tokens = Tokens.drop_tokens_to_line(tokens, 2) opts = [end_line: -1] diff --git a/test/support/helpers.ex b/test/support/helpers.ex index 8017da7e..2203dfbd 100644 --- a/test/support/helpers.ex +++ b/test/support/helpers.ex @@ -3,6 +3,16 @@ defmodule Gradient.TestHelpers do @examples_path "test/examples/" + @spec load(String.t()) :: T.forms() + def load(beam_file) do + beam_file = String.to_charlist(@examples_path <> beam_file) + + {:ok, {_, [abstract_code: {:raw_abstract_v1, ast}]}} = + :beam_lib.chunks(beam_file, [:abstract_code]) + + ast + end + @spec load(String.t(), String.t()) :: {T.tokens(), T.forms()} def load(beam_file, ex_file) do beam_file = String.to_charlist(@examples_path <> beam_file)