Introduce @impl attribute and improve defoverridable #5734

Closed
josevalim opened this Issue Feb 4, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@josevalim
Member

josevalim commented Feb 4, 2017

One of the features added to Elixir early on to help integration with Erlang code was the idea of overridable function definitions. This is what allowed our GenServer definition to be as simple as:

defmodule MyServer do
  use GenServer
end

Implementation-wise, use GenServer defines functions such as:

def terminate(reason, state) do
  :ok
end

and then mark them as overridable:

defoverridable terminate: 2

The usage of defoverridable/1 and @optional_callbacks have one major downside: the lack of warnings for implementation mismatches. For example, imagine that instead of defining handle_call/3, you accidentally define a non-callback handle_call/2. Because handle_call/3 is optional, Elixir won't emit any warnings, so it may take a while for developers to understand why their handle_call/2 callback is not being invoked.

We plan to solve this issue by introducing the @impl true annotation that will check the following function is the implementation of a behaviour. Therefore, if someone writes a code like this:

@impl true
def handle_call(message, state) do
  ...
end

The Elixir compiler will warn that the current module has no behaviour that requires the handle_call/2 function to be implemented, forcing the developer to correctly define a handle_call/3 function. This is a fantastic tool that will not only help the compiler to emit warnings but will also make the code more readable, as any developer that later uses the codebase will understand the purpose of such function is to be a callback implementation.

The @impl annotation is optional. When @impl true is given, we will also add @doc false unless documentation has been given. We will also support a module name to be given. If @impl ... is set once (the setting must be done outside of a quote expression so we need to check the context of the module attribute accodingly), all other callbacks in that module must have @impl declared, otherwise a warning will be emitted.

When a module name is given, Elixir will check the following function is an implementation of a callback in the given behaviour:

@impl GenServer
def handle_call(message, from, state) do
  ...
end

defoverridable with behaviours

While @impl will give more confidence and assistance to developers, it is only useful if developers are defining behaviours for their contracts. Elixir has always advocated that a behaviour must always be defined when a set of functions is marked as overridable but it has never provided any convenience or mechanism to enforce such rules.

Therefore we propose the addition of defoverridable BehaviourName, which will make all of the callbacks in the given behaviour overridable. This will help reduce the duplication between behaviour and defoverridable definitions and push the community towards best practice. Therefore, instead of:

defmodule GenServer do
  defmacro __using__(_) do
    quote do
      @behaviour GenServer
      def init(...) do ... end
      def terminate(..., ...) do ... end
      def code_change(..., ..., ...) do ... end
      defoverridable init: 1, terminate: 2, code_change: 3
    end
  end
end

We propose:

defmodule GenServer do
  defmacro __using__(_) do
    quote do
      @behaviour GenServer
      def init(...) do ... end
      def terminate(..., ...) do ... end
      def code_change(..., ..., ...) do ... end
      defoverridable GenServer
    end
  end
end

By promoting new defoverridable API above, we hope library developers will consistently define behaviours for their overridable functions, also enabling developers to use the @impl true annotation to guarantee the proper callbacks are being implemented.

The existing defoverridable API will continue to work as today and won't be deprecated.

PS: Notice defoverridable always comes after the function definitions, currently and as well as in this proposal. This is required because Elixir functions have multiple clauses and if the defoverridable came before, we would be unable to know in some cases when the overridable function definition ends and when the user overriding starts. By having defoverridable at the end, this boundary is explicit.

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Apr 18, 2017

Contributor

I started looking into this, but I wonder if this @impl is compatible with the existing @impl we already use for Protocols: https://github.com/elixir-lang/elixir/blame/master/lib/elixir/lib/protocol.ex#L573

Contributor

samphilipd commented Apr 18, 2017

I started looking into this, but I wonder if this @impl is compatible with the existing @impl we already use for Protocols: https://github.com/elixir-lang/elixir/blame/master/lib/elixir/lib/protocol.ex#L573

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 18, 2017

Member

Yes, we will probably have to rename that variable to something else.

Member

josevalim commented Apr 18, 2017

Yes, we will probably have to rename that variable to something else.

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 18, 2017

Sam Davies
Use @implprot to store internal protocol metadata
- Required because we want to repurpose @impl
- See: #5734
@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Apr 18, 2017

Contributor

@josevalim you mean rename the existing protocol @impl? May I suggest we use a new name instead? Perhaps something like:

@override true
def handle_call(message, state) do
  ...
end
@override GenServer
def handle_call(message, from, state) do
  ...
end

This seems clearer to me. It's naming is consistent with defoverridable and it avoids having to change unrelated existing code.

Contributor

samphilipd commented Apr 18, 2017

@josevalim you mean rename the existing protocol @impl? May I suggest we use a new name instead? Perhaps something like:

@override true
def handle_call(message, state) do
  ...
end
@override GenServer
def handle_call(message, from, state) do
  ...
end

This seems clearer to me. It's naming is consistent with defoverridable and it avoids having to change unrelated existing code.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 18, 2017

Member

