From b4cb1fe0054285632446ff79cf3059ba84af4b48 Mon Sep 17 00:00:00 2001 From: Devon Estes Date: Tue, 26 Jun 2018 19:02:31 +0200 Subject: [PATCH 1/5] Add `blame` callback for KeyError This starts with an initial implementation of the `blame/2` callback for `KeyError` to add some helpful `did_you_mean` feedback for potentially typo'd keys. Right now it's only implemented for maps and keyword lists, and only for string and atom keys. --- lib/elixir/lib/exception.ex | 77 +++++++++++++++++++++-- lib/elixir/test/elixir/exception_test.exs | 36 +++++++++++ 2 files changed, 108 insertions(+), 5 deletions(-) diff --git a/lib/elixir/lib/exception.ex b/lib/elixir/lib/exception.ex index 087431cc08..b1bf4e0a8e 100644 --- a/lib/elixir/lib/exception.ex +++ b/lib/elixir/lib/exception.ex @@ -1091,18 +1091,85 @@ defmodule Protocol.UndefinedError do end defmodule KeyError do - defexception [:key, :term] + defexception [:key, :term, :message] @impl true - def message(exception) do - msg = "key #{inspect(exception.key)} not found" + def message(exception = %{message: nil}), do: message(exception.key, exception.term) + def message(%{message: message}), do: message + + def message(key, term) do + msg = "key #{inspect(key)} not found" - if exception.term != nil do - msg <> " in: #{inspect(exception.term)}" + if term != nil do + msg <> " in: #{inspect(term)}" else msg end end + + @impl true + def blame(exception = %{term: nil}, stacktrace) do + message = message(exception.key, exception.term) + {%{exception | message: message}, stacktrace} + end + + def blame(exception = %{term: term}, stacktrace) when is_map(term) do + available_keys = Map.keys(term) + hint = did_you_mean(exception.key, available_keys) + message = message(exception.key, term) <> IO.iodata_to_binary(hint) + {%{exception | message: message}, stacktrace} + end + + def blame(exception = %{term: term}, stacktrace) when is_list(term) do + available_keys = Keyword.keys(term) + hint = did_you_mean(exception.key, available_keys) + message = message(exception.key, term) <> IO.iodata_to_binary(hint) + {%{exception | message: message}, stacktrace} + end + + def blame(exception, stacktrace), do: {exception, stacktrace} + + @threshold 0.77 + @max_suggestions 5 + defp did_you_mean(missing_key, available_keys) when is_atom(missing_key) do + missing_key + |> Atom.to_string() + |> did_you_mean(available_keys) + end + + defp did_you_mean(missing_key, available_keys) when is_binary(missing_key) do + result = + available_keys + |> Enum.map(&distance_for_key(&1, missing_key)) + |> Enum.filter(fn {dist, _} -> dist >= @threshold end) + |> Enum.sort(&(elem(&1, 0) >= elem(&2, 0))) + |> Enum.take(@max_suggestions) + |> Enum.sort(&(elem(&1, 1) <= elem(&2, 1))) + + case result do + [] -> [] + suggestions -> [". Did you mean one of:\n\n" | Enum.map(suggestions, &format_fa/1)] + end + end + + defp did_you_mean(_, _), do: [] + + defp distance_for_key(key, missing_key) when is_atom(key) do + stringified_key = Atom.to_string(key) + dist = String.jaro_distance(missing_key, stringified_key) + {dist, key} + end + + defp distance_for_key(key, missing_key) when is_binary(key) do + dist = String.jaro_distance(missing_key, key) + {dist, key} + end + + defp distance_for_key(key, _), do: {0, key} + + defp format_fa({_dist, key}) do + [" * ", inspect(key), ?\n] + end end defmodule UnicodeConversionError do diff --git a/lib/elixir/test/elixir/exception_test.exs b/lib/elixir/test/elixir/exception_test.exs index 88e39996ca..66ab620dda 100644 --- a/lib/elixir/test/elixir/exception_test.exs +++ b/lib/elixir/test/elixir/exception_test.exs @@ -482,6 +482,42 @@ defmodule ExceptionTest do "such as map.field or module.function, make sure the left side of the dot is an atom or a map" end + test "annotates key error with suggestions" do + message = blame_message(%{first: nil, second: nil}, fn map -> map.firts end) + + assert message == """ + key :firts not found in: %{first: nil, second: nil}. Did you mean one of: + + * :first + """ + + message = blame_message(%{"first" => nil, "second" => nil}, fn map -> map.firts end) + + assert message == """ + key :firts not found in: %{\"first\" => nil, \"second\" => nil}. Did you mean one of: + + * \"first\" + """ + + message = + blame_message(%{"first" => nil, "second" => nil}, fn map -> Map.fetch!(map, "firts") end) + + assert message == """ + key \"firts\" not found in: %{\"first\" => nil, \"second\" => nil}. Did you mean one of: + + * \"first\" + """ + + message = + blame_message([first: nil, second: nil], fn kwlist -> Keyword.fetch!(kwlist, :firts) end) + + assert message == """ + key :firts not found in: [first: nil, second: nil]. Did you mean one of: + + * :first + """ + end + defp blame_message(arg, fun) do try do fun.(arg) From b898ad6a76497eaab6f07e4573a21fcc5820e33e Mon Sep 17 00:00:00 2001 From: Devon Estes Date: Tue, 26 Jun 2018 19:39:11 +0200 Subject: [PATCH 2/5] Update a few variable names --- lib/elixir/lib/exception.ex | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/elixir/lib/exception.ex b/lib/elixir/lib/exception.ex index b1bf4e0a8e..a17957d584 100644 --- a/lib/elixir/lib/exception.ex +++ b/lib/elixir/lib/exception.ex @@ -1098,12 +1098,12 @@ defmodule KeyError do def message(%{message: message}), do: message def message(key, term) do - msg = "key #{inspect(key)} not found" + message = "key #{inspect(key)} not found" if term != nil do - msg <> " in: #{inspect(term)}" + message <> " in: #{inspect(term)}" else - msg + message end end @@ -1141,7 +1141,7 @@ defmodule KeyError do result = available_keys |> Enum.map(&distance_for_key(&1, missing_key)) - |> Enum.filter(fn {dist, _} -> dist >= @threshold end) + |> Enum.filter(fn {distance, _} -> distance >= @threshold end) |> Enum.sort(&(elem(&1, 0) >= elem(&2, 0))) |> Enum.take(@max_suggestions) |> Enum.sort(&(elem(&1, 1) <= elem(&2, 1))) @@ -1156,13 +1156,13 @@ defmodule KeyError do defp distance_for_key(key, missing_key) when is_atom(key) do stringified_key = Atom.to_string(key) - dist = String.jaro_distance(missing_key, stringified_key) - {dist, key} + distance = String.jaro_distance(missing_key, stringified_key) + {distance, key} end defp distance_for_key(key, missing_key) when is_binary(key) do - dist = String.jaro_distance(missing_key, key) - {dist, key} + distance = String.jaro_distance(missing_key, key) + {distance, key} end defp distance_for_key(key, _), do: {0, key} From accd2cb5b82ffa180ee2985c2ab106bb90b640c6 Mon Sep 17 00:00:00 2001 From: Devon Estes Date: Wed, 27 Jun 2018 09:35:57 +0200 Subject: [PATCH 3/5] Change behavior to only deal with atom keys --- lib/elixir/lib/exception.ex | 67 +++++++++-------------- lib/elixir/test/elixir/exception_test.exs | 14 +---- 2 files changed, 29 insertions(+), 52 deletions(-) diff --git a/lib/elixir/lib/exception.ex b/lib/elixir/lib/exception.ex index a17957d584..001b4b782c 100644 --- a/lib/elixir/lib/exception.ex +++ b/lib/elixir/lib/exception.ex @@ -1113,62 +1113,47 @@ defmodule KeyError do {%{exception | message: message}, stacktrace} end - def blame(exception = %{term: term}, stacktrace) when is_map(term) do - available_keys = Map.keys(term) - hint = did_you_mean(exception.key, available_keys) - message = message(exception.key, term) <> IO.iodata_to_binary(hint) - {%{exception | message: message}, stacktrace} - end + def blame(exception, stacktrace) do + %{term: term, key: key} = exception + message = message(key, term) - def blame(exception = %{term: term}, stacktrace) when is_list(term) do - available_keys = Keyword.keys(term) - hint = did_you_mean(exception.key, available_keys) - message = message(exception.key, term) <> IO.iodata_to_binary(hint) - {%{exception | message: message}, stacktrace} + if (is_map(term) or Keyword.keyword?(term)) and Enum.all?(term, fn {k, _} -> is_atom(k) end) do + hint = did_you_mean(key, available_keys(term)) + message = message <> IO.iodata_to_binary(hint) + {%{exception | message: message}, stacktrace} + else + {%{exception | message: message}, stacktrace} + end end - def blame(exception, stacktrace), do: {exception, stacktrace} + defp available_keys(term) when is_map(term), do: Map.keys(term) + defp available_keys(term) when is_list(term), do: Keyword.keys(term) @threshold 0.77 @max_suggestions 5 defp did_you_mean(missing_key, available_keys) when is_atom(missing_key) do - missing_key - |> Atom.to_string() - |> did_you_mean(available_keys) - end + stringified_key = Atom.to_string(missing_key) - defp did_you_mean(missing_key, available_keys) when is_binary(missing_key) do - result = - available_keys - |> Enum.map(&distance_for_key(&1, missing_key)) - |> Enum.filter(fn {distance, _} -> distance >= @threshold end) - |> Enum.sort(&(elem(&1, 0) >= elem(&2, 0))) - |> Enum.take(@max_suggestions) - |> Enum.sort(&(elem(&1, 1) <= elem(&2, 1))) + suggestions = + for key <- available_keys, + distance = String.jaro_distance(stringified_key, Atom.to_string(key)), + distance >= @threshold, + do: {distance, key} - case result do + case suggestions do [] -> [] - suggestions -> [". Did you mean one of:\n\n" | Enum.map(suggestions, &format_fa/1)] + suggestions -> [". Did you mean one of:\n\n" | format_suggestions(suggestions)] end end defp did_you_mean(_, _), do: [] - defp distance_for_key(key, missing_key) when is_atom(key) do - stringified_key = Atom.to_string(key) - distance = String.jaro_distance(missing_key, stringified_key) - {distance, key} - end - - defp distance_for_key(key, missing_key) when is_binary(key) do - distance = String.jaro_distance(missing_key, key) - {distance, key} - end - - defp distance_for_key(key, _), do: {0, key} - - defp format_fa({_dist, key}) do - [" * ", inspect(key), ?\n] + defp format_suggestions(suggestions) do + suggestions + |> Enum.sort(&(elem(&1, 0) >= elem(&2, 0))) + |> Enum.take(@max_suggestions) + |> Enum.sort(&(elem(&1, 1) <= elem(&2, 1))) + |> Enum.map(fn {_, key} -> [" * ", inspect(key), ?\n] end) end end diff --git a/lib/elixir/test/elixir/exception_test.exs b/lib/elixir/test/elixir/exception_test.exs index 66ab620dda..cb0900ba5a 100644 --- a/lib/elixir/test/elixir/exception_test.exs +++ b/lib/elixir/test/elixir/exception_test.exs @@ -482,7 +482,7 @@ defmodule ExceptionTest do "such as map.field or module.function, make sure the left side of the dot is an atom or a map" end - test "annotates key error with suggestions" do + test "annotates key error with suggestions if keys are atoms" do message = blame_message(%{first: nil, second: nil}, fn map -> map.firts end) assert message == """ @@ -493,20 +493,12 @@ defmodule ExceptionTest do message = blame_message(%{"first" => nil, "second" => nil}, fn map -> map.firts end) - assert message == """ - key :firts not found in: %{\"first\" => nil, \"second\" => nil}. Did you mean one of: - - * \"first\" - """ + assert message == "key :firts not found in: %{\"first\" => nil, \"second\" => nil}" message = blame_message(%{"first" => nil, "second" => nil}, fn map -> Map.fetch!(map, "firts") end) - assert message == """ - key \"firts\" not found in: %{\"first\" => nil, \"second\" => nil}. Did you mean one of: - - * \"first\" - """ + assert message == "key \"firts\" not found in: %{\"first\" => nil, \"second\" => nil}" message = blame_message([first: nil, second: nil], fn kwlist -> Keyword.fetch!(kwlist, :firts) end) From 797dfe9784821d9ec30220fa86ccfb67e99b43ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 27 Jun 2018 10:35:10 +0200 Subject: [PATCH 4/5] Update exception.ex --- lib/elixir/lib/exception.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/lib/exception.ex b/lib/elixir/lib/exception.ex index 001b4b782c..2dc0486d40 100644 --- a/lib/elixir/lib/exception.ex +++ b/lib/elixir/lib/exception.ex @@ -1117,7 +1117,7 @@ defmodule KeyError do %{term: term, key: key} = exception message = message(key, term) - if (is_map(term) or Keyword.keyword?(term)) and Enum.all?(term, fn {k, _} -> is_atom(k) end) do + if (is_map(term) and Enum.all?(term, fn {k, _} -> is_atom(k) end)) or Keyword.keyword?(term) do hint = did_you_mean(key, available_keys(term)) message = message <> IO.iodata_to_binary(hint) {%{exception | message: message}, stacktrace} From 2f058abaeea98a323db98325c35e03da73bd8a14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 27 Jun 2018 10:39:02 +0200 Subject: [PATCH 5/5] Update exception.ex --- lib/elixir/lib/exception.ex | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/elixir/lib/exception.ex b/lib/elixir/lib/exception.ex index 2dc0486d40..c5d462fddd 100644 --- a/lib/elixir/lib/exception.ex +++ b/lib/elixir/lib/exception.ex @@ -1117,7 +1117,7 @@ defmodule KeyError do %{term: term, key: key} = exception message = message(key, term) - if (is_map(term) and Enum.all?(term, fn {k, _} -> is_atom(k) end)) or Keyword.keyword?(term) do + if is_atom(key) and (map_with_atom_keys_only?(term) or Keyword.keyword?(term)) do hint = did_you_mean(key, available_keys(term)) message = message <> IO.iodata_to_binary(hint) {%{exception | message: message}, stacktrace} @@ -1126,12 +1126,16 @@ defmodule KeyError do end end + defp map_with_atom_keys_only?(term) do + is_map(term) and Enum.all?(term, fn {k, _} -> is_atom(k) end) + end + defp available_keys(term) when is_map(term), do: Map.keys(term) defp available_keys(term) when is_list(term), do: Keyword.keys(term) @threshold 0.77 @max_suggestions 5 - defp did_you_mean(missing_key, available_keys) when is_atom(missing_key) do + defp did_you_mean(missing_key, available_keys) do stringified_key = Atom.to_string(missing_key) suggestions = @@ -1146,8 +1150,6 @@ defmodule KeyError do end end - defp did_you_mean(_, _), do: [] - defp format_suggestions(suggestions) do suggestions |> Enum.sort(&(elem(&1, 0) >= elem(&2, 0)))