Keyword.merge behaviour when a list is the second argument (docs/test)

[PragTob]

Environment

• Elixir version (elixir -v):

Erlang/OTP 19 [erts-8.0] [source] [64-bit] [smp:8:8] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.3.4

• Operating system:

tobi@happy ~ $ uname -a
Linux happy 3.19.0-32-generic #37~14.04.1-Ubuntu SMP Thu Oct 22 09:41:40 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux

Current behavior

The current behaviour is that when Keyword.merge is called with a normal list as the second argument it just appends the list.

iex(1)> Keyword.merge [a: 1], [b: 2]
[a: 1, b: 2]
iex(2)> Keyword.merge [a: 1], [2, 3]
[{:a, 1}, 2, 3]

Expected behavior

Not sure, part of what this is about :) I see 3 options:

1. throw an error (as the second argument is no keyword list).
2. The current behavior can be argued for and could be declared wanted behaviour, in that case I'd want to add a unit test for it and some documentation (there doesn't seem to be any for this case atm)
3. it could be declared "undefined behaviour" and marked as such

I'd have expected 1.) to happen but through the internal representation can also understand that 2.) might be a good course of action.

Thanks for all your effort + Cheers,
Tobi

[eksperimental]

Great catch @PragTob
I think we should throw an error.

looking at the source code this function is not optimized, as we traverse keyword2, as many times as keyword1 elements we have.
I can work on it unless @PragTob is planning on taking a stab.

[PragTob]

I'd love to take a stab at this :) Not sure I'm proficient enough with Elixir yet to fix the performance but I might as well try and learn :D

Which reminds me, Map.merge/3 is also significantly slower than Map.merge/2 - might be related.

[josevalim]

I am not sure yet how we should solve this issue but it is worth mentioning that most functions in keyword will accept "improper" keywords:

Keyword.get [:a, :b, c: 3], :c
3

[whatyouhide]

@PragTob Map.merge/2 is faster than Map.merge/3 because it calls to maps:merge/2 which is implemented in C, while Map.merge/3 manually builds the resulting map with maps:fold/3.

[whatyouhide]

What do we need to discuss in order to move this forward? :)

[PragTob]

As I understood it's me getting up my lazy... bottom to try and implement it that it only really works with kwlists but I haven't gotten to it for #reasons. So to not block this further, please anyone go ahead and implement it - I might still if I get to it.

Thanks for triaging issues @whatyouhide ! :)

[eksperimental]

@josevalim specs mention that both args should be keywords ([{atom, any}]). Will it be considered a backward incompatible change if we throw an error when one is not provided?

[josevalim]

Looking at the history, we changed the Keyword module in the past to raise for functions that expected a Keyword but a mixed list was given.

On the other hand, we had to do changes like this one in the past to support mixed formats when interfacing with Erlang.

Balancing the facts, I believe we should remain "pure", i.e. only keywords list allowed, because the cases we interface with Erlang are rather rare.

So we should fix this, it is mostly a matter of finding the most efficient implementation.

[josevalim]

@eksperimental I don't believe it is backwards incompatible to change merge. I am unsure if we should change get though because of efficiency. Furthermore, we should be super diligent with building (i.e. we should not build invalid keywords), but it is a lesser problem when reading. Similar to how Strings rely on auto synchronization when reading UTF-8.

[eksperimental]

so should we fix any other function that takes improper lists or non-keyword lists?