@samphilipd rename the existing @impl in defprotocol. @overrides doesn't make much sense because you can use @impl GenServer without overriding any callback. It is about behaviours and not defoverridable.

Member

josevalim commented Apr 18, 2017

@samphilipd rename the existing @impl in defprotocol. @overrides doesn't make much sense because you can use @impl GenServer without overriding any callback. It is about behaviours and not defoverridable.

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 18, 2017

Sam Davies
Change module attribute name that stores internal protocol metadata
- Changed from @impl to @__implements_protocols_for
- Required because we want to repurpose @impl - see #5734

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 18, 2017

Sam Davies
Change module attribute name that stores internal protocol metadata
- Changed from @impl to @__protocol_metadata
- Required because we want to repurpose @impl - see #5734

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 18, 2017

Sam Davies
Change module attribute name that stores internal protocol metadata
- Changed from @impl to @protocol_metadata
- Required because we want to repurpose @impl - see #5734

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 20, 2017

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 20, 2017

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 21, 2017

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 21, 2017

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 21, 2017

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 21, 2017

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Apr 22, 2017

Contributor

I am currently implementing this test:

  test "impl with callback name not in behaviour" do
    assert capture_err(fn ->
      Code.eval_string """
      defmodule Sample do
        @behaviour Kernel.WarningTest.FooBehaviour

        @impl Kernel.WarningTest.FooBehaviour
        def baz(), do: :ok
      end
      """
    end) =~ "module attribute @impl was set for function baz/0 but behaviour Kernel.WarningTest.FooBehaviour does not define this callback"
  after
    purge Sample
  end

@josevalim my question is, where is the best place to perform this check and emit warning? I thought of either hardcoding it in elixir_def.erl, or adding it as an @on_definition hook... what is your opinion?

Contributor

samphilipd commented Apr 22, 2017

I am currently implementing this test:

  test "impl with callback name not in behaviour" do
    assert capture_err(fn ->
      Code.eval_string """
      defmodule Sample do
        @behaviour Kernel.WarningTest.FooBehaviour

        @impl Kernel.WarningTest.FooBehaviour
        def baz(), do: :ok
      end
      """
    end) =~ "module attribute @impl was set for function baz/0 but behaviour Kernel.WarningTest.FooBehaviour does not define this callback"
  after
    purge Sample
  end

@josevalim my question is, where is the best place to perform this check and emit warning? I thought of either hardcoding it in elixir_def.erl, or adding it as an @on_definition hook... what is your opinion?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 22, 2017

Member

@samphilipd I would recommend doing something like this on elixir_module:

DocsOnDefinition =
    case elixir_compiler:get_opt(docs) of
      true -> [{'Elixir.Module', compile_doc}];
      _    -> [{elixir_module, delete_doc}]
    end,

ImplOnDefinition =
    case elixir_compiler:get_opt(internal) of
      true -> [{'Elixir.Module', check_impl}];
      _    -> [{elixir_module, delete_impl}]
    end,

OnDefinition = ImplOnDefinition ++ DocsOnDefinition

This way Module.check_impl will be invoked on every definition and you can check it there, similar to compile_doc.

Member

josevalim commented Apr 22, 2017

@samphilipd I would recommend doing something like this on elixir_module:

DocsOnDefinition =
    case elixir_compiler:get_opt(docs) of
      true -> [{'Elixir.Module', compile_doc}];
      _    -> [{elixir_module, delete_doc}]
    end,

ImplOnDefinition =
    case elixir_compiler:get_opt(internal) of
      true -> [{'Elixir.Module', check_impl}];
      _    -> [{elixir_module, delete_impl}]
    end,

OnDefinition = ImplOnDefinition ++ DocsOnDefinition

This way Module.check_impl will be invoked on every definition and you can check it there, similar to compile_doc.

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Apr 22, 2017

Contributor

@josevalim I get an error using this code on latest master, even with check_impl correctly defined in module.ex.

Crash stacktrace:

error: undef
stacktrace: [{'Elixir.Module',check_impl,
                 [#{'__struct__' => 'Elixir.Macro.Env',aliases => [],
                    context => nil,context_modules => [],export_vars => nil,
                    file =>
                        <<"/Users/sam/code/contributions/elixir/lib/elixir/lib/kernel.ex">>,
                    function => {abs,1},
                    functions => [],lexical_tracker => nil,line => 57,
                    macro_aliases => [],
                    macros =>
                        [{elixir_bootstrap,
                             [{'@',1},
                              {def,1},
                              {def,2},
                              {defmacro,1},
                              {defmacro,2},
                              {defmacrop,2},
                              {defmodule,2},
                              {defp,2}]}],
                    module => 'Elixir.Kernel',prematch_vars => nil,
                    requires =>
                        ['Elixir.Kernel','Elixir.Kernel.Typespec',
                         elixir_bootstrap],
                    vars => []},
                  def,abs,
                  [{number,[{line,57}],nil}],
                  [],
                  [{do,{{'.',[{line,58}],[erlang,abs]},
                        [{line,58}],
                        [{number,[{line,58}],nil}]}}]],
                 []},
             {elixir_def,'-run_on_definition_callbacks/7-lc$^0/1-0-',7,
                 [{file,"src/elixir_def.erl"},{line,179}]},
             {elixir_def,run_on_definition_callbacks,7,
                 [{file,"src/elixir_def.erl"},{line,179}]},
             {elixir_def,store_definition,10,
                 [{file,"src/elixir_def.erl"},{line,144}]},
             {elixir_compiler_1,'__MODULE__',1,
                 [{file,
                      "/Users/sam/code/contributions/elixir/lib/elixir/lib/kernel.ex"},
                  {line,57}]},
             {elixir_compiler,dispatch,6,
                 [{file,"src/elixir_compiler.erl"},{line,85}]},
             {elixir_module,eval_form,6,
                 [{file,"src/elixir_module.erl"},{line,216}]},
             {elixir_module,compile,5,
                 [{file,"src/elixir_module.erl"},{line,74}]}]
