Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Improve error messages for dot syntax #12003

Merged
merged 3 commits into from
Jul 26, 2022
Merged

Proposal: Improve error messages for dot syntax #12003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4dcfb9
Improve error messages for dot syntax
sabiwara Jul 21, 2022
d341950
Apply suggestions
sabiwara Jul 25, 2022
55a3e64
Remove extra clause
sabiwara Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions lib/elixir/lib/exception.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -824,8 +824,8 @@ defmodule ArgumentError do
not is_atom(module) and is_atom(function) and args == [] ->
"you attempted to apply a function named #{inspect(function)} on #{inspect(module)}. " <>
"If you are using Kernel.apply/3, make sure the module is an atom. " <>
"If you are using the dot syntax, such as map.field or module.function(), " <>
"make sure the left side of the dot is an atom or a map"
"If you are using the dot syntax, such as module.function(), " <>
"make sure the left-hand side of the dot is a module atom"

not is_atom(module) ->
"you attempted to apply a function on #{inspect(module)}. " <>
Expand Down Expand Up @@ -1114,8 +1114,8 @@ defmodule UndefinedFunctionError do
end

defp hint(nil, _function, 0, _loaded?) do
". If you are using the dot syntax, such as map.field or module.function(), " <>
"make sure the left side of the dot is an atom or a map"
". If you are using the dot syntax, such as module.function(), " <>
"make sure the left-hand side of the dot is a module atom"
end

defp hint(module, function, arity, true) do
Expand Down Expand Up @@ -1635,10 +1635,19 @@ defmodule ErlangError do
%KeyError{key: key, term: term}
end

def normalize({:badkey, key, map}, _stacktrace) do
def normalize({:badkey, key, map}, _stacktrace) when is_map(map) do
%KeyError{key: key, term: map}
end

def normalize({:badkey, key, term}, _stacktrace) do
message =
"key #{inspect(key)} not found in: #{inspect(term)}. " <>
"If you are using the dot syntax, such as map.field, " <>
"make sure the left-hand side of the dot is a map"

%KeyError{key: key, term: term, message: message}
end

