Skip to content
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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parser: correctly lex the prefix of keyword in macro #4659

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Jul 2, 2017

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 IndexError which is raised from align_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 raises IndexError).

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 s is the part of struct). 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 馃槉

@makenowjust makenowjust force-pushed the fix/crystal/lex-part-of-keyword-in-macro branch from 811addd to d126386 Compare Jul 2, 2017
@sdogruyol
Copy link
Member

@sdogruyol sdogruyol commented Jul 2, 2017

You're awesome @makenowjust 馃帀

@makenowjust makenowjust mentioned this pull request Aug 6, 2017
@makenowjust
Copy link
Contributor Author

@makenowjust makenowjust commented Aug 6, 2017

ping anyone. Many people need this fix.

@makenowjust makenowjust changed the title Parser: correct to lex the part of keyword in macro Parser: correctly lex the part of keyword in macro Aug 6, 2017
@makenowjust makenowjust changed the title Parser: correctly lex the part of keyword in macro Parser: correctly lex the prefix of keyword in macro Aug 6, 2017
RX14
RX14 approved these changes Aug 6, 2017
@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 14, 2017

This just needs another review, seems like a simple fix to me.

@makenowjust makenowjust reopened this Aug 14, 2017
@makenowjust
Copy link
Contributor Author

@makenowjust makenowjust commented Aug 14, 2017

Sorry 馃槃

@@ -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.
Copy link
Contributor

@bmulvihill bmulvihill Aug 14, 2017

Choose a reason for hiding this comment

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

Small typo here: @reader

@makenowjust
Copy link
Contributor Author

@makenowjust makenowjust commented Aug 15, 2017

@RX14

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
end

And, 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.

@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 15, 2017

@makenowjust I think I like that idea more. It seems like a cleaner and more reusable way to do it.

@sdogruyol
Copy link
Member

@sdogruyol sdogruyol commented Sep 10, 2017

Can someone please review this 馃檹

@RX14
Copy link
Contributor

@RX14 RX14 commented Sep 10, 2017

Are we not waiting for @makenowjust to etst the suggested improvement?

@makenowjust makenowjust force-pushed the fix/crystal/lex-part-of-keyword-in-macro branch from d126386 to 9ebb740 Compare Sep 11, 2017
@makenowjust
Copy link
Contributor Author

@makenowjust makenowjust commented Sep 11, 2017

@RX14 Fixed! What do you think?

RX14
RX14 approved these changes Sep 11, 2017
Copy link
Contributor

@RX14 RX14 left a comment

馃憣

@asterite asterite merged commit d92d3e3 into crystal-lang:master Sep 14, 2017
2 checks passed
@makenowjust makenowjust deleted the fix/crystal/lex-part-of-keyword-in-macro branch Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants