Raise when given non-keyword-lists in Keyword.merge/3 (#5479)

This commit makes sure a valid keyword is given as both arguments.

Additionally, it adds tests to make sure Keyword.merge/2,3 work
exactly the same way if the function passed to merge/3 returns
the value from the second keyword list.

Similar to Similar to #5466.

Author: eksperimental

lib/elixir/lib/keyword.ex

@@ -618,26 +618,40 @@ defmodule Keyword do
       ...> end)
       [b: 2, a: 4, d: 4, a: 8]
 
+      iex> Keyword.merge([a: 1, b: 2], [:a, :b], fn :a, v1, v2 ->
+      ...>  v1 + v2
+      ...> end)
+      ** (ArgumentError) expected a keyword list as the second argument, got: [:a, :b]
+
   """
   @spec merge(t, t, (key, value, value -> value)) :: t
-  def merge(keywords1, keywords2, fun) when is_list(keywords1) and is_list(keywords2) do
-    do_merge(keywords2, [], keywords1, keywords1, fun)
+  def merge(keywords1, keywords2, fun) when is_list(keywords1) and is_list(keywords2) and is_function(fun, 3) do
+    if keyword?(keywords1) do
+      do_merge(keywords2, [], keywords1, keywords1, fun, keywords2)
+    else
+      raise ArgumentError, message: "expected a keyword list as the first argument, got: #{inspect keywords1}"
+    end
   end
 
-  defp do_merge([{k, v2} | t], acc, rest, original, fun) do
-    case :lists.keyfind(k, 1, original) do
-      {^k, v1} ->
-        do_merge(t, [{k, fun.(k, v1, v2)} | acc],
-                 delete(rest, k), :lists.keydelete(k, 1, original), fun)
+  defp do_merge([{key, value2} | tail], acc, rest, original, fun, keywords2) when is_atom(key) do
+    case :lists.keyfind(key, 1, original) do
+      {^key, value1} ->
+        do_merge(tail, [{key, fun.(key, value1, value2)} | acc],
+                 delete(rest, key), :lists.keydelete(key, 1, original), fun, keywords2)
+
       false ->
-        do_merge(t, [{k, v2} | acc], rest, original, fun)
+        do_merge(tail, [{key, value2} | acc], rest, original, fun, keywords2)
     end
   end
 
-  defp do_merge([], acc, rest, _original, _fun) do
+  defp do_merge([], acc, rest, _original, _fun, _keywords2) do
     rest ++ :lists.reverse(acc)
   end
 
+  defp do_merge(_other, _acc, _rest, _original, _fun, keywords2) do
+    raise ArgumentError, message: "expected a keyword list as the second argument, got: #{inspect keywords2}"
+  end
+
   @doc """
   Returns whether a given `key` exists in the given `keywords`.
 

lib/elixir/test/elixir/keyword_test.exs

@@ -74,4 +74,75 @@ defmodule KeywordTest do
     # any key in keywords1 is removed if key is present in keyword2
     assert Keyword.merge([a: 1, b: 2, c: 3, c: 4], [c: 11, c: 12, d: 13]) == [a: 1, b: 2, c: 11, c: 12, d: 13]
   end
+
+  test "merge/3" do
+    fun = fn _key, value1, value2 -> value1 + value2 end
+
+    assert Keyword.merge([a: 1, b: 2], [c: 11, d: 12], fun) == [a: 1, b: 2, c: 11, d: 12]
+    assert Keyword.merge([], [c: 11, d: 12], fun) == [c: 11, d: 12]
+    assert Keyword.merge([a: 1, b: 2], [], fun) == [a: 1, b: 2]
+
+    assert_raise ArgumentError, "expected a keyword list as the first argument, got: [1, 2]", fn ->
+      Keyword.merge([1, 2], [c: 11, d: 12], fun)
+    end
+
+    assert_raise ArgumentError, "expected a keyword list as the first argument, got: [1 | 2]", fn ->
+      Keyword.merge([1 | 2], [c: 11, d: 12], fun)
+    end
+
+    assert_raise ArgumentError, "expected a keyword list as the second argument, got: [{:x, 1}, :y, :z]", fn ->
+      Keyword.merge([a: 1, b: 2], [{:x, 1}, :y, :z], fun)
+    end
+
+    assert_raise ArgumentError, "expected a keyword list as the second argument, got: [:x | :y]", fn ->
+      Keyword.merge([a: 1, b: 2], [:x | :y], fun)
+    end
+
+    assert_raise ArgumentError, "expected a keyword list as the second argument, got: [{:x, 1} | :y]", fn ->
+      Keyword.merge([a: 1, b: 2], [{:x, 1} | :y], fun)
+    end
+
+    # duplicate keys in keywords1 are left untouched if key is not present in keywords2
+    assert Keyword.merge([a: 1, b: 2, a: 3], [c: 11, d: 12], fun) == [a: 1, b: 2, a: 3, c: 11, d: 12]
+    assert Keyword.merge([a: 1, b: 2, a: 3], [a: 11], fun) == [b: 2, a: 12]
+
+    # duplicate keys in keywords2 are always kept
+    assert Keyword.merge([a: 1, b: 2], [c: 11, c: 12, d: 13], fun) == [a: 1, b: 2, c: 11, c: 12, d: 13]
+
+    # every key in keywords1 is replaced with fun result if key is present in keyword2
+    assert Keyword.merge([a: 1, b: 2, c: 3, c: 4], [c: 11, c: 50, d: 13], fun) == [a: 1, b: 2, c: 14, c: 54, d: 13]
+  end
+
+  test "merge/2 and merge/3 behave exactly the same way" do
+    fun = fn _key, _value1, value2 -> value2 end
+
+    args = [
+      {[a: 1, b: 2], [c: 11, d: 12]},
+      {[], [c: 11, d: 12]},
+      {[a: 1, b: 2], []},
+      {[a: 1, b: 2, a: 3], [c: 11, d: 12]},
+      {[a: 1, b: 2, a: 3], [a: 11]},
+      {[a: 1, b: 2], [c: 11, c: 12, d: 13]},
+      {[a: 1, b: 2, c: 3, c: 4], [c: 11, c: 12, d: 13]},
+    ]
+
+    args_error = [
+      {[1, 2], [c: 11, d: 12]},
+      {[1 | 2], [c: 11, d: 12]},
+      {[a: 1, b: 2], [11, 12, 0]},
+      {[a: 1, b: 2], [11 | 12]},
+      {[a: 1, b: 2], [{:x, 1}, :y, :z]},
+      {[a: 1, b: 2], [:x | :y]},
+      {[a: 1, b: 2], [{:x, 1} | :y]},
+    ]
+
+    for {arg1, arg2} <- args do
+      assert Keyword.merge(arg1, arg2) == Keyword.merge(arg1, arg2, fun)
+    end
+
+    for {arg1, arg2} <- args_error do
+      error = assert_raise ArgumentError, fn -> Keyword.merge(arg1, arg2) end
+      assert_raise ArgumentError, error.message, fn -> Keyword.merge(arg1, arg2, fun) end
+    end
+  end
 end