Elixir: existing_atoms_only: true raises with an unhelpful error message

[blaketan]

When using Code.string_to_quoted/2 and Code.string_to_quoted!/2 with existing_atoms_only: true, any identifiers that do not already exist as an atom results in a ArgumentError with no other details. I'm hoping that we can return a better error here, hopefully with line and token, which would be really helpful to use cases where we try to convert potentially unsafe inputs into quoted form.

Environment

• Elixir & Erlang versions (elixir --version): Elixir 1.5.2, OPT 20.1
• Operating system: macOS Sierra

Current behavior

iex(1)> Code.string_to_quoted "existence_is_pain", existing_atoms_only: true
** (ArgumentError) argument error
    :erlang.list_to_existing_atom('existence_is_pain')
    (elixir) src/elixir_tokenizer.erl:679: :elixir_tokenizer.unsafe_to_atom/3
    (elixir) src/elixir_tokenizer.erl:891: :elixir_tokenizer.tokenize_identifier/3
    (elixir) src/elixir_tokenizer.erl:477: :elixir_tokenizer.tokenize/5
iex(1)> Code.string_to_quoted! "existence_is_pain", existing_atoms_only: true
** (ArgumentError) argument error
    :erlang.list_to_existing_atom('existence_is_pain')
    (elixir) src/elixir_tokenizer.erl:679: :elixir_tokenizer.unsafe_to_atom/3
    (elixir) src/elixir_tokenizer.erl:891: :elixir_tokenizer.tokenize_identifier/3
    (elixir) src/elixir_tokenizer.erl:477: :elixir_tokenizer.tokenize/5

Expected behavior

iex(1)> Code.string_to_quoted "existence_is_pain", existing_atoms_only: true
{:error, {1, "unsafe atom does not exist: ", "existence_is_pain"}}
iex(2)> Code.string_to_quoted! "existence_is_pain", existing_atoms_only: true
** (SyntaxError) nofile:1: unsafe atom does not exist: existence_is_pain
    (stdlib) erl_eval.erl:670: :erl_eval.do_apply/6
    (iex) lib/iex/evaluator.ex:245: IEx.Evaluator.handle_eval/5
    (iex) lib/iex/evaluator.ex:225: IEx.Evaluator.do_eval/3
    (iex) lib/iex/evaluator.ex:203: IEx.Evaluator.eval/3
    (iex) lib/iex/evaluator.ex:89: IEx.Evaluator.loop/1
    (iex) lib/iex/evaluator.ex:24: IEx.Evaluator.init/4

I have a fix here that implements the expected behavior above, but I think it might be deceptive to raise an SyntaxError, and it might be better to raise a new exception BadAtomError/NoExistingAtomError? Would be happy to change implementation and open a PR.

[josevalim]

Haha, I actually saw this today and though "the error message is really bad, i will fix it later". I guess later arrived sooner than I expected. :)

I think we can keep it as argument error and just improve the error message to say something.

[blaketan]

That'll be an improvement over the current behavior of ArgumentError: argument error for sure.

We'd be forced to encode file, line and token information into the error message though (if we want the error message to be helpful), instead of having a struct with file and line attributes.

[josevalim]

We could do that but it would be encoded in the text and not be fetchable programmatically. We can also emit a parser error or compile error but both feel out of place?

[josevalim]

Ah, we can also include the atom name. That may even be enough information since the atom name also has to be written literally in the text.

[blaketan]

My suggestion of having an separate exception type was to help keep the exception messages and structs raised by Code.string_to_quoted!/2 consistent between SyntaxError and TokenMissingError, both of which have [:file, :line, :description] attributes and pretty much format their messages similarly.

Although honestly that might be too nitpicky, and I'd be happy to match against the exception message of ArgumentError as well.

Should the plain Code.string_to_quoted/2 still raise as well (current behavior), or return an error tuple in this case?

[josevalim]

Should the plain Code.string_to_quoted/2 still raise as well (current behavior), or return an error tuple in this case?

That's a good point and pretty much dictates how we should handle this. If we want to return an error tuple, then it will pretty much end-up as a SyntaxError or ParserError.

[blaketan]

It may break with current behavior, but it feels like an anti-pattern to have to handle both an error tuple and a raised exception from a function call. It may be better to standardize the plain version to return only in the tuple format, and raise in the string_to_quoted! case.

[josevalim]

Agreed, the error message with file and line also makes it worth it. Closing this in favor of the PR!