Show code snippet on syntax and token missing errors #11332

v0idpwn · 2021-10-22T11:27:15Z

Closes #11280.
Mostly works, there are 3~4 tests that legitimately fail.

Need to carefully refactor as I've been smashing keyboard until it works. 🥲

If possible, some directions would help me: is the approach that I took okay? Is there anything big that must be done differently?

josevalim · 2021-10-22T11:32:51Z

Overall the approach looks good to me. I would do the following changes:

call it "Snippet" instead of "Hint"
store the snippet as a field in the SyntaxError and TokenMissingError messages
then change the Exception.message/1 callback to include the snippet

Another option is to send the whole file to the exception and do all of the work in computing the snippet in the Exception.message/1 callback. But I am not sure if it would be better or worse. Thoughts?

v0idpwn · 2021-10-22T11:37:54Z

Agree 100% with 1 and 2!

Instead of sending the whole file to the exception, I was thinking about sending it to parse_error and doing the snippet part there. Then I could send the snippet to the exception. Do you think it is okay?

josevalim · 2021-10-22T11:52:20Z

Yeah.

v0idpwn · 2021-10-22T14:39:02Z

There are some places where the error line is available, but not the column. How should these be handled?

As of now (pushing here soon), I'm printing the line, but not the column indicator. Looks a bit weird, maybe nothing should be printed.

josevalim · 2021-10-22T15:10:53Z

Can you share those cases? I would be more inclined to not show anything.

josevalim · 2021-10-22T15:12:01Z

Btw, once this is done and merged, I will gladly accept PRs for EEx and HEEx in LiveView too.

v0idpwn · 2021-10-22T15:52:18Z

Strings that go through the error flow without a column, extracted from the test suite:

":foo.Bar"
"fn 1 end"
"2 -> 3 end"
"foo (hello, world)"
"[foo 1, 2]"
"[foo bar 1, 2]"
"[do: foo 1, 2]"
"foo(do: bar 1, 2)"
"{foo 1, 2}"
"{foo bar 1, 2}"
"foo 1, foo 2, 3"
"foo 1, @bar 3, 4"
"foo 1, 2 + bar 3, 4"
"foo(1, foo 2, 3)"
"if true do:"
"foo..bar baz//bat"
"foo++bar//bat"
"foo..(bar//bat)"
"call foo: 1, :bar"
"call(foo: 1, :bar)"
"[foo: 1, :bar]"
"%{foo: 1, :bar => :bar}"

josevalim · 2021-10-22T15:56:39Z

