Proposal: Improve error messages for dot syntax #12003

sabiwara · 2022-07-22T14:46:03Z

Current behavior

iex(3)> nil.id
** (UndefinedFunctionError) function nil.id/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
    nil.id()

The message looks very obvious to an Elixir dev used to it, but it could quite a lot to unpack and some prior knowledge is assumed:

nil is an atom
modules are atoms, so nil gets interpreted as a module
elixir doesn't distinguish between user.id and user.id() because of how its AST works

Besides, this message might be slightly sub-optimal:

AFAIK nil, true and false are not actually allowed module names, despite being atoms
"If you are using the dot syntax" -> this comes from apply/3, but in this case we know the dot syntax was used
the expression had no_parens, so it might be better to assume it was an attempt to access a map key than an undefined function? (and maybe mention nil.id/0 as a second scenario to be exhaustive)

Proposed behaviour

Exact error messages and strategy TBD :)

iex(1)> nil.id
** (ArgumentError) could not find key :id on nil. When using the map.field syntax, make sure the left side of the dot is a map
iex(1)> nil.id()
** (ArgumentError) could not find function :id on nil. When using the module.function() syntax, make sure the left side of the dot is a module

sabiwara · 2022-07-22T14:49:54Z

lib/elixir/src/elixir_erl_pass.erl

