After sleeping on it, here is my criticism of inline annotations.
Too much repetition
For example, take encode_kv_pair. The different clauses are effectively variations of the same type, which are repeated over and over again:
defp encode_kv_pair({key, _}: QueryPair, _encoding: EncodingType) -> no_return when is_list(key)
defp encode_kv_pair({_, value}: QueryPair, _encoding: EncodingType) -> no_return when is_list(value)
defp encode_kv_pair({key, value}: QueryPair, :rfc3986) -> String
defp encode_kv_pair({key, value}: QueryPair, :www_form) -> String
In the example above, there was even a need to rely on inference to reduce some of the repetition. With a separate clause, the type signature is defined once:
$ query_pair(), encoding_type() -> string()
defp encode_kv_pair({key, _}, _encoding) when is_list(key)
defp encode_kv_pair({_, value}, _encoding) when is_list(value)
defp encode_kv_pair({key, value}, :rfc3986)
defp encode_kv_pair({key, value}, :www_form)
While I definitely prefer the second one, and some of it can be described as taste, there is no discussion the second one is more concise and less repetitive. You can see this happening over and over when comparing snippets, in almost every function that has more than one clause.
It obscures the actual signature
One of the benefits of type signatures is to provide a brief description of what the function does. However, if you only have inline type annotations, this can become very hard. For example, let’s look at your merge function and have it with the actual code:
def merge(%URI{scheme: nil} = uri, _rel: t | String) -> no_return do
raise ArgumentError, "you must merge onto an absolute URI"
end
def merge(base: t | String, %URI{scheme: rel_scheme} = rel) -> t when rel_scheme != nil do
%{rel | path: remove_dot_segments_from_path(rel.path)}
end
def merge(%URI{} = base, %URI{host: host} = rel) -> t when host != nil do
%{rel | scheme: base.scheme, path: remove_dot_segments_from_path(rel.path)}
end
def merge(%URI{} = base, %URI{path: nil} = rel) -> t do
%{base | query: rel.query || base.query, fragment: rel.fragment}
end
def merge(%URI{host: nil, path: nil} = base, %URI{} = rel) -> t do
%{
base
| path: remove_dot_segments_from_path(rel.path),
query: rel.query,
fragment: rel.fragment
}
end
def merge(%URI{} = base, %URI{} = rel) -> t do
new_path = merge_paths(base.path, rel.path)
%{base | path: new_path, query: rel.query, fragment: rel.fragment}
end
def merge(base: String, rel: String) -> t do
merge(parse(base), parse(rel))
end
It is really hard to tell the arguments it receives and the expected return types. You need to go clause by clause, build which one overlaps and which ones do not in our head. Maybe each of them handle a different type, maybe not.
However, if you have a separate type declaration at the top, regardless of the syntax, then it is immediately clear:
def merge(base_uri: t | String, relative_uri: t | String) -> t
# or
$ t | String, t | String -> t
def merge(uri_or_string, uri_or_string)
Incorrect type annotations
Some of your examples have clearly invalid type annotations. Let’s keep annotation and code together once more:
defp merge_paths(base_path: nil, rel_path: String?) -> String? do
merge_paths("/", rel_path)
end
defp merge_paths(base_path: String?, "/" <> _ = rel_path: String?) -> String? do
remove_dot_segments_from_path(rel_path)
end
defp merge_paths(base_path: String?, rel_path: String?) -> String? do
(path_to_segments(base_path) ++ [:+] ++ path_to_segments(rel_path))
|> remove_dot_segments([])
|> join_reversed_segments()
end
The second clause says "/" <> _ = rel_path: String? but it clearly cannot handle nils. The last clause says it deals with nils, but it does not. Here is another one:
# First argument doesn't deal with all Segments, only with part of them (empty lists)
defp remove_dot_segments([]: Segments, acc: Segments) -> Segments
We see a similar mistakes in the return types to split_authority and hex_to_dec. Furthermore, because you were relying on inference, many of these mistakes have been hidden. If you fully type the arguments, you will see this popping up more and more.
Of course, the type system would find these bugs in practice, but the fact those issues are popping so frequently shows there are fundamental semantic inconsistencies with inline type annotations, which we will explore next.
Type aliases awkwardness
Inference is a great feature to have and most of our work so far has been on inference. However, many teams on statically typed languages prefer to rely on inference as little as possible. Especially because inference may hide bugs in certian cases. For example, you chose to rely on inference for decode_with_encoding, to avoid repetition:
defp decode_with_encoding(string: String, :www_form) -> String
defp decode_with_encoding(string: String, :rfc3986) -> String
However, this clause has one issue: if you change EncodingType to have a new entry, you won’t have a typing violation in this function, because nowhere you defined it is supposed to handle all EncodingTypes. While in this case you will likely get a warning anyway, because it is all defined in the same module, you won’t be able to rely on inference when implementing clauses for a type alias defined in another module (as I showed in log_payment_status earlier).
Of course, the answer would be to annotate those types:
defp decode_with_encoding(string: String, :www_form: EncodingType) -> String
defp decode_with_encoding(string: String, :rfc3986: EncodingType) -> String
However, per the previous section, the definition above is invalid. A type alias means, by definition, that if you have typealias Alias = Type1 or Type2, you can replace the Alias by Type1 or Type2. That’s how they behave in all typed programming languages. This means you literally wrote this signature:
defp decode_with_encoding(string: String, :www_form: :www_form | :rfc3986) -> String
defp decode_with_encoding(string: String, :rfc3986: :www_form | :rfc3986) -> String
which, per above, is clearly wrong.
This puts us in a pickle. If type inference won’t help us catch type alias changing and we cannot use the type alias, there is literally nowhere we can annotate that this function is meant to handle all EncodingType, unless we define the type annotation separately. This is what Wojtek and I referred to earlier in this thread in the hostname function. The issue was not the syntax, the issue is that inline annotations are semantically at odds with pattern matching, which is accentuated by type aliases.
Summary
There are a couple other issues with your inline type annotations, such as the syntax being fundamentally incompatible (anyone who disagrees is welcome to change the parser and prove me wrong), but hopefully the above is enough to show they have enough syntactical and semantic issues when typing existing code. And that’s within a single module! The Elixir repository has 447 modules and over 5700 public functions, while the URI module has 29 of them. So these issued popped up when typing 0.5% of a single codebase.
But how can we be certain that having type annotations apart is better? Well, that’s how we have been adding annotations for the last 10+ years via typespecs, and new type system is meant to improve on the flaws of the existing typespecs. I acknowledge there are a separate discussion to have about syntax, in this thread someone already asked about parens being required or optional, or even the $ of itself, but it is clear having type annotations separate from clauses is the superior choice.
Finally, I have to say it is a bit frustrating that at no moment none of the cons above have been mentioned, posing inline type annotations as having only benefits and no trade-offs. In particular, I disagree with almost all items from your summary:
- No mental mapping of positional types to parameters
I actually agree with this one, having to positionally map types to arguments is one of the downsides of having them separate.
- Self-documenting function signatures
As shown above, that’s clearly false. Whenever a function has more than one clause, I need to parse through every single annotation to figure out the types it accepts and returns. The combination of inline annotations with type inference in your latest snippet only makes this harder and defeats any self-documenting purpose.
- Easier refactoring - types move with parameters
I disagree. While inline annotations makes it easier to move code to a new place, it comes with the huge downside that you can break code when you move it around, because you change the specification and implementation at the same time. Given the purpose of types is to help us find bugs, I’d rather err on making sure bugs do not go undetected.
- Better IDE support potential for inline type information
I disagree. I see no reason why IDEs would struggle with any of the approaches.
- Pattern-aware - leverages Elixir’s existing pattern matching strengths
Strongly disagree. Mixing inline annotations with pattern matching leads to excessive typing violations, as explained at length above, especially when aliases are used. And when relying on type inference to avoid repetition, as done above, allows bugs to creep in.
Still, I appreciate the time for this discussion because I know others would have the same questions. Now we can hopefully put this particular topic past us and focus on approaches more suitable to the language. Thank you.
)
