Skip to content

Parse MacroVar with empty expressions: %var{}#16772

Open
straight-shoota wants to merge 3 commits intocrystal-lang:masterfrom
straight-shoota:fix/parse-macro_vars-empty-exps
Open

Parse MacroVar with empty expressions: %var{}#16772
straight-shoota wants to merge 3 commits intocrystal-lang:masterfrom
straight-shoota:fix/parse-macro_vars-empty-exps

Conversation

@straight-shoota
Copy link
Copy Markdown
Member

Fixes the parser choking on %var{} in a macro context.

It might be debatable whether a macro var with empty expressions should be valid syntax. It doesn't make much sense semantically. But I don't see a good reason why it shouldn't.

Similar to #16741, this seems like an omission. Any number of expressions should be acceptable, including zero.

@oprypin
Copy link
Copy Markdown
Member

oprypin commented Mar 22, 2026

This change effectively introduces new valid syntax so maybe it's worth a slightly deeper discussion.

Maybe it's best to turn it into an explicit error instead? I wouldn't call it a necessary generalization because %var is already the generalization, and this is duplicate syntax for it. I think it can only bring confusion to the reader, and doesn't satisfy any new use case.

I think there were some issues solved long ago where splatting of 0 arguments inside macros was made to work, and it was necessary because the input could be any number of args. But this is not like that, there's no way to use this syntax in a programmatic way

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Mar 23, 2026

Even if there is no programmatic way to produce this syntax, it could still appear when rewriting code.

The syntax is pretty unambiguous. What else could %var{} possibly be interpreted as?
So I'd rather allow the syntax than make it an error.
And we should rewrite to drop the braces in the formatter. Forgot to add that bit in this PR.
We can't change this in the formatter easily because it might produce invalid code: macro foo;%var{}end becomes macro foo;%varend. So the current state of this PR should be fine.

@ysbaddaden
Copy link
Copy Markdown
Collaborator

I'm fine with either a proper parser error or accepting the syntax.

@ysbaddaden ysbaddaden added this to the 1.20.0 milestone Mar 23, 2026
straight-shoota added a commit that referenced this pull request Mar 25, 2026
This change adds specs to document the current behaviour, which seems very incorrect: We shouldn't be parsing `MacroVar` inside a literal.
It doesn't affect the result when the macro var syntax is valid, but it easily breaks when it's not (see #16772 for example)
@ysbaddaden
Copy link
Copy Markdown
Collaborator

@straight-shoota the assertion from #16778 is now failing.

@straight-shoota straight-shoota removed this from the 1.20.0 milestone Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants