What is the idiomatic way to use @spec?

Questions / Help
1

Background

I have a multiclause function and I want to use @spec. I am aware of two ways I can do it, but I don’t know which way is the most correct.

Code

  defp get_country({:ok, [country | _tail]}, _ip), do: String.upcase(country)

  defp get_country({:error, reason}, ip) do
    Logger.error("Error Geolocating #{inspect ip} -> #{inspect reason}")
    nil
  end

Using Spec

I know of 2 ways to use @spec that wont make dialyzer throw errors:

Option 1:

  @spec get_country({:ok, [String.t]} | {:error, any}, String.t) :: String.t | nil
  defp get_country({:ok, [country | _tail]}, _ip), do: String.upcase(country)

  defp get_country({:error, reason}, ip) do
    Logger.error("Error Geolocating #{inspect ip} -> #{inspect reason}")
    nil
  end

Option 2:

  @spec get_country({:ok, [String.t]}, String.t) :: String.t 
  defp get_country({:ok, [country | _tail]}, _ip), do: String.upcase(country)

  @spec get_country({:error, any}, String.t) :: nil
  defp get_country({:error, reason}, ip) do
    Logger.error("Error Geolocating #{inspect ip} -> #{inspect reason}")
    nil
  end

Question

Which way is the correct way?
If they are both correct, which way is the most idiomatic/preferred?

3 Likes
2

I usually do option 3:

  @doc """
  Blah blah blah...
  """
  @spec get_country({:ok, [String.t]} | {:error, any}, String.t) :: String.t | nil
  def get_country(data, ip)

  def get_country({:ok, [country | _tail]}, _ip), do: String.upcase(country)

  def get_country({:error, reason}, ip) do
    Logger.error("Error Geolocating #{inspect ip} -> #{inspect reason}")
    nil
  end

Of course defp’s don’t have the doc but I use the same principle there for consistency.

4 Likes
3

What are the advantages of option 3, compares to the other two?

4

Well it’s really only an improvement over option 1, making it clearer (to me) that the annotations apply to all of the clauses instead of just one. But I did not know you could have a spec for every body, I might start using that. :thinking:

5

I prefer Option 1 because I can see all the relevant specs at a glance.

Really the key point is to be consistent as both options work.

I would use Option 3 if get_country has a precondition.

6

What about the fact that option 2 is clearer on the intention which clause processes what “types”?

The only problem I see is that dialyzer doesn’t validate the intention given that all clauses belong to the same function - so it’s only verifying what option 1 implies.

  # in one place but still separate
  @spec get_country({:ok, [String.t]}, String.t) :: String.t 
  @spec get_country({:error, any}, String.t) :: nil

  defp get_country({:ok, [country | _tail]}, _ip), do: String.upcase(country)

  defp get_country({:error, reason}, ip) do
    Logger.error("Error Geolocating #{inspect ip} -> #{inspect reason}")
    nil
  end

AFAIK in Erlang one is forced to put all the multi clause specs in the same place (so option 2 wouldn’t exist):

-spec plus(integer(), float())   -> float();
          (float(),   integer()) -> float();
          (float(),   float())   -> float();
          (integer(), integer()) -> integer().
7

Option 2 gives me the impression there are two different functions and not two separate function clauses.

However, either works, as long as someone is consistent using it through out the entire project.

8

Would dialyzer analyze all 3 options in the same way? Does it care?

9

Option 1 and 3 are similar. I never tried Option 2 and can’t comment from personal experience but it should be similar.

10

I don’t think it makes a distinction. I think I ran into a case where I had something like:

  @spec get_country({:ok, [String.t]}, String.t) :: String.t 
  @spec get_country({:error, any}, String.t) :: nil

and it wouldn’t complain about a

  @spec get_country({:error, any}, String.t) :: String.t

clause.

I understand the preference of option 1 for a user perspective - but I prefer option 2 (or 2a) from a code review perspective because it tells me which “case” the code is supposed to be dealing with.

2 Likes
11

But if option 2 makes so that dialyzer doesn’t fully complain when it should, then it is technically incorrect. This is my fear.

12 
@spec get_country({:ok, [String.t]}, String.t) :: String.t 
@spec get_country({:error, any}, String.t) :: nil

Should be the same as:

@spec get_country({:ok, [String.t]} | {:error, any}, String.t) :: String.t | nil

So it cannot complain about @spec get_country({:error, any}, String.t) :: String.t, because it’s a valid subset.

1 Like
13

As far as dialyzer is concerned it’s not incorrect.

  @spec get_country({:ok, [String.t]}, String.t) :: String.t 
  @spec get_country({:error, any}, String.t) :: nil

… I think it’s the mental model we assume when we see the above that is incorrect.

The impression I was getting is that it simply collects accepted input shapes and output shapes separately and validates against those - it doesn’t correlate input shapes to output shapes.

And given that we are talking about one-and-the-same-function it makes sense given there is no such thing as a function-clause-type but only a function-type.

So

@spec get_country({:ok, [String.t]}, String.t) :: String.t 
@spec get_country({:error, any}, String.t) :: nil

accumulates to

@spec get_country({:ok, [String.t]} | {:error, any}, String.t) :: String.t | nil

I just like documenting the separate cases as they are handled by the code (i.e. aligning with the clauses that implement them).

And there are cases where you can’t spec separate clauses:

  -spec foo(pos_integer()) -> pos_integer()
         ; (integer()) -> integer().

generates a warning because

  -spec foo(integer()) -> integer().

already covers everything.

So it could prove difficult to be consistent with option 2 - keeping the “clause specs” in the same place could mitigate that somewhat.

1 Like
14

So long as there are no overlapping domains, you can use as many @specs as desired