Skip to content

Fix: string/symbol array literals recognize nested delimiters and escaped whitespace #5667

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

Merged
RX14 merged 1 commit into from
Feb 2, 2018
Merged

Fix: string/symbol array literals recognize nested delimiters and escaped whitespace #5667

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
18 changes: 18 additions & 0 deletions spec/compiler/parser/parser_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,8 +1615,26 @@ describe "Parser" do
assert_syntax_error %(case x; when /x/; 2; when /x/; end), "duplicate when /x/ in case"
assert_syntax_error %(case x; when X; 2; when X; end), "duplicate when X in case"

it_parses "%w{one two}", (["one".string, "two".string] of ASTNode).array_of(Path.global("String"))
it_parses "%w{one\ntwo}", (["one".string, "two".string] of ASTNode).array_of(Path.global("String"))
it_parses "%w{one\ttwo}", (["one".string, "two".string] of ASTNode).array_of(Path.global("String"))
it_parses "%w{\n}", ([] of ASTNode).array_of(Path.global("String"))
it_parses "%w{one\\ two}", (["one two".string] of ASTNode).array_of(Path.global("String"))
it_parses "%w{one{} two}", (["one{}".string, "two".string] of ASTNode).array_of(Path.global("String"))
it_parses "%w{\\{one}", (["{one".string] of ASTNode).array_of(Path.global("String"))
it_parses "%w{one\\}}", (["one}".string] of ASTNode).array_of(Path.global("String"))
it_parses "%i(one\\ two)", (["one two".symbol] of ASTNode).array_of(Path.global("Symbol"))
it_parses "%i{(one two)}", (["(one".symbol, "two)".symbol] of ASTNode).array_of(Path.global("Symbol"))
it_parses "%i((one two))", (["(one".symbol, "two)".symbol] of ASTNode).array_of(Path.global("Symbol"))
it_parses "%i(foo(bar) baz)", (["foo(bar)".symbol, "baz".symbol] of ASTNode).array_of(Path.global("Symbol"))
it_parses "%i{foo\\nbar baz}", (["foo\\nbar".symbol, "baz".symbol] of ASTNode).array_of(Path.global("Symbol"))

assert_syntax_error "%w(", "Unterminated string array literal"
assert_syntax_error "%w{one}}", "expecting token 'EOF', not '}'"
assert_syntax_error "%w{{one}", "Unterminated string array literal"
assert_syntax_error "%i(", "Unterminated symbol array literal"
assert_syntax_error "%i{one}}", "expecting token 'EOF', not '}'"
assert_syntax_error "%i{{one}", "Unterminated symbol array literal"
assert_syntax_error "%x(", "Unterminated command literal"
assert_syntax_error "%r(", "Unterminated regular expression"
assert_syntax_error "%q(", "Unterminated string literal"
Expand Down
39 changes: 37 additions & 2 deletions src/compiler/crystal/syntax/lexer.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2477,7 +2477,40 @@ module Crystal
end

start = current_pos
while !current_char.ascii_whitespace? && current_char != '\0' && current_char != @token.delimiter_state.end
sub_start = start
value = String::Builder.new

escaped = false
while true
Copy link
Copy Markdown
Contributor

@Sija Sija Feb 1, 2018

Choose a reason for hiding this comment

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

loop do?

Copy link
Copy Markdown
Member Author

@straight-shoota straight-shoota Feb 1, 2018

Choose a reason for hiding this comment

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

Everywhere else in this file it's while true... I'd rather stick with that until (maybe) all occurances would be converted (not sure if).

Copy link
Copy Markdown
Member

@asterite asterite Feb 1, 2018

Choose a reason for hiding this comment

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

loop do and while true are the same. In fact, loop do might be a bit slower to compile, and it has a counter.

Please, let's not comment such things, nowhere it says that loop do is better than while true nor that it's the std/compile style that we prefer.

Copy link
Copy Markdown
Contributor

@hugoabonizio hugoabonizio Feb 1, 2018

Choose a reason for hiding this comment

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

@asterite Since loop do is borrowed from Ruby, we could borrow the style guide rule for this too.

Copy link
Copy Markdown
Member

@asterite asterite Feb 1, 2018

Choose a reason for hiding this comment

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

But they are not the same. That advice is wrong. If you declare a variable inside the loop, it won't exist outside it, because of blocks. So I always prefer while true. It's also so much lightweight: no block involves, just pure procedural code.

Copy link
Copy Markdown
Member

@asterite asterite Feb 1, 2018

Choose a reason for hiding this comment

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

BOTH are accepted. End of discussion.

Copy link
Copy Markdown
Member Author

@straight-shoota straight-shoota Feb 1, 2018

Choose a reason for hiding this comment

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

Having a style guide even if it says "both are accepted" would help avoiding such discussions ;)

Copy link
Copy Markdown
Member

@asterite asterite Feb 1, 2018

Choose a reason for hiding this comment

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

Having no style guide and having everyone stop commenting on such small details would also help.

I mean, we have a formatter already. If it's formatted, we accept it. If there's no performance loss between two constructs, we accept both of them. That's the style guide.

Otherwise, as I said before, let's restrict the language to just the constructs we like. Let's end with a language like Go.

Copy link
Copy Markdown

@ghost ghost Mar 30, 2018

Choose a reason for hiding this comment

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

Just a comment - the "style guide" in ruby is not official. I don't agree with most of it either. I have no comment on crystal itself, that is up to the crystal devs, but hugoabonizio referred to the style code in ruby, used and created by rubocop + the rubocop devs, and I have to add that this is not an "official" style guide. It's just the default one that the rubocop main dev added and you can change it by customizing rubocop (so I don't have a big problem with it either).

Copy link
Copy Markdown
Contributor

@faustinoaq faustinoaq Mar 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hey, there is ameba, is a nice tool, have been used successfully on amber framework amberframework/amber#465 and will be probably integrated in scry crystal-lang-tools/scry#58 too 😉

ameba

Thank you @veelenga ! 🎉

case current_char
when Char::ZERO
break # raise is handled by parser
when @token.delimiter_state.end
unless escaped
if @token.delimiter_state.open_count == 0
break
else
@token.delimiter_state = @token.delimiter_state.with_open_count_delta(-1)
end
end
when @token.delimiter_state.nest
unless escaped
@token.delimiter_state = @token.delimiter_state.with_open_count_delta(+1)
end
when .ascii_whitespace?
break unless escaped
else
if escaped
value << '\\'
end
end

escaped = current_char == '\\'
if escaped
value.write @reader.string.to_slice[sub_start, current_pos - sub_start]
sub_start = current_pos + 1
end

next_char
end

Expand All @@ -2486,8 +2519,10 @@ module Crystal
return @token
end

value.write @reader.string.to_slice[sub_start, current_pos - sub_start]

@token.type = :STRING
@token.value = string_range(start)
@token.value = value.to_s
set_token_raw_from_start(start)

@token
Expand Down