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

Crash when formatting case and macro for #8633

Open
wooster0 opened this issue Dec 28, 2019 · 5 comments
Open

Crash when formatting case and macro for #8633

wooster0 opened this issue Dec 28, 2019 · 5 comments

Comments

@wooster0
Copy link
Contributor

wooster0 commented Dec 28, 2019

case
{% for index in 0..0 %}
{% end %}
end
$ crystal tool format --show-backtrace
expecting {%, not `NEWLINE, `, at :1:5 (Exception)
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???

couldn't format './cr.cr', please report a bug including the contents of it: https://github.com/crystal-lang/crystal/issues
@jan-zajic
Copy link
Contributor

It is not a valid macro, you can't do that. As documented here: Macros

This means, for example, a macro cannot generate a one or more when expressions of a case statement unless case was a part of the generated code.

You can do this:

{% begin %}
case
{% for index in 0..0 %}
{% end %}
end
{% end %}

@straight-shoota
Copy link
Member

Technically, the code is valid because the expanded macro is empty. But of course, it doesn't make sense because you can't do anything useful with it.

Still, since it's valid syntax, the formatter should be able to handle it.

We could also consider making an outer macro expansion directly following a case or when expression a syntax error.

@asterite
Copy link
Member

Also this fails to format:

case
1
end

The above is equivalent to:

case 1
end

In the original snippet it's the same.

I wonder if we should disallow the condition case condition to be in a separate line... it looks pretty weird. Same goes for if, while, etc. 🤔

@straight-shoota
Copy link
Member

@asterite I would have never assumed that a newline would be appropriate at the location. So 👍

Especially for case, which can go without an expression, it looks confusing.

@straight-shoota
Copy link
Member

So only use case I could think of is when the expression is very long and you want to split it into serveral lines, it makes sense to start all lines at the same column. But the formatter already takes care of that, at least for if and unless:

if a_long_conditional_expression &&
   another_even_longer_expression &&
   this_is_the_longest_expression_of_them_all
  body
end
unless a_long_conditional_expression &&
       another_even_longer_expression &&
       this_is_the_longest_expression_of_them_all
  body
end

But not with case:

case a_long_conditional_expression &&
  another_even_longer_expression &&
  this_is_the_longest_expression_of_them_all
when foo
  body
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants