Skip to content

Improve diagnostic for unclosed strings #13185

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

Closed
viniciusmuller wants to merge 1 commit into from
Closed

Improve diagnostic for unclosed strings #13185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lib/elixir/src/elixir_interpolation.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ extract_char(Rest, Buffer, Output, Line, Column, Scope, Interpol, Last) ->
Token = io_lib:format("\\u~4.16.0B", [Char]),
Pre = "invalid bidirectional formatting character in string: ",
Pos = io_lib:format(". If you want to use such character, use it in its escaped ~ts form instead", [Token]),
{error, {?LOC(Line, Column), {Pre, Pos}, Token}};
{error, {[{error_type, invalid_bidi} | ?LOC(Line, Column)], {Pre, Pos}, Token}};

[Char | NewRest] ->
extract(NewRest, [Char | Buffer], Output, Line, Column + 1, Scope, Interpol, Last);
Expand Down
12 changes: 11 additions & 1 deletion lib/elixir/src/elixir_tokenizer.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,16 @@ handle_heredocs(T, Line, Column, H, Scope, Tokens) ->
handle_strings(T, Line, Column, H, Scope, Tokens) ->
case elixir_interpolation:extract(Line, Column, Scope, true, T, H) of
{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
Copy link
Member

@josevalim josevalim Dec 14, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

@josevalim josevalim Dec 14, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@viniciusmuller viniciusmuller Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@josevalim josevalim Dec 14, 2023

Choose a reason for hiding this comment

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

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.

% Don't override other errors that may have happened downstream already
{error_type, mismatched_delimiter} -> Meta;
{error_type, invalid_bidi} -> lists:keydelete(error_type, 1, Meta);
_ ->
{line, EndLine} = lists:keyfind(line, 1, Meta),
[{line, Line}, {column, Column - 1}, {error_type, unclosed_delimiter}, {end_line, EndLine}, {opening_delimiter, '"'}]
end,
{error, {NewMeta, Message, Token}, NewRest, NewScope, NewTokens};

{NewLine, NewColumn, Parts, [$: | Rest], InterScope} when ?is_space(hd(Rest)) ->
NewScope = case is_unnecessary_quote(Parts, InterScope) of
Expand Down Expand Up @@ -1493,6 +1502,7 @@ terminator('(') -> ')';
terminator('[') -> ']';
terminator('{') -> '}';
terminator('"""') -> '"""';
terminator('"') -> '"';
Copy link
Member

@josevalim josevalim Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@viniciusmuller viniciusmuller Dec 14, 2023

Choose a reason for hiding this comment

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

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)

terminator('<<') -> '>>'.

%% Keywords checking
Expand Down
8 changes: 7 additions & 1 deletion lib/elixir/test/elixir/code_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,13 @@ defmodule CodeTest do
test "string_to_quoted returns error on incomplete escaped string" do
assert Code.string_to_quoted("\"\\") ==
{:error,
{[line: 1, column: 3], "missing terminator: \" (for string starting at line 1)", ""}}
{[
line: 1,
column: 1,
Copy link
Contributor Author

@viniciusmuller viniciusmuller Dec 14, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

@josevalim josevalim Dec 14, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

@josevalim josevalim Dec 14, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

@josevalim josevalim Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@viniciusmuller viniciusmuller Dec 14, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@viniciusmuller viniciusmuller Dec 14, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Member

@josevalim josevalim Dec 14, 2023

Choose a reason for hiding this comment

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

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

error_type: :unclosed_delimiter,
end_line: 1,
opening_delimiter: :"\""
], "missing terminator: \" (for string starting at line 1)", ""}}
end

test "compile source" do
Expand Down
25 changes: 25 additions & 0 deletions lib/elixir/test/elixir/kernel/diagnostics_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,31 @@ defmodule Kernel.DiagnosticsTest do
"""
end

test "missing string terminator" do
output =
capture_raise(
"""
a = "test string

IO.inspect(10 + 20)
""",
TokenMissingError
)

assert output == """
** (TokenMissingError) token missing on nofile:3:20:
error: missing terminator: " (for string starting at line 1)
│
1 │ a = "test string
│ └ unclosed delimiter
2 │
3 │ IO.inspect(10 + 20)
│ └ missing closing delimiter (expected ")
│
└─ nofile:3:20\
"""
end

test "shows in between lines if EOL is not far below" do
output =
capture_raise(
Expand Down