-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Parser: prevent to finish a macro body when {% if/for/begin %} is nested #4801
Conversation
You're awesome! Thanks =) |
Sorry, I forgot to fix the formatter. |
03a774e
to
287085f
Compare
Done fixing the formatter. |
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.
287085f
to
446f4d6
Compare
Can we get this approved? |
I think I'd prefer a What is |
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. |
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. |
Thanks the detailed explanation @makenowjust. That makes sense. |
…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.
Fixed #4769
This introduced
control_nest
counter variable to keep macro control (if/for/begin) nesting information. When the parser findsend
inside macro body, it checkscontrol_nest
and finishes a macro body if and only ifcontrol_nest
is0
(andnest
which is usual block nestinginformation is
0
also).