Fix HashDict so that key comparison uses exact match

This change means that HashDict buckets are nolonger strictly ordered.
Previously keys which compared equal (==) but not exactly equal (===)
would be considered different for large sizes but equivalent for small
sizes. This ambiguity occured because hashes are only equal when terms
are exactly equal (===), so both now use exact (===) key comparisons.
Small HashDicts are still a single ordered list and large HashDicts
contain ordered buckets. Both of these may contain keys that compare
equal (==) but are not exactly equal (===), should these keys be present
in the same list or bucket the ordering of these keys (relative to each
other) is undefined.

See github issue #1171 for examples.

Note: If assumptions were made about the form of a HashDict they may
nolonger be valid. Particularly that HashDicts can nolonger be reliably
pattern matched on for equality.

Author: fishcakez

lib/elixir/lib/hash_dict.ex

@@ -209,24 +209,14 @@ defmodule HashDict do
     ordered()
   end
 
-  def equal?(ordered(bucket: a, size: size), ordered(bucket: b, size: ^size)) do
-    a == b
-  end
-
-  def equal?(trie(size: size) = a, trie(size: ^size) = b) do
-    a == b
-  end
-
-  def equal?(ordered() = a, trie() = b) do
-    equal?(b, a)
-  end
-
-  def equal?(trie(size: size) = a, ordered(bucket: b, size: ^size)) do
-    :lists.keysort(1, to_list(a)) == :lists.keysort(1, b)
-  end
-
-  def equal?(_, _) do
-    false
+  def equal?(dict1, dict2) do
+    size = elem(dict1, 1)
+    case elem(dict2, 1) do
+      ^size ->
+        dict_equal?(dict1, dict1)
+      _ ->
+        false
+    end
   end
 
   @doc """
@@ -237,7 +227,7 @@ defmodule HashDict do
   end
 
   def to_list(dict) do
-    dict_fold(dict, [], [&1|&2]) |> :lists.reverse
+    dict_fold(dict, [], [&1|&2])
   end
 
   @doc """
@@ -335,11 +325,11 @@ defmodule HashDict do
   ## Dict-wide functions
 
   defp dict_get(ordered(bucket: bucket), key) do
-    :lists.keyfind(key, 1, bucket)
+    bucket_get(bucket, key)
   end
 
   defp dict_get(trie(root: root, depth: depth), key) do
-    :lists.keyfind(key, 1, node_bucket(root, depth, bucket_hash(key)))
+    bucket_get(node_bucket(root, depth, bucket_hash(key)), key)
   end
 
   defp dict_fold(ordered(bucket: bucket), acc, fun) do
@@ -402,28 +392,64 @@ defmodule HashDict do
     end
   end
 
+  defp dict_equal?(dict1, dict2) do
+    fun = fn({ key, value }, acc) ->
+      case fetch(dict2, key) do
+        { _ok, ^value } ->
+          acc
+        { _ok, _other} ->
+          throw(:error)
+        :error ->
+          throw(:error)
+      end
+    end
+    try do
+      reduce(dict1, true, fun)
+    catch
+      :error ->
+        false
+    end
+  end
+
   ## Bucket helpers
 
-  # Puts a value in the bucket
-  defp bucket_put([{k,_}=e|bucket], key, { :put, value }) when key < k do
-    { [{key,value},e|bucket], 1 }
+  # Get value from the bucket
+  defp bucket_get([{k,_}|_bucket], key) when k > key do
+    :false
   end
 
-  defp bucket_put([{k,_}=e|bucket], key, { :update, initial, _fun }) when key < k do
-    { [{key,initial},e|bucket], 1 }
+  defp bucket_get([{key,_}=e|_bucket], key) do
+    e
   end
 
-  defp bucket_put([{k,_}=e|bucket], key, value) when key > k do
-    { rest, count } = bucket_put(bucket, key, value)
-    { [e|rest], count }
+  defp bucket_get([_e|bucket], key) do
+    bucket_get(bucket, key)
   end
 
-  defp bucket_put([{_,_}|bucket], key, { :put, value }) do
+  defp bucket_get([], _key) do
+    :false
+  end
+
+  # Puts a value in the bucket
+  defp bucket_put([{k,_}|_]=bucket, key, { :put, value }) when k > key do
+    { [{key, value}|bucket], 1 }
+  end
+
+  defp bucket_put([{k,_}|_]=bucket, key, { :update, initial, _fun }) when k > key do
+    { [{key, initial}|bucket], 1 }
+  end
+
+  defp bucket_put([{key,_}|bucket], key, { :put, value }) do
     { [{key,value}|bucket], 0 }
   end
 
-  defp bucket_put([{_,value}|bucket], key, { :update, _initial, fun }) do
-    { [{key,fun.(value)}|bucket], 0 }
+  defp bucket_put([{key,value}|bucket], key, { :update, _initial, fun }) do
+    { [{key, fun.(value)}|bucket], 0 }
+  end
+
+  defp bucket_put([e|bucket], key, value) do
+    { rest, count } = bucket_put(bucket, key, value)
+    { [e|rest], count }
   end
 
   defp bucket_put([], key, { :put, value }) do
@@ -436,23 +462,23 @@ defmodule HashDict do
 
   # Puts a value in the bucket without returning
   # the operation value