make: *** [lib/elixir/ebin/Elixir.Kernel.beam] Error 1
Contributor

samphilipd commented Apr 22, 2017

@josevalim I get an error using this code on latest master, even with check_impl correctly defined in module.ex.

Crash stacktrace:

error: undef
stacktrace: [{'Elixir.Module',check_impl,
                 [#{'__struct__' => 'Elixir.Macro.Env',aliases => [],
                    context => nil,context_modules => [],export_vars => nil,
                    file =>
                        <<"/Users/sam/code/contributions/elixir/lib/elixir/lib/kernel.ex">>,
                    function => {abs,1},
                    functions => [],lexical_tracker => nil,line => 57,
                    macro_aliases => [],
                    macros =>
                        [{elixir_bootstrap,
                             [{'@',1},
                              {def,1},
                              {def,2},
                              {defmacro,1},
                              {defmacro,2},
                              {defmacrop,2},
                              {defmodule,2},
                              {defp,2}]}],
                    module => 'Elixir.Kernel',prematch_vars => nil,
                    requires =>
                        ['Elixir.Kernel','Elixir.Kernel.Typespec',
                         elixir_bootstrap],
                    vars => []},
                  def,abs,
                  [{number,[{line,57}],nil}],
                  [],
                  [{do,{{'.',[{line,58}],[erlang,abs]},
                        [{line,58}],
                        [{number,[{line,58}],nil}]}}]],
                 []},
             {elixir_def,'-run_on_definition_callbacks/7-lc$^0/1-0-',7,
                 [{file,"src/elixir_def.erl"},{line,179}]},
             {elixir_def,run_on_definition_callbacks,7,
                 [{file,"src/elixir_def.erl"},{line,179}]},
             {elixir_def,store_definition,10,
                 [{file,"src/elixir_def.erl"},{line,144}]},
             {elixir_compiler_1,'__MODULE__',1,
                 [{file,
                      "/Users/sam/code/contributions/elixir/lib/elixir/lib/kernel.ex"},
                  {line,57}]},
             {elixir_compiler,dispatch,6,
                 [{file,"src/elixir_compiler.erl"},{line,85}]},
             {elixir_module,eval_form,6,
                 [{file,"src/elixir_module.erl"},{line,216}]},
             {elixir_module,compile,5,
                 [{file,"src/elixir_module.erl"},{line,74}]}]
make: *** [lib/elixir/ebin/Elixir.Kernel.beam] Error 1
@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 22, 2017

Member

You need to run make clean_exbeam compile after doing those changes so we can rebootstrap.

Member

josevalim commented Apr 22, 2017

You need to run make clean_exbeam compile after doing those changes so we can rebootstrap.

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Apr 23, 2017

Contributor

Thanks Jose, I will add this to the README.

Contributor

samphilipd commented Apr 23, 2017

Thanks Jose, I will add this to the README.

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Apr 23, 2017

Contributor

Hmm, it still failed :/

I think I figured out why though. It's because internal flag is set during the bootstrap process then unset later, so the case condition is the wrong way around. This order appears to work:

  ImplOnDefinition =
      case elixir_compiler:get_opt(internal) of
        true -> [{elixir_module, delete_impl}];
        _    -> [{'Elixir.Module', check_impl}]
      end,
Contributor

samphilipd commented Apr 23, 2017

Hmm, it still failed :/

I think I figured out why though. It's because internal flag is set during the bootstrap process then unset later, so the case condition is the wrong way around. This order appears to work:

  ImplOnDefinition =
      case elixir_compiler:get_opt(internal) of
        true -> [{elixir_module, delete_impl}];
        _    -> [{'Elixir.Module', check_impl}]
      end,
@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 23, 2017

Member
Member

josevalim commented Apr 23, 2017

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 23, 2017

samphilipd pushed a commit to samphilipd/elixir that referenced this issue Apr 24, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue Apr 25, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue Apr 25, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue Apr 25, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue Apr 28, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue Apr 28, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue Apr 28, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue Apr 28, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue May 6, 2017

samphilipd added a commit to samphilipd/elixir that referenced this issue May 6, 2017

@josevalim josevalim closed this May 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment