Analyzer for `dancing-dots` doesn't consider using alias in `@impl`

[pichi]

Implementations like this

defmodule DancingDots.Flicker do
  alias DancingDots.Dot
  alias DancingDots.Animation
  use Animation

  @impl Animation
  def handle_frame(%Dot{} = dot, frame_number, opts) do
  end
end

ends with recommendation:

Don't forget to annotate all the callback implementations with @impl DancingDots.Animation.

But the compiler seems happy with it. I don't see in the documentation if aliases are allowed in @impl attribute and I'm not sure how to test it.

[jiegillet]

The real story about @impl is that it's not actually required. According to this post:

The impl attribute is not actually required. You can remove the @impl attribute, and nothing bad will happen. However, specifying an @impl attribute provides some advantages. It documents which functions are behaviour implementations, which helps with maintenance and reading the code. It also disables documentation for that function, since the real documentation is associated with the @callback definition in the behaviour. Finally, it helps static analysis tools like dialyzer understand your code better and make more helpful suggestions.

That makes it hard to test and be sure of the behavior without jumping into the Elixir source code...

That being said, you are right about the analyzer, it shouldn't send you that message.
However, I cannot reproduce the comment with the code snippet you shared. Would you mind sharing the whole file? Alternatively, send us a mentoring link

[pichi]

Complete solution

defmodule DancingDots.Animation do
  @type dot :: DancingDots.Dot.t()
  @type opts :: keyword
  @type error :: any
  @type frame_number :: pos_integer

  @callback init(opts) :: {:ok, opts} | {:error, error}
  @callback handle_frame(dot, frame_number, opts) :: dot

  defmacro __using__(_) do
    quote do
      @behaviour unquote(__MODULE__)
      def init(opts), do: {:ok, opts}
      defoverridable init: 1
    end
  end
end

defmodule DancingDots.Flicker do
  alias DancingDots.Dot
  alias DancingDots.Animation
  use Animation

  @impl Animation
  def handle_frame(%Dot{} = dot, frame_number, _opts) when rem(frame_number, 4) == 0,
    do: %{dot | opacity: dot.opacity / 2}

  def handle_frame(%Dot{} = dot, _frame_number, _opts), do: dot
end

defmodule DancingDots.Zoom do
  alias DancingDots.Dot
  alias DancingDots.Animation
  use Animation

  @impl Animation
  def init(opts), do: check_velocity(opts[:velocity], opts)
  defp check_velocity(v, opts) when is_number(v), do: {:ok, opts}
  defp check_velocity(v, _), do: {:error, error_velocity_msg(v)}

  defp error_velocity_msg(v),
    do: "The :velocity option is required, and its value must be a number. Got: #{inspect(v)}"

  @impl Animation
  def handle_frame(%Dot{} = dot, frame_number, opts),
    do: %{dot | radius: dot.radius + (frame_number - 1) * opts[:velocity]}
end

[jiegillet]

Thank you, I was able to determine the source of the comment, it is the second handle_frame in

  @impl Animation
  def handle_frame(%Dot{} = dot, frame_number, _opts) when rem(frame_number, 4) == 0,
    do: %{dot | opacity: dot.opacity / 2}

  def handle_frame(%Dot{} = dot, _frame_number, _opts), do: dot

since it is missing an annotation. Nothing to do with aliases, which are handled correctly.

While, as we discussed, the annotations are not strictly mandatory, it is good practice to have them, and the exercise introduction mentions

To mark which function comes from which behaviour, we should use the module attribute @impl before each function.

So I don't consider this an analyzer bug, it's working as expected.

[pichi]

Thank you, I was able to determine the source of the comment, it is the second handle_frame in

  @impl Animation
  def handle_frame(%Dot{} = dot, frame_number, _opts) when rem(frame_number, 4) == 0,
    do: %{dot | opacity: dot.opacity / 2}

  def handle_frame(%Dot{} = dot, _frame_number, _opts), do: dot

since it is missing an annotation. Nothing to do with aliases, which are handled correctly.

There is no such thing as a second handle_frame function, this is the second function clause of the same function handle_frame/2. So there is @impl before each function and the Elixir compiler handles it as such. You can test it by removing @impl from init/1. If you remove the annotation from init/1 the Elixir compiler warns you that you forgot to put @impl annotation. If it would be true that there is a missing annotation before the second clause of handle_frame/2 there would be a warning. There is not any warning because the second clause of the same function is not another function. Ergo there is an annotation before each function which is an implementation of the behavior. If the analyzer considers each clause of the same function as a different function and expects @impl annotation there its a bug.

[angelikatyborska]

I'm with @jiegillet that this is not an analyzer bug. While it's not strictly necessary to add @impl for each clause of the same function, it is a practice that we want to encourage (see for example this GenServer code example: https://elixir-lang.org/getting-started/mix-otp/genserver.html#the-need-for-monitoring).

I think the comment using the phrase "all the callback implementations" is clear (implementation = clause), but we could also improve the comment if you think it's confusing?

[pichi]

I'm with @jiegillet that this is not an analyzer bug. While it's not strictly necessary to add @impl for each clause of the same function, it is a practice that we want to encourage (see for example this GenServer code example: https://elixir-lang.org/getting-started/mix-otp/genserver.html#the-need-for-monitoring).

I think the comment using the phrase "all the callback implementations" is clear (implementation = clause), but we could also improve the comment if you think it's confusing?

I have checked all examples on the linked page and none of them have the second function clause so I don't know what I'm supposed to see there. I have checked a lot of available Elixir source code-base and none of them have annotated any function clause except the first one with any annotation not only @impl. I'm a novice in Elixir development but I code professionally in Erlang for over 15 years and it doesn't make any sense to me that anybody would even think about the next function clauses in the way that it would or should need annotation. We can look for examples.

Phoenix framework here: Phoenix.Socket.V1.JSONSerializer.encode!/1 and Phoenix.Socket.V2.JSONSerializer.fastlane!/1 and Phoenix.Socket.V2.JSONSerializer.encode!/1 and form component template

Phoenix Liveview here: Phoenix.LiveView.Channel.handle_info/2 and Phoenix.LiveView.Channel.handle_call/2 and Phoenix.LiveView.UploadChannel.handle_info/2 and Phoenix.LiveView.UploadChannel.handle_call/2

Ecto here: Ecto.Association.Has.on_repo_change/5 and Ecto.Association.BelongsTo.on_repo_change/5 and so on.

Elixir itself here: Calendar.ISO.time_from_day_fraction/1 and Calendar.ISO.day_of_era/3 and Calendar.UTCOnlyTimeZoneDatabase.time_zone_period_from_utc_iso_days/2 and Calendar.UTCOnlyTimeZoneDatabase.time_zone_periods_from_wall_datetime/2 and Config.Reader.init/1 and DynamicSupervisor.handle_call/3 and DynamicSupervisor.handle_info/2 and so on. I think you can see the pattern there. I have found over twenty more examples of multi-clause functions in Elixir code-base alone.

Especially Elixir source code I would consider one of the best-documented code-base I have ever seen in my carrier. It seems meticulously annotated as well. So I grow a little bit curious about who is we in the sentence it is a practice that we want to encourage because it doesn't seem as the best practice in the Elixir code-base what I have seen so far.

[angelikatyborska]

I have checked all examples on the linked page and none of them have the second function clause so I don't know what I'm supposed to see there.

Here, two clauses of handle_info/2:

So I grow a little bit curious about who is we in the sentence it is a practice that we want to encourage

I thought that would be clear from the context of the discussion. "We" are the maintainers of the Elixir track on Exercism, which would be @jiegillet and myself, previously also @neenjaw but he's less active at the moment and I'm not sure what's his opinion on this particular problem (if this pinged you, Tim: hi 👋 no need to respond).

I have checked a lot of available Elixir source code-base and none of them have annotated any function clause except the first one with any annotation not only @impl

Here are some counter-examples from the same files that you have linked:
https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/association.ex#L1039-L1049
https://github.com/phoenixframework/phoenix_live_view/blob/master/lib/phoenix_live_view/upload_channel.ex#L115-L128

and from some other files from the same repositories:
https://github.com/elixir-lang/elixir/blob/2c1ee7fca5ef212fd16690b7091a1944a1d18115/lib/elixir/lib/exception.ex#L893-L915
https://github.com/elixir-lang/elixir/blob/385b36027f933f4c014d0cfdb928fe5fcda0a57d/lib/mix/lib/mix/tasks/compile.ex#L82-L122
https://github.com/elixir-lang/elixir/blob/3a86e213d4eb5c13dbda61e778b559b5e6fd9e61/lib/mix/lib/mix/project_stack.ex#L319-L334

it doesn't make any sense to me that anybody would even think about the next function clauses in the way that it would or should need annotation.

My reasoning for doing this is: the repeated information makes it easier to notice that a certain function clauses are callback implementations in situations where function bodies are long enough so that all the other function clauses are far apart and not all visible at once on most computer screens. If the repeated information is not incorrect (the code still compiles, it doesn't change anything), why not add it if it helps?

[pichi]

I'm curious why you didn't include a third function clause in the second link https://github.com/phoenixframework/phoenix_live_view/blob/master/lib/phoenix_live_view/upload_channel.ex#L115-L134

It's clearly an optional feature silently ignored by the Elixir compiler and then it doesn't make sense to make it mandatory in your analyzer. If you like to make your course participants ignore the analyzer's recommendations you are on a good track. I was trying to help you improve it, if you do not have any other argument that adding useless clutter improves readability I'm not going to convince you otherwise from the position of over 30 years of professional experience in the field. Good luck with this approach. I'm done here.

[pichi]

My reasoning for doing this is: the repeated information makes it easier to notice that a certain function clauses are callback implementations in situations where function bodies are long enough so that all the other function clauses are far apart and not all visible at once on most computer screens. If the repeated information is not incorrect (the code still compiles, it doesn't change anything), why not add it if it helps?

You lost me there where function bodies are long enough so that all the other function clauses are far apart If your function bodies are long enough that other function clauses are far apart you already lost all readability. Fix the more urgent problems first then you will find out that adding annotation doesn't make sense because other function clauses are not far apart. Fix more urgent issues first then you will easily find out that other problems disappear. It's again my long experience in the industry talking.