-  defp bucket_put!([{k,_}=e|bucket], key, value) when key < k, do: [{key,value},e|bucket]
-  defp bucket_put!([{k,_}=e|bucket], key, value) when key > k, do: [e|bucket_put!(bucket, key, value)]
-  defp bucket_put!([{_,_}|bucket], key, value), do: [{key,value}|bucket]
+  defp bucket_put!([{k,_}|_]=bucket, key, value) when k > key, do: [{key,value}|bucket]
+  defp bucket_put!([{key,_}|bucket], key, value), do: [{key,value}|bucket]
+  defp bucket_put!([{_,_}=e|bucket], key, value), do: [e|bucket_put!(bucket, key, value)]
   defp bucket_put!([], key, value), do: [{key,value}]
 
   # Deletes a key from the bucket
-  defp bucket_delete([{k,_}|_] = bucket, key) when key < k do
+  defp bucket_delete([{k,_}|_]=bucket, key) when k > key do
     { bucket, nil, 0 }
   end
 
-  defp bucket_delete([{k,_}=e|bucket], key) when key > k do
-    { rest, value, count } = bucket_delete(bucket, key)
-    { [e|rest], value, count }
+  defp bucket_delete([{key,value}|bucket], key) do
+    { bucket, value, -1 }
   end
 
-  defp bucket_delete([{_,value}|bucket], _key) do
-    { bucket, value, -1 }
+  defp bucket_delete([e|bucket], key) do
+    { rest, value, count } = bucket_delete(bucket, key)
+    { [e|rest], value, count }
   end
 
   defp bucket_delete([], _key) do

lib/elixir/test/elixir/hash_dict_test.exs

@@ -62,6 +62,11 @@ defmodule HashDictTest do
     dict = HashDict.put_new(dict, 11, 13)
     assert HashDict.get(dict, 11) == 13
     assert HashDict.size(dict) == 9
+
+    dict = HashDict.put_new(dict, 11.0, 15)
+    assert HashDict.get(dict, 11.0) == 15
+    assert HashDict.get(dict, 11) == 13
+    assert HashDict.size(dict) == 10
   end
 
   test :update do
@@ -82,6 +87,17 @@ defmodule HashDictTest do
     dict = HashDict.update(dict, 11, 13, &1 * 2)
     assert HashDict.get(dict, 11) == 13
     assert HashDict.size(dict) == 9
+
+    assert_raise KeyError, fn->
+      HashDict.update(dict, 11.0, &1 * 2)
+    end
+    dict = HashDict.update(dict, 11.0, 15, &1 * 2)
+    assert HashDict.get(dict, 11.0) == 15
+    assert HashDict.size(dict) == 10
+    dict = HashDict.update(dict, 11.0, 15, &1 * 2)
+    assert HashDict.get(dict, 11.0) == 30
+    assert HashDict.get(dict, 11) == 13
+    assert HashDict.size(dict) == 10
   end
 
   test :to_list do
@@ -95,13 +111,13 @@ defmodule HashDictTest do
     list = dict |> HashDict.to_list
     assert length(list) == 20
     assert { 1, 1 } in list
-    assert list == Enum.to_list(dict)
+    assert :lists.keysort(1, list) == :lists.keysort(1, Enum.to_list(dict))
 
     dict = filled_dict(120)
     list = dict |> HashDict.to_list
     assert length(list) == 120
     assert { 1, 1 } in list
-    assert list == Enum.to_list(dict)
+    assert :lists.keysort(1, list) == :lists.keysort(1, Enum.to_list(dict))
   end
 
   test :keys do

lib/elixir/test/elixir/keyword_test.exs

@@ -25,7 +25,7 @@ defmodule KeywordTest do
     list = [{:b,2},{:a,1},{:c,3}]
     dict = HashDict.new list
     assert Keyword.from_enum(list) == [b: 2, a: 1, c: 3]
-    assert Keyword.from_enum(dict) == [a: 1, b: 2, c: 3]
+    assert Keyword.equal?(Keyword.from_enum(dict), [a: 1, b: 2, c: 3])
   end
 
   test :keyword? do

lib/elixir/test/elixir/uri_test.exs

@@ -23,16 +23,16 @@ defmodule URITest do
   end
 
   test :decode_query do
-    assert URI.decode_query("q=search%20query&cookie=ab%26cd&block%20buster=") ==
-                HashDict.new [{"block buster", ""}, {"cookie", "ab&cd"}, {"q", "search query"}]
-    assert URI.decode_query("") == HashDict.new
-    assert URI.decode_query("something=weird%3Dhappening") == HashDict.new [{"something", "weird=happening"}]
+    assert HashDict.equal?(URI.decode_query("q=search%20query&cookie=ab%26cd&block%20buster="),
+      HashDict.new [{"block buster", ""}, {"cookie", "ab&cd"}, {"q", "search query"}])
+    assert HashDict.equal?(URI.decode_query(""), HashDict.new)
+    assert HashDict.equal?(URI.decode_query("something=weird%3Dhappening"), HashDict.new [{"something", "weird=happening"}])
 
     assert URI.decode_query("", []) == []
 
-    assert URI.decode_query("garbage")                   == HashDict.new [{"garbage", nil}]
-    assert URI.decode_query("=value")                    == HashDict.new [{"", "value"}]
-    assert URI.decode_query("something=weird=happening") == HashDict.new [{"something", "weird=happening"}]
+    assert HashDict.equal?(URI.decode_query("garbage"), HashDict.new [{"garbage", nil}])
+    assert HashDict.equal?(URI.decode_query("=value"), HashDict.new [{"", "value"}])
+    assert HashDict.equal?(URI.decode_query("something=weird=happening"), HashDict.new [{"something", "weird=happening"}])
   end
 
   test :decoder do