Elixir: Consider making @impl required

[josevalim]

We have added @impl to mark which functions are callback implementations but we may want to make such required at some point down the road.

[brodeuralexis]

if the @impl attribute is required when using @behaviour, wouldn't compilation fail for a lot of libraries, plug being one of them ?

[josevalim]

Required means they would always warn. We probably wouldn't halt compilation.

@michalmuskala why did you "downvote" the feature? Can you please explain your rationale?

[whatyouhide]

Small note: warning is the way to go IMO, because that's how most of the behaviour-related functionality works. For example, missing callbacks warn, they don't halt compilation :).

[brodeuralexis]

Hi @josevalim, I have another question for you.

Should we show missing @impl true warnings inside exceptions for the exception/1 and message/1 optional callbacks ?

It would means that for this code :

defmodule MyException do
  defexception where: nil

  def message(e), do: "Something horrible happened in #{e.where}"
end

We would have a warning similar to this :

module attribute @impl was not set for function message/1 callback (specified in Exception). This either means you forgot to add the "@impl true" annotation before the definition or that you are accidentally overriding this callback
  my_exception.ex:4

And it should therefor be rewritten as :

defmodule MyException do
  defexception where: nil

  @impl Exception
  def message(e), do: "Something horrible happened in #{e.where}"
end

[josevalim]

@brodeuralexis yup, something along those lines. Note this is still under discussion though so while contributions are welcome, we should hold back on implementations until it is agreed. Plus we should probably start porting the behaviour work in #5800 before working on this one.

[brodeuralexis]

Is it possible that the list in #5800 is not up to date ? I see that we define quite a few warnings for behaviours already.

[josevalim]

The list in #5800 may be out of date but the warnings you linked are not tracking in that list. That list is about the warnings raised by Erlang and impl is an Elixir specific thing.

[michalmuskala]

I missed it when I was asked to explain my 👎 on this feature, so here it goes.

I was never a fan of the @impl feature in the first place. I feel like it just adds noise to the source code without providing additional value. Especially in code like GenServer that heavily leverages pattern matching in multiple heads - the distance between the "annotation" and the code that it affects is often so large, it feels completely out of place (I have a similar feeling about adding @doc false annotations to callbacks - it too feels kind of strange). There's also the case of a "tower of attributes" - with @impl a regular callbacks looks more-or-less like this:

@doc false
@impl true
@spec handle_info(:timeout, t) :: {:noreply, t}
def handle_info(:timeout, state) do
  # do something
  {:noreply, state}
end

Two annotations is already bad, but now we're up to three.

Let's think about an alternative, so this doesn't become a completely meaningless criticism.

What is the reason people face issues when implementing, for example, GenServer callbacks? It's the fact that they decide to use the call mechanism, but they misdefine the handle_call callback.
So what if we pushed @impl even further? What if we made defining a callback different from defining a function. What if we used defc for defining callbacks. It would automatically check if the declaration is actually a callback (like @impl does) and failed otherwise. It would also fail if it was mixed with def or defp declarations (like def and defp do today). But most importantly - the declaration is local and not global. I can look at just a single head and know that this is supposed to be a callback - no need to scroll all the way up. This is very similar in how the def/defp distinction is superior to @exports attribute - it decreases the amount of things one needs to keep in mind when scanning code. It also doesn't conflict with defp since callbacks can't be private.

One downside is that it doesn't allow specifying which behaviour we're implementing in a multi-behaviour module, but I think this is a rather rare edge-case.

I guess this is more of a discussion of @impl feature itself rather than making it required. As to making it required (and this in fact would be exactly the same for the defc alternative).

Making it required would be a breaking change and not only that - a very annoying breaking change, since the code itself doesn't need to change in any way (and there is no attached improvement in runtime behaviour or performance) - just some annotation to suppress the compiler warning. This is very different from how I feel, for example, about the seemingly similar "unsafe variable warning" - there the change it requires is meaningful and often improves the logic of the program. It is not an "empty" compiler annotation.

Finally there's the technical aspect that was mentioned already - there's a lot of code that doesn't use it and where use would become awkward. Exceptions are one example, plug is another, yet another can be protocols - they implicitly define a behaviour that defimpl uses. Does that mean, I'd need to use @impl in every protocol implementation? If not, then why some behaviours are specially handled by the compiler and others aren't?

If there's a decision to make it required, I proposed somewhat of a "middle ground" solution - if you use @impl or defc on any function in a module, you need to use it on all callbacks, but if you don't use it - that's fine, no warning emitted.

[wojtekmach]

about:

@doc false
@impl true
@spec handle_info(:timeout, t) :: {:noreply, t}
def handle_info(:timeout, state) do

@impl true already adds @doc false so it can be skipped (but you can still do @doc "foo" to overwrite it). Furthermore, perhaps @impl true could also "pre-fill" @spec if that would be useful.

[josevalim]

That's valid criticism @michalmuskala. As @wojtekmach said, @doc false is not necessary. We could maybe even allow this:

@impl handle_info(:timeout, t) :: {:noreply, t}
def handle_info(:timeout, state)

But I am not sure if we would gain much at this point.

If there's a decision to make it required, I proposed somewhat of a "middle ground" solution - if you use @impl or defc on any function in a module, you need to use it on all callbacks, but if you don't use it - that's fine, no warning emitted.

That's already how it works today.

In any case, I feel like you brought enough concerns for us to not make it required by default, at least for now. If someone feels like @impl hadle_info(...) is a good, feel free to argue its use case. We can open a new issue for it.