Proposal: Behaviours, defoverridable and impl

[José Valim]

Hi everyone,

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

As the community grew, defoverridable/1 started to show some flaws in its implementation. Furthermore, the community did not always follow up on best practices, often times marking functions as overridable but without defining a proper Behaviour behind the scenes.

The goal of this proposal is to clarify the existing functionality and propose extensions that will push the community towards best practices.

Using @optional_callbacks

In the example above, we have used defoverridable terminate: 2 to make the definition of the terminate/2 function optional.

However, in some cases, the use of defoverridable seems to be unnecessary. For instance, we provide a default implementation for handle_call/3 and mark it as overridable, but the default implementation simply raises when invoked. That's counter-intuitive as it would be best to simply not define a default implementation in the first place, truly making the handle_call/3 callback optional.

Luckily, Erlang 18 added support for marking callbacks as optional, which we support on Elixir v1.4. We propose Elixir and libraries to leverage this feature and no longer define default implementations for the handle_* functions and instead mark them as optional.

Instead of the version we have today:

defmodule GenServer do
  @callback handle_call(message, from, state)

  defmacro __using__(_) do
    quote do
      @behaviour GenServer

      def handle_call(_message, _from, _state) do
        raise "handle_call/3 not implemented"
      end

      # ...

      defoverridable handle_call: 3
    end
  end
end

We propose:

defmodule GenServer do
  @callback handle_call(message, from, state)
  @optional_callbacks handle_call: 3

  defmacro __using__(_) do
    quote do
      @behaviour GenServer

      # ...
    end
  end
end

The proposed code is much simpler conceptually since we are using the @optional_callbacks feature instead of defoverridable to correctly mark optional callbacks as optional. defoverridable will still be used for functions such as terminate/2, which are truly required.

For developers using GenServer, no change will be necessary to their code base. The goal is that, by removing unnecessary uses of defoverridable/1, the Elixir code base can lead by example and hopefully push the community to rely less on such tools when they are not necessary.

The @impl annotation

Even with the improvements above, the usage of defoverridable/1 and @optional_callbacks still 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. 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
      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.

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.

Summing up

This proposal promotes the use the of @optional_callbacks, which is already supported by Elixir, and introduces defoverridable(behaviour_name) which will push library developers to define proper behaviours and callbacks for overridable code.

We also propose the addition of the @impl true or @impl behaviour_nameannotation, that will check the following function has been listed as a callback by any behaviour used by the current module.

Feedback?

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

[Norbert Melzer]

This very same post is on elixirforum (https://elixirforum.com/t/behaviours-defoverridable-and-implementations/3338) as well, and I commented there. I will post the same comment here and also continue discussion here if it happens:

> I really like the idea, but I think that @impl might get confused with something related to protocols because of Kernel.defimpl/3, Protocol.assert_impl!/2, and Protocol.extract_impls/2 beeing the only things mentioning "impl" in elixir until now. Therefore I'd opt for something like @override as in Java.  

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4J2EE%3DvM9k6hz-wsASfYUuTs%2B_JwRW4cnyFn-eYAcuD0g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Iuri Machado]

Hi José,

I really like the idea of changing the behaviour of defoverridable, although I would rather go with the check on the implementation to raise warnings as the default instead of setting @impl true.

[Paul Schoenfelder]

I believe the reason why `@impl` is required is because if a callback is optional, you can't warn about it missing, but the bug would be if someone *thought* they were implementing the optional callback, but had the signature wrong. I do wonder though if it would be possible to check for function definitions with the same name as a callback, but a signature which doesn't match. It seems to me that it would be worth a warning in that case anyway, since naming an unrelated function the same as a behaviour callback is potentially a problem if the signature of the callback changes to match it at some point in the future. So if you have `handle_call/3` defined as an optional callback in the GenServer behaviour, and the implementing module has `handle_call/2` defined, but no `handle_call/3`, you could warn about potentially mismatched signatures. If it's a legitimate false-positive, it will encourage the dev to choose a better name for that `handle_call/2` function to avoid the warning - but I suspect the vast majority of the time, such false positives are unlikely to occur.

I'm +1 on all of these changes myself, though as a library maintainer, it seems to me that there is a bit of a pain point with `defoverridable` specifically, in that it takes a Keyword.t today, but will take atom() in the new version. Will `defoverridable` handle this for us, or will we need to check for the current Elixir version and conditionally use one form or the other? (gen_statem comes to mind as another example of a breaking change between versions where this had to be done, as seen in GenStateMachine).

Paul

--

You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-core+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/85e62305-2b5f-4ae2-8e20-19f5cdf6e6d8%40googlegroups.com.

[Amos King]

Using @optional_callbacks

I love the idea of this. I think it simplifies a lot of the work and is still explicit. If this is in place can a callback still have an implementation? I ask because sometimes it is nice to have the exception give some larger context of information when an undefined callback is used. Many times I think a default error is okay, but sometimes I want to provide the developer with a more in depth explanation.

defoverridable with behaviours

I like the simplification, but I'm not sure of the implementation. The original way is more explicit about what is overridable and what is not. Would it be possible to be explicit by changing defoverridable to be used the same way as def, defmodule, and defimple? The defoverridable feels like it goes along with the other "def" tokens.

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

end 

If this isn't possible it might be nice to look at a new name altogether.

The @impl annotation

I like the notation here when it includes that name of the behavior, but I'm not a fan of the true. I understand that it appears directly above the header in question, but I like the explicit version. Would this only go over one clause?, or would it have to be over top of each clause?

Cheers,

Amos

Amos King

Owner

Binary Noggin

http://binarynoggin.com #business

http://thisagilelife.com #podcast

=======================================================
I welcome VSRE emails. Learn more at http://vsre.info/
=======================================================

On Thu, Jan 19, 2017 at 10:39 AM, Iuri Machado <imet...@gmail.com> wrote:

--

You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-core+unsubscribe@googlegroups.com.

[José Valim]

I believe the reason why `@impl` is required is because if a callback is optional, you can't warn about it missing, but the bug would be if someone *thought* they were implementing the optional callback, but had the signature wrong.

Yes, exactly.

 

I do wonder though if it would be possible to check for function definitions with the same name as a callback, but a signature which doesn't match.

Unfortunately may led to false positives.

 

I'm +1 on all of these changes myself, though as a library maintainer, it seems to me that there is a bit of a pain point with `defoverridable` specifically, in that it takes a Keyword.t today, but will take atom() in the new version.

The existing defoverridable API will continue to work as today. That won't change at all (nor be deprecated).

[José Valim]

Using @optional_callbacks

I love the idea of this. I think it simplifies a lot of the work and is still explicit. If this is in place can a callback still have an implementation? I ask because sometimes it is nice to have the exception give some larger context of information when an undefined callback is used. Many times I think a default error is okay, but sometimes I want to provide the developer with a more in depth explanation.

So the behaviour definition + __using__macro is how you provide default implementations for callbacks. We currently have no plans for streamlining this system beyond the proposal above.

 The original way is more explicit about what is overridable and what is not. Would it be possible to be explicit by changing defoverridable to be used the same way as def, defmodule, and defimple? The defoverridable feels like it goes along with the other "def" tokens.

I don't like defoverridable as a function definition because we would need defoverridablemacro and it doesn't play well with function clauses. Remember a function may always have multiple clauses. This is similar to the PS I explain in the original issue.

 

I like the notation here when it includes that name of the behavior, but I'm not a fan of the true. I understand that it appears directly above the header in question, but I like the explicit version. Would this only go over one clause?, or would it have to be over top of each clause?

Over one clause. Annotations are usually for a single function (and each function is made of multiple clauses).

[jisaa...@gmail.com]

I like all these changes - and I agree with Norbert, I like `@override true` better than `@impl true`

But I'm happy to have the functionality whatever the name. +1

[Drew Olson]

These changes all seem great. I also prefer `@override true` to `@impl true`.

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-core+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/0c46c5d9-3952-49a2-adfb-9eae6791eb86%40googlegroups.com.

[José Valim]

I have answered why @override true does not work on the forum.

The gist is that @impl true works for behaviors even if they don't provide overridable inplementations. @override true would only make sense for defoverridable and that would unnecessarily constrain us.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/0c46c5d9-3952-49a2-adfb-9eae6791eb86%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--


You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.


To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAN3B1jd_d3ZshY8hpE0eFYx-j5Fd_2dac5_CoB-vrWMYWaVsow%40mail.gmail.com.