Make sure @impl warns even when @impl is first-used later

3 files changed, 150 insertions, 130 deletions

diff --git a/lib/elixir/lib/module.ex b/lib/elixir/lib/module.ex
index 3025db070..dc6aa210b 100644
--- a/lib/elixir/lib/module.ex
+++ b/lib/elixir/lib/module.ex
@@ -906,8 +906,11 @@ defmodule Module do
         raise ArgumentError, "cannot pass module #{inspect(behaviour)} as argument to defoverridable/1 because #{error_explanation}"
     end
 
-    behaviour_callbacks = for callback <- behaviour.behaviour_info(:callbacks),
-                          do: normalize_macro_or_function_callback(callback)
+    behaviour_callbacks =
+      for callback <- behaviour.behaviour_info(:callbacks) do
+        {pair, _kind} = normalize_macro_or_function_callback(callback)
+        pair
+      end
 
     tuples = for function_tuple <- definitions_in(module),
                  function_tuple in behaviour_callbacks,
@@ -935,8 +938,8 @@ defmodule Module do
   defp normalize_macro_or_function_callback({function_name, arity}) do
     case :erlang.atom_to_list(function_name) do
       # Macros are always provided one extra argument in behaviour_info
-      'MACRO-' ++ tail -> {:erlang.list_to_atom(tail), arity - 1}
-      _ -> {function_name, arity}
+      'MACRO-' ++ tail -> {{:erlang.list_to_atom(tail), arity - 1}, :defmacro}
+      _ -> {{function_name, arity}, :def}
     end
   end
 
@@ -1147,96 +1150,84 @@ defmodule Module do
   end
 
   @doc false
-  # Used internally to check the validity of arguments to @impl and warn if the
-  # function does not correctly implement a callback.
+  # Used internally to check the validity of arguments to @impl.
   # This function is private and must be used only internally.
-  def check_impl(env, kind, name, args, _guards, _body) do
+  def compile_impl(env, kind, name, args, _guards, _body) do
     %{module: module, line: line, file: file} = env
     table = data_table_for(module)
-    value = get_impl_info(table)
 
-    arity = length(args)
-    pair = {name, arity}
-    {required?, callbacks} = impl_required_and_callbacks(table, value)
+    case :ets.take(table, :impl) do
+      [{:impl, value, _, _}] ->
+        impls = :ets.lookup_element(table, {:elixir, :impls}, 2)
+        impl = {{name, length(args)}, kind, line, file, value}
+        :ets.insert(table, {{:elixir, :impls}, [impl | impls]})
+      [] ->
+        :ok
+    end
+
+    :ok
+  end
+
+  @doc false
+  def check_behaviours_and_impls(env, table, all_definitions) do
+    behaviours = :ets.lookup_element(table, :behaviour, 2)
+    impls = :ets.lookup_element(table, {:elixir, :impls}, 2)
 
-    case impl_warn(table, kind, pair, value, required?, callbacks) do
-      :ok -> :ok
-      {:error, message} -> :elixir_errors.warn line, file, message
+    if impls != [] do
+      non_implemented_callbacks = check_impls(behaviours, impls)
+      warn_missing_impls(env, non_implemented_callbacks, all_definitions)
     end
   end
 
-  defp precompute_behaviour_callbacks(table, behaviour) do
-    [{{:elixir, :behaviours}, required?, computed}] = :ets.lookup(table, {:elixir, :behaviours})
+  defp check_impls(behaviours, impls) do
+    callbacks = callbacks_with_behaviour(behaviours)
 
-    computed =
-      if function_exported?(behaviour, :behaviour_info, 1) do
-        :lists.foldl(fn pair, acc ->
-          :maps.put(normalize_macro_or_function_callback(pair), behaviour, acc)
-        end, computed, behaviour.behaviour_info(:callbacks))
-      else
-        computed
+    Enum.reduce(impls, callbacks, fn {pair, kind, line, file, value}, acc ->
+      case impl_warn(pair, kind, value, behaviours, callbacks) do
+        :ok -> :ok
+        {:error, message} -> :elixir_errors.warn(line, file, message)
       end
-
-    :ets.insert(table, {{:elixir, :behaviours}, required?, computed})
+      Map.delete(acc, {pair, kind})
+    end)
   end
 
-  defp impl_required_and_callbacks(table, value) do
-    [{{:elixir, :behaviours}, required?, computed}] = :ets.lookup(table, {:elixir, :behaviours})
-    if value && not required? do
-      :ets.update_element(table, {:elixir, :behaviours}, {2, true})
-    end
-    {required?, computed}
+  defp callbacks_with_behaviour(behaviours) do
+    for behaviour <- behaviours,
+        function_exported?(behaviour, :behaviour_info, 1),
+        callback <- behaviour.behaviour_info(:callbacks),
+        do: {normalize_macro_or_function_callback(callback), behaviour},
+        into: %{}
   end
 
-  defp impl_warn(_, kind, _, nil, _, _) when kind in [:defp, :defmacrop] do
-    :ok
-  end
-  defp impl_warn(_, kind, pair, _, _, _) when kind in [:defp, :defmacrop] do
+  defp impl_warn(pair, kind, _, _, _) when kind in [:defp, :defmacrop] do
     {:error, "#{format_kind_pair(kind, pair)} is private, @impl is always discarded for private functions/macros"}
   end
-  defp impl_warn(_, _, _, nil, required?, _) when not required? do
-    :ok
+  defp impl_warn(pair, kind, value, [], _callbacks) do
+    {:error, "got @impl #{inspect value} for #{format_kind_pair(kind, pair)} but no behaviour was declared"}
   end
-  defp impl_warn(_, kind, pair, nil, required?, callbacks) when required? do
-    case callbacks do
-      %{^pair => behaviour} ->
-        message = "module attribute @impl was not set for callback " <>
-                  "#{format_kind_pair(kind, pair)} (callback specified in #{inspect(behaviour)})." <>
-                  "This either means you forgot to add the \"@impl true\" annotation before the " <>
-                  "definition or that you are accidentally overriding a callback"
-        {:error, message}
-      %{} ->
-        :ok
-    end
-  end
-  defp impl_warn(_, kind, pair, false, _, callbacks) do
-    cond do
-      callbacks == %{} ->
-        message = "got @impl false for #{format_kind_pair(kind, pair)} but there are no known callbacks"
-        {:error, message}
-      behaviour = Map.get(callbacks, pair) ->
-        message = "got @impl false for #{format_kind_pair(kind, pair)} " <>
-                  "but it is a callback specified in #{inspect(behaviour)}"
-        {:error, message}
-      true ->
-        :ok
+  defp impl_warn(pair, kind, false, _behaviours, callbacks) do
+    if behaviour = Map.get(callbacks, {pair, kind}) do
+      message = "got @impl false for #{format_kind_pair(kind, pair)} " <>
+                "but it is a callback specified in #{inspect(behaviour)}"
+      {:error, message}
+    else
+      :ok
     end
   end
-  defp impl_warn(_, kind, pair, true, _, callbacks) do
-    cond do
-      Map.has_key?(callbacks, pair) ->
-        :ok
-      true ->
-        message = "got @impl true for #{format_kind_pair(kind, pair)} " <>
-                  "but no behaviour specifies this callback#{known_callbacks(callbacks)}"
-        {:error, message}
+  defp impl_warn(pair, kind, true, _, callbacks) do
+    if Map.has_key?(callbacks, {pair, kind}) do
+      :ok
+    else
+      message = "got @impl true for #{format_kind_pair(kind, pair)} " <>
+                "but no behaviour specifies this callback#{known_callbacks(callbacks)}"
+      {:error, message}
     end
   end
-  defp impl_warn(table, kind, pair, behaviour, _, callbacks) do
+  defp impl_warn(pair, kind, behaviour, behaviours, callbacks) do
     cond do
-      {pair, behaviour} in callbacks ->
+      Map.get(callbacks, {pair, kind}) == behaviour ->
         :ok
-      behaviour not in :ets.lookup_element(table, :behaviour, 2) ->
+      behaviour not in behaviours ->
         message = "got @impl #{inspect behaviour} for #{format_kind_pair(kind, pair)} " <>
                   "but the given behaviour was not declared with @behaviour"
         {:error, message}
@@ -1247,6 +1238,23 @@ defmodule Module do
     end
   end
 
+  defp warn_missing_impls(_env, callbacks, _defs) when map_size(callbacks) == 0 do
+    :ok
+  end
+  defp warn_missing_impls(env, non_implemented_callbacks, defs) do
+    for {pair, kind, meta, _clauses} <- defs,
+        kind in [:def, :defmacro],
+        behaviour = Map.get(non_implemented_callbacks, {pair, kind}) do
+      message = "module attribute @impl was not set for callback " <>
+                "#{format_kind_pair(kind, pair)} (callback specified in #{inspect behaviour}). " <>
+                "This either means you forgot to add the \"@impl true\" annotation before the " <>
+                "definition or that you are accidentally overriding a callback"
+      :elixir_errors.warn(:elixir_utils.get_line(meta), env.file, message)
+    end
+
+    :ok
+  end
+
   defp format_kind_pair(kind, {name, arity}) do
     "#{kind} #{name}/#{arity}"
   end
@@ -1256,8 +1264,8 @@ defmodule Module do
     "and make sure they define callbacks"
   end
   defp known_callbacks(callbacks) do
-    formatted = for {{name, arity}, module} <- callbacks do
-      "\n  * " <> Exception.format_mfa(module, name, arity)
+    formatted = for {{{name, arity}, kind}, module} <- callbacks do
+      "\n  * " <> Exception.format_mfa(module, name, arity) <> "(#{kind})"
     end
     ". The known callbacks are:\n#{formatted}"
   end
@@ -1316,10 +1324,6 @@ defmodule Module do
         :ok
     end
 
-    if key == :behaviour do
-      precompute_behaviour_callbacks(table, value)
-    end
-
     case :ets.lookup(table, key) do
       [{^key, {line, <<_::binary>>}, accumulated?, _unread_line}]
           when key in [:doc, :typedoc, :moduledoc] and is_list(stack) ->
@@ -1369,7 +1373,7 @@ defmodule Module do
     case value do
       bool when is_boolean(bool) ->
         value
-      module when is_atom(module)  ->
+      module when is_atom(module) and module != nil  ->
         # Attempt to compile behaviour but ignore failure (will warn later)
         _ = Code.ensure_compiled(module)
         value
@@ -1417,13 +1421,6 @@ defmodule Module do
     end
   end
 
-  defp get_impl_info(table) do
-    case :ets.take(table, :impl) do
-      [{:impl, value, _, _}] -> value
-      [] -> nil
-    end
-  end
-
   defp data_table_for(module) do
     :elixir_module.data_table(module)
   end
diff --git a/lib/elixir/src/elixir_module.erl b/lib/elixir/src/elixir_module.erl
index 1582218d6..197e5326d 100644
--- a/lib/elixir/src/elixir_module.erl
+++ b/lib/elixir/src/elixir_module.erl
@@ -78,6 +78,8 @@ compile(Line, Module, Block, Vars, E) ->
     [elixir_locals:record_local(Tuple, Module) || Tuple <- OnLoad],
 
     {AllDefinitions, Unreachable} = elixir_def:fetch_definitions(File, Module),
+    (not elixir_compiler:get_opt(internal)) andalso
+     'Elixir.Module':check_behaviours_and_impls(E, Data, AllDefinitions),
     CompileOpts = lists:flatten(ets:lookup_element(Data, compile, 2)),
 
     ModuleMap = #{
@@ -177,7 +179,7 @@ build(Line, File, Module, Lexical) ->
   ImplOnDefinition =
       case elixir_compiler:get_opt(internal) of
         true -> [{elixir_module, delete_impl}];
-        _    -> [{'Elixir.Module', check_impl}]
+        _    -> [{'Elixir.Module', compile_impl}]
       end,
 
   %% Docs must come first as they read the impl callback.
@@ -206,7 +208,7 @@ build(Line, File, Module, Lexical) ->
     {typep, [], true, nil},
 
     % Internal
-    {{elixir, behaviours}, false, #{}}
+    {{elixir, impls}, []}
   ]),
 
   Persisted = [behaviour, on_load, compile, external_resource, dialyzer, vsn],
diff --git a/lib/elixir/test/elixir/kernel/impl_test.exs b/lib/elixir/test/elixir/kernel/impl_test.exs
index 7fa208e1f..7f619e4d5 100644
--- a/lib/elixir/test/elixir/kernel/impl_test.exs
+++ b/lib/elixir/test/elixir/kernel/impl_test.exs
@@ -24,13 +24,12 @@ defmodule Kernel.ImplTest do
     @macrocallback bar :: any
   end
 
-  test "sets impl to false" do
+  test "sets impl to boolean" do
     defmodule ImplAttributes do
       @behaviour Behaviour
 
-      def foo() do
-        :ok
-      end
+      @impl true
+      def foo(), do: :ok
 
       @impl false
       def foo(term) do
@@ -39,24 +38,12 @@ defmodule Kernel.ImplTest do
     end
   end
 
-  test "sets impl to true" do
-    defmodule ImplAttributes do
-      @behaviour Behaviour
-
-      @impl true
-      def foo() do
-        :ok
-      end
-    end
-  end
-
   test "sets impl to nil" do
-    defmodule ImplAttributes do
-      @behaviour Behaviour
-
-      @impl nil
-      def foo() do
-        :ok
+    assert_raise ArgumentError, ~r/expected impl attribute to contain a module or a boolean/, fn ->
+      defmodule ImplAttributes do
+        @behaviour Behaviour
+        @impl nil
+        def foo(), do: :ok
       end
     end
   end
@@ -64,21 +51,15 @@ defmodule Kernel.ImplTest do
   test "sets impl to behaviour" do
     defmodule ImplAttributes do
       @behaviour Behaviour
-
       @impl Behaviour
-      def foo() do
-        :ok
-      end
+      def foo(), do: :ok
     end
   end
 
   test "does not set impl" do
     defmodule ImplAttributes do
       @behaviour Behaviour
-
-      def foo() do
-        :ok
-      end
+      def foo(), do: :ok
     end
   end
 
@@ -95,6 +76,38 @@ defmodule Kernel.ImplTest do
     end) =~ "got @impl :abc for def foo/0 but the behaviour does not specify this callback. There are no known callbacks"
   end
 
+  test "warns for callbacks without impl and @impl has been set before" do
+    assert capture_err(fn ->
+      Code.eval_string """
+      defmodule Kernel.ImplTest.ImplAttributes do
+        @behaviour Kernel.ImplTest.Behaviour
+        @behaviour Kernel.ImplTest.MacroBehaviour
+
+        @impl true
+        def foo(), do: :ok
+
+        defmacro bar(), do: :ok
+      end
+      """
+    end) =~ "module attribute @impl was not set for callback defmacro bar/0 (callback specified in Kernel.ImplTest.MacroBehaviour)"
+  end
+
+  test "warns for callbacks without impl and @impl has been set after" do
+    assert capture_err(fn ->
+      Code.eval_string """
+      defmodule Kernel.ImplTest.ImplAttributes do
+        @behaviour Kernel.ImplTest.Behaviour
+        @behaviour Kernel.ImplTest.MacroBehaviour
+
+        defmacro bar(), do: :ok
+
+        @impl true
+        def foo(), do: :ok
+      end
+      """
+    end) =~ "module attribute @impl was not set for callback defmacro bar/0 (callback specified in Kernel.ImplTest.MacroBehaviour)"
+  end
+
   test "warns when impl is set on private function" do
     assert capture_err(fn ->
       Code.eval_string """
@@ -126,7 +139,7 @@ defmodule Kernel.ImplTest do
         def foo(), do: :ok
       end
       """
-    end) =~ "got @impl true for def foo/0 but no behaviour specifies this callback. There are no known callbacks"
+    end) =~ "got @impl true for def foo/0 but no behaviour was declared"
   end
 
   test "warns for @impl true with callback name not in behaviour" do
