Improve diagnostic for unclosed strings #13185

viniciusmuller · 2023-12-14T10:21:56Z

lib/elixir/src/elixir_tokenizer.erl

viniciusmuller · 2023-12-14T10:25:56Z

lib/elixir/test/elixir/code_test.exs

-              {[line: 1, column: 3], "missing terminator: \" (for string starting at line 1)", ""}}
+              {[
+                 line: 1,
+                 column: 1,


I have my doubts about the correctness of this case here though

It is correct according to our definition of MissingTerminator. But you need to add end_column here.

Actually, maybe it should be swapped around. line and column always points to where the error happens and we have opening_line and opening_column to point to where it began.

@lukaszsamson do you have any opinion on what the editor should show here? The line where the identifier starts or is it the line where it is not closed (usually the last line of the file)? What about a mismatched delimiter? Do we show point to the opening one (incorrectly closed) or the closing one? It is hard to say because either side may be wrong.

The issue though with showing the end location is that we would need a negative span for the diagnostic.

I was also thinking about this, currently on the diagnostics that are printed, the location is shown as the file bottom, but it's not a problem there because the new snippet shows both the start and end

For a language server or editor extension that needs to show one point, I think it makes sense for us to use the starting one in the unclosed delimiter case, as the problem is always there and the end is always empty. For the mismatched delimiter I'm not sure

But you need to add end_column here

Currently our TokenMissingError does not support the end_column, we actually just calculate them when building the snippet

Should we update it so that we calculate the end column based on the snippet before building the exception? And if so, should we do it on this PR or another one?

I have created this issue to help with the discussion: #13187

josevalim · 2023-12-14T10:28:52Z

lib/elixir/src/elixir_tokenizer.erl

    {error, Reason} ->
-      interpolation_error(Reason, [H | T], Scope, Tokens, " (for string starting at line ~B)", [Line]);
+      {error, {Meta, Message, Token}, NewRest, NewScope, NewTokens} = interpolation_error(Reason, [H | T], Scope, Tokens, " (for string starting at line ~B)", [Line]),
+      NewMeta = case lists:keyfind(error_type, 1, Meta) of


Shouldn't this matching be handled inside interpolation_error instead? 🤔 This way we don't need to make changes to bidi (i.e. all other errors pass through).

I also think this code is called for 'foo'. So you need to use the variable H to detect which kind of delimiter we are expecting.

Shouldn't this matching be handled inside interpolation_error instead?

From my tests and understanding, we get the bidi error from elixir_interpolation:extract, and then interpolation_error just passes it through

I've updated bidi to add its error type so we can differentiate and not override it, because the way it currently is we receive the same error structure whether it's a bidi error or an unclosed string one

I also think this code is called for 'foo'

Great point, I'll do some tests with single-quote strings, I believe they should have this same behavior if unclosed, right?

I've updated bidi to add its error type so we can differentiate and not override it, because the way it currently is we receive the same error structure whether it's a bidi error or an unclosed string one

I think you only receive the same structure because you are intercepting the error. Instead of intercepting it, change where the error is raised to already include all metadata.

josevalim · 2023-12-14T10:31:30Z

lib/elixir/src/elixir_tokenizer.erl

 terminator('[')  -> ']';
 terminator('{')  -> '}';
 terminator('"""') -> '"""';
+terminator('"')  -> '"';


I have been thinking for a while that we should include the expected_delimiter directly in the exception struct. The fact we are adding new entries here make me thing we should go ahead with adding these fields. WDYT?

I guess that's fine, I see no problem in doing it this way as well, but perhaps by also adding it to the exception makes it a bit more complete

In the end I think that the least work we have to do in the exception code and can do it before is better, because the overall diagnostics get closer to the diagnostics we print to the terminal as well (although in some cases it will probably better to just keep them different)

josevalim · 2023-12-14T16:03:06Z

I will push a commit soon. It turns out that it was simpler to tackle all of them together, because they all use the same exception handling already, and tackling them separately was making things more complicated than it had to. :)

Closes #13183. Closes #13185. Closes #13186. Closes #13187.

viniciusmuller · 2023-12-14T17:08:02Z

@josevalim that's a lot of productivity 😆
thank you

Improve diagnostic for unclosed strings

6c8b0bb

viniciusmuller commented Dec 14, 2023

View reviewed changes

lib/elixir/src/elixir_tokenizer.erl Show resolved Hide resolved

viniciusmuller commented Dec 14, 2023

View reviewed changes

josevalim reviewed Dec 14, 2023

View reviewed changes

josevalim added a commit that referenced this pull request Dec 14, 2023

Normalize token missing and mismatched delimiter exceptions

d23e42e

Closes #13183. Closes #13185. Closes #13186. Closes #13187.

josevalim closed this in 4d712b2 Dec 14, 2023

Improve diagnostic for unclosed strings #13185

Improve diagnostic for unclosed strings #13185

Uh oh!

Conversation

viniciusmuller commented Dec 14, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim Dec 14, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viniciusmuller Dec 14, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

josevalim Dec 14, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

josevalim commented Dec 14, 2023

Uh oh!

viniciusmuller commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

josevalim Dec 14, 2023 •

edited

Loading

viniciusmuller Dec 14, 2023 •

edited

Loading

josevalim Dec 14, 2023 •

edited

Loading