Elixir: Enum.into/2 is broken for keyword lists

[josevalim]

See example below:

iex(6)> Enum.into([a: 2], %{a: 1})[:a]
2
iex(7)> Enum.into([a: 2], [a: 1])[:a]
1

The map result is correct, as the left side overrides the right side. To be more precise, Enum.into/2 for two lists should be equivalent to left ++ right. We need to change both the shortcuts in Enum.into/2 as well as the collectable implementation for lists.

[whatyouhide]

@josevalim if we make Enum.into/2 be enumerable ++ collectable, then we will break the current behaviour:

iex> Enum.into([1, 2, 3], [:a, :b])
[:a, :b, 1, 2, 3]

right?

[josevalim]

Right, that's my point though, the current behaviour is broken. For example, even the behaviour we have for comprehensions is not useful because we just end-up prepending the value given to into to the generated comprehension and it would be much better if the into value was appended.

The other option is to deprecate giving a list to Enum.into so we can provide proper behaviour later on.

[whatyouhide]

@josevalim yeah I am not sure. So what you're saying is that "into" should work for basically linked lists instead of arrays? If we had lists we could easily append to, I would expect the behaviour today, that is, we "insert" the elements at the end, but I guess for linked lists it makes sense to inject elements at the head of the list. If this is what you mean, I may agree :P

However, I am not sure we can do this just like this. We could potentially break existing code. Would you consider this as a bug that application should have not relied on? It feels like an inconsistent behaviour but not what I would call buggy 🤔

[josevalim]

@whatyouhide so we have three options:

1. not change this
2. deprecate passing a non-empty list to collectable
3. change this now and risk breaking code

I am thinking 2 is the best way to go then?

FWIW, I would consider the keyword behaviour buggy.

[whatyouhide]

@josevalim yeah, I would guess 2. is the safest option we have. I wouldn't go with 1., and as I said 3. looks risky.

I would consider the keyword behaviour buggy.

Yeah I agree. Maybe we can go with 2. and deprecate non-empty lists to collectable, but actually check for a keyword list and if a kw list is passed then fix the behaviour. So we would "fix" the behaviour for kw lists but in a slower way (we need to check for them being kw lists), and deprecate (but leave untouched) any other non-empty list. Thoughts? Maybe this is too complex, but for the sake of discussion let's explore the possibility :)

[fishcakez]

@whatyouhide I have keyword code relying on the broken behavior, but there have been many more times when I wanted the "correct" behavior for any list (not just keyword) and wished into/2 worked as described in the top.

[whatyouhide]

Okay, so maybe we can just go with 2. and deprecate non-empty lists and call it a day for now, and change this on Elixir 2.0.

[michalmuskala]

FWIW I think that changing the behaviour would be too disruptive at that point. We also should be careful with the deprecation - I'd try to introduce it and try some sample applications to see how noisy that would be. If it's a lot of warnings, I'm afraid a simple deprecation would also be quite problematic 😕

[whatyouhide]

@michalmuskala I'll try that, sure.

[soteras]

iex> Enum.into([a: 2], [a: 1])[:a]
2

Is that the right result?

[josevalim]

Let's go with the deprecation for now then.

[josevalim]

Closing in favor of the PR.