Parser: correctly lex the prefix of keyword in macro #4659
Merged
+22
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
makenowjust
force-pushed
the
fix/crystal/lex-part-of-keyword-in-macro
branch
from
811addd to
d126386
Compare
|
You're awesome @makenowjust 🎉 |
Closed
|
ping anyone. Many people need this fix. |
makenowjust
changed the title
makenowjust
changed the title
RX14
approved these changes
Aug 6, 2017
|
This just needs another review, seems like a simple fix to me. |
|
Sorry 😄 |
bmulvihill
reviewed
Aug 14, 2017
src/compiler/crystal/syntax/lexer.cr
Outdated
| @@ -2139,6 +2139,12 @@ module Crystal | |||
| @macro_curly_count -= 1 | |||
| end | |||
| else | |||
| # In the following codes `check_macro_opening_keyword(beginning_of_line)` and some `next_char` may let `@rader` forward. | |||
There was a problem hiding this comment.
Small typo here: @reader
|
I consider another way. Add such a method: def lookahead
old_pos = @reader.pos
old_line_number, old_column_number = @line_number, @column_number
yield.tap do |result|
unless result
@reader.pos = old_pos
@line_number, @column_number = old_line_number, old_column_number
end
end
endAnd, use it to check keyword, like: if !delimiter_state && whitespace && lookahead { char == 'y' && next_char == 'i' && next_char == 'e' && next_char == 'l' && next_char == 'd' && !ident_part_or_end?(peek_next_char) }Perhaps it makes same result (I don't test it.) However, it is a bit heavier process than this PR way. |
|
@makenowjust I think I like that idea more. It seems like a cleaner and more reusable way to do it. |
|
Can someone please review this 🙏 |
|
Are we not waiting for @makenowjust to etst the suggested improvement? |
makenowjust
force-pushed
the
fix/crystal/lex-part-of-keyword-in-macro
branch
from
d126386 to
9ebb740
Compare
|
@RX14 Fixed! What do you think? |
asterite
approved these changes
Sep 14, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed #4656
I'll explain why this fix is related to #4656. Following code is minified #4656 example:
{% begin %} {{ :foo }}s {% end %} foo({ foo: 1, foo_bar: 42, })This example produces formatting error because of
IndexErrorwhich is raised fromalign_infos. In other words, #4656's reason is the formatter try to align incorrect line (If this incorrect line has enough columns to align, it produces incorrect result like #4656 gif, otherwise if it doesn't have, it raisesIndexError).Then, why the formatter try to align incorrect line, is the lexer is buggy. The lexer will report incorrect line number when a newline follows the part of some keywords inside macro (for example
sis the part ofstruct). This bug is explained in detail on the source code comments, see them. And so, the formatter mistakes line number by this bug.@sdogruyol expected me good thing, this helped me really. Thank you 😊