def normalize({:case_clause, term}, _stacktrace) do
%CaseClauseError{term: term}
end
Expand Down
54 changes: 37 additions & 17 deletions lib/elixir/src/elixir_erl_pass.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,30 +224,50 @@ translate({{'.', _, [Left, Right]}, Meta, []}, _Ann, #elixir_erl{context=guard}
TRight = {atom, Ann, Right},
{?remote(Ann, erlang, map_get, [TRight, TLeft]), SL};

translate({{'.', _, [Left, Right]}, Meta, []}, _Ann, S) when is_tuple(Left), is_atom(Right), is_list(Meta) ->
translate({{'.', _, [Left, Right]}, Meta, []}, _Ann, S)
when is_tuple(Left) orelse Left =:= nil orelse is_boolean(Left), is_atom(Right), is_list(Meta) ->
Ann = ?ann(Meta),
{TLeft, SL} = translate(Left, Ann, S),
TRight = {atom, Ann, Right},

Generated = erl_anno:set_generated(true, Ann),
{Var, SV} = elixir_erl_var:build('_', SL),
TVar = {var, Generated, Var},
TError = {tuple, Ann, [{atom, Ann, badkey}, TRight, TVar]},

{{'case', Generated, TLeft, [
{clause, Generated,
[{map, Ann, [{map_field_exact, Ann, TRight, TVar}]}],
[],
[TVar]},
{clause, Generated,
[TVar],
[[?remote(Generated, erlang, is_map, [TVar])]],
[?remote(Ann, erlang, error, [TError])]},
{clause, Generated,
[TVar],
[],
[{call, Generated, {remote, Generated, TVar, TRight}, []}]}
]}, SV};

case proplists:get_value(no_parens, Meta, false) of
true ->
TError = {tuple, Ann, [{atom, Ann, badkey}, TRight, TVar]},
{{'case', Generated, TLeft, [
{clause, Generated,
[{map, Ann, [{map_field_exact, Ann, TRight, TVar}]}],
[],
[TVar]},
{clause, Generated,
[TVar],
[[
?remote(Generated, erlang, is_atom, [TVar]),
{op, Generated, '=/=', TVar, {atom, Generated, nil}},
{op, Generated, '=/=', TVar, {atom, Generated, true}},
{op, Generated, '=/=', TVar, {atom, Generated, false}}
]],
[{call, Generated, {remote, Generated, TVar, TRight}, []}]},
{clause, Generated,
[TVar],
[],
[?remote(Ann, erlang, error, [TError])]}
]}, SV};
false ->
{{'case', Generated, TLeft, [
{clause, Generated,
[{map, Ann, [{map_field_exact, Ann, TRight, TVar}]}],
[],
[TVar]},
{clause, Generated,
[TVar],
[],
[{call, Generated, {remote, Generated, TVar, TRight}, []}]}
Comment on lines +261 to +268
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I removed the is_map -> badkey clause here:

iex(1)> x = %{}; x.foo()
** (ArgumentError) you attempted to apply a function named :foo on %{}. If you are using Kernel.apply/3, make sure the module is an atom. If you are using the dot syntax, such as module.function(), make sure the left-hand side of the dot is a module atom

The happy path is still supported:

iex(1)> x = %{foo: 1}; x.foo()
1

but we don't assume this is what the user was trying to achieve when it doesn't. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an issue to make x.foo() on a map warn (see #9510). We can work on it as soon as we branch out for v1.14 (hopefully this week).

]}, SV}
end;

translate({{'.', _, [Left, Right]}, Meta, Args}, _Ann, S)
when (is_tuple(Left) orelse is_atom(Left)), is_atom(Right), is_list(Meta), is_list(Args) ->
Expand Down
27 changes: 22 additions & 5 deletions lib/elixir/test/elixir/exception_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,15 +478,15 @@ defmodule ExceptionTest do
end

test "annotates badarg on apply" do
assert blame_message([], & &1.foo) ==
assert blame_message([], & &1.foo()) ==
"you attempted to apply a function named :foo on []. If you are using Kernel.apply/3, make sure " <>
"the module is an atom. If you are using the dot syntax, such as " <>
"map.field or module.function(), make sure the left side of the dot is an atom or a map"
"module.function(), make sure the left-hand side of the dot is a module atom"

assert blame_message([], &apply(&1, :foo, [])) ==
"you attempted to apply a function named :foo on []. If you are using Kernel.apply/3, make sure " <>
"the module is an atom. If you are using the dot syntax, such as " <>
"map.field or module.function(), make sure the left side of the dot is an atom or a map"
"module.function(), make sure the left-hand side of the dot is a module atom"

assert blame_message([], &apply(Kernel, &1, [1, 2])) ==
"you attempted to apply a function named [] on module Kernel. However [] is not a valid function name. " <>
Expand Down Expand Up @@ -588,10 +588,27 @@ defmodule ExceptionTest do
"function :erlang.hash/2 is undefined or private, use erlang:phash2/2 instead"
end

test "annotates undefined function clause error with nil hints" do
test "annotates undefined key error with nil hints" do
assert blame_message(nil, & &1.foo) ==
"key :foo not found in: nil. If you are using the dot syntax, " <>
"such as map.field, make sure the left-hand side of the dot is a map"

# we use `Code.eval_string/1` to escape the formatter and warnings
assert blame_message("nil.foo", &Code.eval_string/1) ==
"key :foo not found in: nil. If you are using the dot syntax, " <>
"such as map.field, make sure the left-hand side of the dot is a map"
end

test "annotates undefined function clause error with nil hints" do
assert blame_message(nil, & &1.foo()) ==
"function nil.foo/0 is undefined. If you are using the dot syntax, " <>
"such as module.function(), make sure the left-hand side of " <>
"the dot is a module atom"

assert blame_message("nil.foo()", &Code.eval_string/1) ==
"function nil.foo/0 is undefined. If you are using the dot syntax, " <>
"such as map.field or module.function(), make sure the left side of the dot is an atom or a map"
"such as module.function(), make sure the left-hand side of " <>
"the dot is a module atom"
end

test "annotates key error with suggestions if keys are atoms" do
Expand Down
2 changes: 1 addition & 1 deletion lib/elixir/test/elixir/kernel/errors_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ defmodule Kernel.ErrorsTest do
"defmodule MisplacedOperator, do: (def bar(1 | 2), do: :ok)"
end

defp bad_remote_call(x), do: x.foo
defp bad_remote_call(x), do: x.foo()

defmacro sample(0), do: 0

Expand Down