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 On Thu, Jan 19, 2017 at 10:39 AM, Iuri Machado <[email protected]> wrote: > 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. > > Em quinta-feira, 19 de janeiro de 2017 09:52:42 UTC-2, José Valim escreveu: >> >> 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 GenServerend >> >> Implementation-wise, use GenServer defines functions such as: >> >> def terminate(reason, state) do >> :okend >> >> 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 >> endend >> >> We propose: >> >> defmodule GenServer do >> @callback handle_call(message, from, state) >> @optional_callbacks handle_call: 3 >> >> defmacro __using__(_) do >> quote do >> @behaviour GenServer >> >> # ... >> end >> endend >> >> 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 truedef 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 GenServerdef 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 >> endend >> >> 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 >> endend >> >> 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(beha >> viour_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 >> > -- > 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 [email protected]. > To view this discussion on the web visit https://groups.google.com/d/ > msgid/elixir-lang-core/85e62305-2b5f-4ae2-8e20- > 19f5cdf6e6d8%40googlegroups.com > <https://groups.google.com/d/msgid/elixir-lang-core/85e62305-2b5f-4ae2-8e20-19f5cdf6e6d8%40googlegroups.com?utm_medium=email&utm_source=footer> > . > > 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAK%3D%2B-TsRoUZPnGSFYeu1F46%3D7xW8cTHPNQpCd-AYLwNA9trV4w%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