I see. All syntax error coming from the parser. :( let me play with this a bit because Erlang has to have addressed them somehow.

josevalim · 2021-10-22T19:22:02Z

Please rebase, we should have the column everywhere now. But keep in mind that it can be turned off, so it is still important to check if the column is there (and skip the snippet if the column is not there).

v0idpwn · 2021-10-23T20:20:28Z

Okay, there's a few more things to refactor (mainly move part of the logic to exception, also writing tests), but I think it works for real now. The one thing that I didn't like was adding 2 (!) new arguments to parse_error.

I think the main thing that is left to handle is the formatting of the message. Currently it looks like this:

** (SyntaxError) broken_foo.ex:3:9: syntax error before: '*'
%     3|    a + * b
%      |        ^

The first message implementation is really naive and doesn't handle most of the stuff. When we have a decision on how the message should look like, I can reimplement it.

My first suggestion is:

use % as the starting symbol
6 reserved characters before the |, in this sequence: [space, space, digit_or_space, digit_or_space, digit, space]
if there are more than 3 digits, append as many as necessary
have a space between | and code.

So it would look something like:
Simple case:

** (SyntaxError) broken_foo.ex:3:9: syntax error before: '*'
%    3 |     a + * b
%      |         ^

No indentation case:

** (SyntaxError) broken_foo.ex:3:9: syntax error before: '*'
%    3 | a + * b
%      |     ^

Many digits case:

** (SyntaxError) broken_foo.ex:3:9: syntax error before: '*'
%  11320 |     a + * b
%        |         ^

lib/elixir/lib/code.ex

josevalim

Looks great! Just one minor comment on parse_error API. I understand the goal is to mirror location, but location was designed to mirror the meta node and I don't think we need to replicate it here. :)

lib/elixir/lib/exception.ex

josevalim · 2021-10-25T11:33:13Z

lib/elixir/src/elixir_errors.erl

+    {column, Column} ->
+      Lines = string:split(InputString, "\n", all),
+      Snippet = elixir_utils:characters_to_binary(lists:nth(Line - StartLine + 1, Lines)),
+      #{content => Snippet, column => (Column - StartColumn + 1)};


I think you need to subtract start_column only if the line is the same as the start_line.

Also, columns start at 0, so we don't need the + 1. It only works because you do column - 1 in format_snippet. :)

Here are two tests that you can add:

Code.string_to_quoted!("2 + * 3", column: 3) Code.string_to_quoted!(":ok\n2 + * 3", column: 3)

Also, what if we rename column to something else to avoid confusion? Maybe offset?

lib/elixir/src/elixir_errors.erl

Co-authored-by: José Valim <jose.valim@gmail.com>

v0idpwn · 2021-10-25T12:28:57Z

lib/ex_unit/test/ex_unit/doc_test_test.exs

+                Doctest did not compile, got: (TokenMissingError) test/ex_unit/doc_test_test.exs:#{starting_line + 69}:7: missing terminator: } (for "{" starting at line #{starting_line + 69})
+                 #{line_placeholder(starting_line + 69)} |
+                 #{starting_line + 69} | {:ok, #MapSet<[1, 2, 3]>}
+                 #{line_placeholder(starting_line + 69)} |       ^. If you are planning to assert on the result of an iex> expression which contains a value inspected as #Name<...>, please make sure the inspected value is placed at the beginning of the expression; otherwise Elixir will treat it as a comment due to the leading sign #.


Perhaps we should add a newline by the beginning of this hint?

Yes, we should definitely.

josevalim · 2021-10-25T14:18:33Z

💚 💙 💜 💛 ❤️

v0idpwn force-pushed the feat/show-code-snippet-on-syntax-error branch from 5e92ea4 to 7710441 Compare October 23, 2021 13:13

maennchen approved these changes Oct 23, 2021

View reviewed changes

v0idpwn force-pushed the feat/show-code-snippet-on-syntax-error branch 3 times, most recently from fa9aaaa to 487ff7c Compare October 23, 2021 20:09

v0idpwn force-pushed the feat/show-code-snippet-on-syntax-error branch 2 times, most recently from dbb65c5 to 8be83a6 Compare October 24, 2021 14:23

v0idpwn added 2 commits October 24, 2021 19:16

Show snippet on syntax and token missing errors

860b6b7

Change message format

46872ea

v0idpwn force-pushed the feat/show-code-snippet-on-syntax-error branch from 7204872 to 46872ea Compare October 24, 2021 16:16

josevalim reviewed Oct 24, 2021

View reviewed changes

lib/elixir/lib/code.ex Outdated Show resolved Hide resolved

josevalim reviewed Oct 24, 2021

View reviewed changes

v0idpwn added 2 commits October 24, 2021 19:32

Handle snippet inside exception

1cbc306

Move snippet formatting inside of exception

2a71178

josevalim reviewed Oct 24, 2021

View reviewed changes

lib/elixir/lib/exception.ex Outdated Show resolved Hide resolved

josevalim reviewed Oct 24, 2021

View reviewed changes

lib/elixir/lib/exception.ex Outdated Show resolved Hide resolved

josevalim mentioned this pull request Oct 25, 2021

Show code snippet on SyntaxError and TokenMissingError #11280

Closed

v0idpwn added 2 commits October 25, 2021 13:53

Handle start columns case

0ab69e2

Improve :elixir_errors.parse_error api

5e28b79

josevalim reviewed Oct 25, 2021

View reviewed changes

lib/elixir/src/elixir_errors.erl Outdated Show resolved Hide resolved

v0idpwn and others added 2 commits October 25, 2021 15:07

Formatting suggestion

36bbcbd

Co-authored-by: José Valim <jose.valim@gmail.com>

Rename snippet column to snippet offset

5b8773e

v0idpwn force-pushed the feat/show-code-snippet-on-syntax-error branch from a21683f to 5b8773e Compare October 25, 2021 12:19

v0idpwn commented Oct 25, 2021

View reviewed changes

tests

bd3d7aa

josevalim merged commit edcb909 into elixir-lang:master Oct 25, 2021

v0idpwn deleted the feat/show-code-snippet-on-syntax-error branch October 27, 2021 11:48

Show code snippet on syntax and token missing errors #11332

Show code snippet on syntax and token missing errors #11332

Uh oh!

Conversation

v0idpwn commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josevalim commented Oct 22, 2021

Uh oh!

v0idpwn commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josevalim commented Oct 22, 2021

Uh oh!

v0idpwn commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josevalim commented Oct 22, 2021

Uh oh!

josevalim commented Oct 22, 2021

Uh oh!

v0idpwn commented Oct 22, 2021

Uh oh!

josevalim commented Oct 22, 2021

Uh oh!

josevalim commented Oct 22, 2021

Uh oh!

v0idpwn commented Oct 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

josevalim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

josevalim Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

josevalim Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

v0idpwn Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

josevalim Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

josevalim commented Oct 25, 2021

Uh oh!

Uh oh!

v0idpwn commented Oct 22, 2021 •

edited

Loading

v0idpwn commented Oct 22, 2021 •

edited

Loading

v0idpwn commented Oct 22, 2021 •

edited

Loading

v0idpwn commented Oct 23, 2021 •

edited

Loading

josevalim Oct 25, 2021 •

edited

Loading

josevalim Oct 25, 2021 •

edited

Loading