@@ -153,32 +166,28 @@ defmodule Kernel.ImplTest do
     end) =~ "got @impl true for defmacro foo/0 but no behaviour specifies this callback"
   end
 
-  test "warns for @impl true with wrong arity" do
+  test "warns for @impl true with callback kind not in behaviour" do
     assert capture_err(fn ->
       Code.eval_string """
       defmodule Kernel.ImplTest.ImplAttributes do
-        @behaviour Kernel.ImplTest.Behaviour
+        @behaviour Kernel.ImplTest.MacroBehaviour
         @impl true
-        def foo(arg), do: arg
+        def foo(), do: :ok
       end
       """
-    end) =~ "got @impl true for def foo/1 but no behaviour specifies this callback"
+    end) =~ "got @impl true for def foo/0 but no behaviour specifies this callback"
   end
 
-  test "warns for callbacks without impl and @impl has been set before" do
+  test "warns for @impl true with wrong arity" do
     assert capture_err(fn ->
       Code.eval_string """
       defmodule Kernel.ImplTest.ImplAttributes do
         @behaviour Kernel.ImplTest.Behaviour
-        @behaviour Kernel.ImplTest.MacroBehaviour
-
         @impl true
-        def foo(), do: :ok
-
-        defmacro bar(), do: :ok
+        def foo(arg), do: arg
       end
       """
-    end) =~ "module attribute @impl was not set for callback defmacro bar/0 (callback specified in Kernel.ImplTest.MacroBehaviour)"
+    end) =~ "got @impl true for def foo/1 but no behaviour specifies this callback"
   end
 
   test "warns for @impl false and there are no callbacks" do
@@ -189,7 +198,7 @@ defmodule Kernel.ImplTest do
         def baz(term), do: term
       end
       """
-    end) =~ "got @impl false for def baz/1 but there are no known callbacks"
+    end) =~ "got @impl false for def baz/1 but no behaviour was declared"
   end
 
   test "warns for @impl false and it is a callback" do
@@ -212,7 +221,7 @@ defmodule Kernel.ImplTest do
         def foo(), do: :ok
       end
       """
-    end) =~ "got @impl Kernel.ImplTest.Behaviour for def foo/0 but the given behaviour was not declared with @behaviour"
+    end) =~ "got @impl Kernel.ImplTest.Behaviour for def foo/0 but no behaviour was declared"
   end
 
   test "warns for @impl module with callback name not in behaviour" do
@@ -239,6 +248,18 @@ defmodule Kernel.ImplTest do
     end) =~ "got @impl Kernel.ImplTest.MacroBehaviour for defmacro foo/0 but the behaviour does not specify this callback"
   end
 
+  test "warns for @impl module with macro callback kind not in behaviour" do
+    assert capture_err(fn ->
+      Code.eval_string """
+      defmodule Kernel.ImplTest.ImplAttributes do
+        @behaviour Kernel.ImplTest.MacroBehaviour
+        @impl Kernel.ImplTest.MacroBehaviour
+        def foo(), do: :ok
+      end
+      """
+    end) =~ "got @impl Kernel.ImplTest.MacroBehaviour for def foo/0 but the behaviour does not specify this callback"
+  end
+
   test "warns for @impl module and callback belongs to another known module" do
     assert capture_err(fn ->
       Code.eval_string """