@@ -242,11 +243,22 @@ translate({{'.', _, [Left, Right]}, Meta, []}, _Ann, S) when is_tuple(Left), is_
    {clause, Generated,
      [TVar],
      [[?remote(Generated, erlang, is_map, [TVar])]],
-      [?remote(Ann, erlang, error, [TError])]},
+      [?remote(Ann, erlang, error, [TBadKeyError])]},
+    {clause, Generated,


I'm creating more clauses directly in the generated code: should I be delegating to some dot_apply function instead? (that would be @doc false maybe?)

We definitely want to avoid generating additional code and we want to avoid changing the semantics of the failure. My suggestion is the following:

If no_parens is true, we generate the existing bad key error (and we can improve the BadKeyError message as a whole)

If no_parens is false, we raise the undefined function error (and we can improve the UndefinedFunctionError as a whole)

I know there will still be some ambiguity in relation to apply or Map.fetch(nil, ...) but I think it is important to not add new exception types.

In the future, we can use the error_info from EEP54 to add some special annotation (and annotate that we were indeed in a dot or a similar).

In the future, we can use the error_info from EEP54 to add some special annotation (and annotate that we were indeed in a dot or a similar).

Oh, I see, this is the piece I was missing to be able to propagate this information without making a mess. Will wait for EEP54 for the dot-aware messages.

we want to avoid changing the semantics of the failure

Makes perfect sense, will try the approach you suggested 👍

EEP54 requires OTP 24, so we should add that particular bit later.

sabiwara · 2022-07-22T14:53:07Z

lib/elixir/test/elixir/exception_test.exs

+      assert blame_message(nil, fn _ -> nil.foo() end) ==
+               "could not find function :foo on nil. When using the module.function() " <>
+                 "syntax, make sure the left side of the dot is a module"


this is actually going in a different branch as blame_message(nil, & &1.foo()), which is why I added a test

this triggers a compile-time warning, any idea how I could silence it? warning: nil.foo/0 is undefined

You need to use eval.

whatyouhide · 2022-07-24T20:57:47Z

lib/elixir/lib/exception.ex

+    %ArgumentError{
+      message:
+        "could not find key #{inspect(key)} on #{inspect(map)}. " <>
+          "When using the map.field syntax, make sure the left side of the dot is a map"


Not a native speaker here, but I've mostly seen this referred to as

Suggested change

"When using the map.field syntax, make sure the left side of the dot is a map"

"When using the map.field syntax, make sure the left-hand side of the dot is a map"

?

Wikipedia agrees, sounds good.

I copied this from the existing hint, after grepping through the codebase it seems we are currently using all of the following:

left side

left hand side

left-hand side (the correct one)

We might want to fix the existing ones as well in a separate PR, WDYT?

whatyouhide · 2022-07-24T20:58:10Z

lib/elixir/lib/exception.ex

+  def normalize({:baddot, false, key, module}, _stacktrace) do
+    %ArgumentError{
+      message:
+        "could not find function #{inspect(key)} on #{inspect(module)}. " <>


I think we generally talk about functions being in modules?

Suggested change

"could not find function #{inspect(key)} on #{inspect(module)}. " <>

"could not find function #{inspect(key)} in #{inspect(module)}. " <>

sabiwara · 2022-07-25T00:20:12Z

lib/elixir/src/elixir_erl_pass.erl

+        {clause, Generated,
+          [{map, Ann, [{map_field_exact, Ann, TRight, TVar}]}],
+          [],
+          [TVar]},
+        {clause, Generated,
+          [TVar],
+          [],
+          [{call, Generated, {remote, Generated, TVar, TRight}, []}]}


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?

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).

sabiwara · 2022-07-25T00:32:44Z

Current behavior:

`x`	`x.id` or `x.id()`	`x.foo(:bar)` (for comparison)
`%{}`	`** (KeyError) key :id not found in: %{}`	`** (ArgumentError) you attempted to apply a function on %{}. Modules (the first argument of apply) must always be an atom`
`0`	`** (ArgumentError) you attempted to apply a function named :id on 0. 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`	`** (ArgumentError) you attempted to apply a function on 0. Modules (the first argument of apply) must always be an atom`
`nil`	`** (UndefinedFunctionError) function nil.id/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`	`** (UndefinedFunctionError) function nil.foo/1 is undefined`

Proposed behavior:

`x`	`x.id`	`x.id()`
`%{}`	`** (KeyError) key :id not found in: %{}`	`** (ArgumentError) you attempted to apply a function named :id 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`
`0`	`** (KeyError) key :id not found in: 0. If you are using the dot syntax, such as map.field, make sure the left-hand side of the dot is a map`	`** (ArgumentError) you attempted to apply a function named :id on 0. 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`
`nil`	`** (KeyError) key :id 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`	`** (UndefinedFunctionError) function nil.id/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`

josevalim · 2022-07-25T07:14:40Z

100% agreed on the proposed behaviour! Great summary!

josevalim · 2022-07-26T08:56:49Z

lib/elixir/src/elixir_erl_pass.erl

+translate_remote(Left, Right, Meta, [], S)
+    when (Left =:= nil orelse is_boolean(Left)), is_atom(Right) ->
+  Ann = ?ann(Meta),
+  TLeft = {atom, Ann, Left},
+  TRight = {atom, Ann, Right},
+  case proplists:get_value(no_parens, Meta, false) of
+    true ->
+      TError = {tuple, Ann, [{atom, Ann, badkey}, TRight, TLeft]},
+      {?remote(Ann, erlang, error, [TError]), S};
+    false ->
+      {{call, Ann, {remote, Ann, TLeft, TRight}, []}, S}
+  end;


I think we don't need this, correct? we can just emit the original code and let it fail as usual?

This is the case where nil.id is being called (as opposed to x = nil; x.id).

Without this, we would get:

But maybe there's a better way: we can replace the translate clause above to check for these earlier: 55a3e64

WDYT?

The tradeoff is that we generate a bit of a garbage case for nil.id, but this should not really be a thing for production code anyway, so it probably doesn't have to be optimal, it just have to be correct when you try it out in IEx?

josevalim · 2022-07-26T10:20:26Z

💚 💙 💜 💛 ❤️

sabiwara commented Jul 22, 2022

View reviewed changes

whatyouhide reviewed Jul 24, 2022

View reviewed changes

sabiwara added 2 commits July 25, 2022 08:59

Improve error messages for dot syntax

f4dcfb9

Apply suggestions

d341950

sabiwara force-pushed the improve-dot-error-message branch from f38b14a to d341950 Compare July 25, 2022 00:13

sabiwara commented Jul 25, 2022

View reviewed changes

sabiwara marked this pull request as ready for review July 26, 2022 00:26

josevalim reviewed Jul 26, 2022

View reviewed changes

Remove extra clause

55a3e64

josevalim merged commit 1184fca into elixir-lang:main Jul 26, 2022

sabiwara deleted the improve-dot-error-message branch July 26, 2022 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Improve error messages for dot syntax #12003

Proposal: Improve error messages for dot syntax #12003

sabiwara commented Jul 22, 2022

sabiwara Jul 22, 2022

josevalim Jul 22, 2022

sabiwara Jul 22, 2022

josevalim Jul 24, 2022

sabiwara Jul 22, 2022

josevalim Jul 24, 2022

whatyouhide Jul 24, 2022

sabiwara Jul 24, 2022

whatyouhide Jul 24, 2022

sabiwara Jul 25, 2022

josevalim Jul 25, 2022

sabiwara commented Jul 25, 2022

josevalim commented Jul 25, 2022

josevalim Jul 26, 2022

sabiwara Jul 26, 2022

sabiwara Jul 26, 2022 •

edited

Loading

josevalim commented Jul 26, 2022

	"When using the map.field syntax, make sure the left side of the dot is a map"
	"When using the map.field syntax, make sure the left-hand side of the dot is a map"

	"could not find function #{inspect(key)} on #{inspect(module)}. " <>
	"could not find function #{inspect(key)} in #{inspect(module)}. " <>

Proposal: Improve error messages for dot syntax #12003

Proposal: Improve error messages for dot syntax #12003

Conversation

sabiwara commented Jul 22, 2022

Current behavior

Proposed behaviour

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabiwara commented Jul 25, 2022

Current behavior:

Proposed behavior:

josevalim commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabiwara Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

josevalim commented Jul 26, 2022

sabiwara Jul 26, 2022 •

edited

Loading