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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parser: prevent to finish a macro body when {% if/for/begin %} is nested #4801

Merged

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Aug 6, 2017

Fixed #4769

This introduced control_nest counter variable to keep macro control (if/for/begin) nesting information. When the parser finds end inside macro body, it checks control_nest and finishes a macro body if and only if control_nest is 0 (and nest which is usual block nesting
information is 0 also).

@makenowjust makenowjust mentioned this pull request Aug 6, 2017
@straight-shoota
Copy link
Member

You're awesome! Thanks =)
The original code of #4772 with end formats and compiles with this fix.
However the workaround using {{ "end".id }} still produces an error in the formatter (but compiles fine).

@makenowjust
Copy link
Contributor Author

makenowjust commented Aug 6, 2017

Sorry, I forgot to fix the formatter.

@makenowjust makenowjust force-pushed the fix/not-finish-macro-control-nested branch from 03a774e to 287085f Compare August 6, 2017 17:02
@makenowjust
Copy link
Contributor Author

Done fixing the formatter.

@straight-shoota
Copy link
Member

Confirmed, now it works as expected. Thanks again!

Fixed crystal-lang#4769

This introduced `control_nest` counter variable to keep macro control
(if/for/begin) nesting information. When the parser find `end` inside
macro body, it checks `control_nest` and finishes a macro body if and
only if `control_nest` is `0` (and `nest` which is usual block nesting
information is `0` also).

And formatter is fixed also.
@makenowjust makenowjust force-pushed the fix/not-finish-macro-control-nested branch from 287085f to 446f4d6 Compare August 7, 2017 00:12
@straight-shoota
Copy link
Member

Can we get this approved?

@ysbaddaden
Copy link
Contributor

I think I'd prefer a parse_macro_control_body method, instead of repeating incr, parse_body, decr.

What is nest for? Any nested macro content?

@makenowjust
Copy link
Contributor Author

makenowjust commented Aug 9, 2017

nest is for counting if, while and other usual (not macro) control block nesting to detect macro block's end. But it behaves like stack against macro control internally, in other codes:

macro foo
  # `nest == 0`
  {% if true %}
    # `nest == 0` also because `{% if true %}` is not counted by `nest`
    if true
      # `nest == 1` because `if true` is counted
  {% end %}

  # Now, `nest == 0`. oops! after closing `{% if true %} ... {% end %}`, `nest` is reset before it.
  
  {% if true %}
    # `nest == 0` and the parser found `end`, so the parser mistakes it as macro body `end`, and then #4769 bug is caused.
    end
  {% end %}
end

Macro body parsing is very very and very complex task (hmm, @asterite?), so unfortunately we cannot count macro/usual control block nesting in simple way.

Of course, to correct this behavior is the right way to fix #4769, but I think this PR way is also right and this is simpler than that. The most preferable way is to apply both, but I think this makes it hard to review.

@straight-shoota
Copy link
Member

Yes, this is a fix for incorrect behaviour and does not add much complexity. It makes sense to merge this as a bugfix for now and address refactoring in a different PR.

@ysbaddaden
Copy link
Contributor

Thanks the detailed explanation @makenowjust. That makes sense.

@RX14 RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler topic:stdlib:macros labels Aug 10, 2017
@RX14 RX14 merged commit de80a09 into crystal-lang:master Aug 10, 2017
@RX14 RX14 added this to the Next milestone Aug 10, 2017
@makenowjust makenowjust deleted the fix/not-finish-macro-control-nested branch August 11, 2017 00:35
Val pushed a commit to Val/crystal that referenced this pull request Aug 12, 2017
…ted (crystal-lang#4801)

Fixed crystal-lang#4769

This introduced `control_nest` counter variable to keep macro control
(if/for/begin) nesting information. When the parser find `end` inside
macro body, it checks `control_nest` and finishes a macro body if and
only if `control_nest` is `0` (and `nest` which is usual block nesting
information is `0` also).

And formatter is fixed also.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler topic:stdlib:macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants