Elixir: is_binary(Path.join(['a'])) == false

[andreao]

Path.join(['a']) == 'a'
Path.join(['a','b']) == "a/b"

Documentation says output of Path.join/1 should be binary.
Also Path.join([]) throws FunctionClauseError, which is undocumented.

Erlang/OTP 19 [erts-8.2] [source]
Elixir 1.4.0

[whatyouhide]

I think the spec for this function should be join(nonempty(t)), not join([t]), that would at least document somehow that the list cannot be empty. 🤔

[andreao]

@whatyouhide I agree. Java Paths.get and Python os.path.join disallows empty.

[OvermindDL1]

Based on the documentation of Path.join/1 it states that:

Joins a list of strings.

You are giving it a list of character lists (or an empty list), which is not following the documented contract.

However, based on https://github.com/elixir-lang/elixir/blob/master/lib/elixir/lib/path.ex#L469-L473 and https://github.com/elixir-lang/elixir/blob/master/lib/elixir/lib/path.ex#L490-L499 it seems like it does do a character list to binary conversion on line https://github.com/elixir-lang/elixir/blob/master/lib/elixir/lib/path.ex#L492 when the length of the list is >1...

Overall it seems this function was designed for binaries only, and 'kind-of' works on character lists in some (read: very few) situations (like a single-element list of a character-list). It should probably have a binary guard of the elements put on it, that way all of your above cases would fail with an exception properly.

And yep, the spec needs fixing, empty should not be allowed. :-)

[andreao]

@OvermindDL1: You are right, I didn't actually follow the contract. It's a very easy mistake to make, especially when working with erlang libraries, so I agree a binary guard is a good solution. Thanks for quick feedback :)

[whatyouhide]

But the spec is [t] (should be nonempty_list(t) as we said), and Path.t is :unicode.chardata:

iex> Path.join [[?a, "foo", [?8, ?9]], "bar"]
"afoo89/bar"

so I don't think strictly checking for binaries is the right thing to do? Maybe we should just fix the documentation saying "takes a (nonempty) list of paths" or something like this :)

[OvermindDL1]

so I don't think strictly checking for binaries is the right thing to do? Maybe we should just fix the documentation saying "takes a (nonempty) list of paths" or something like this :)

If so then at least one case that I immediately see would need fixing at line: https://github.com/elixir-lang/elixir/blob/master/lib/elixir/lib/path.ex#L473

  def join([name]), do:
    name

Because right now it returns a non-binary if the element there is, for example, an iolist. Easy fix though.

If this function is supposed to be able to take a single list of :unicode.chardata and return a binary only then this conversion here needs to be completed, and perhaps in other locations as well though I've not checked those?

[whatyouhide]

If so then at least one case that I immediately see would need fixing

@OvermindDL1 yes, that's exactly what caused the bug reported in this issue :).