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

Formatter: try to format macros that don't interpolate content #12378

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

asterite
Copy link
Member

Fixes #12164

src/compiler/crystal/tools/formatter.cr Outdated Show resolved Hide resolved
src/compiler/crystal/tools/formatter.cr Outdated Show resolved Hide resolved
Comment on lines +1737 to +1738
# Only format the macro contents if it's valid Crystal code
Parser.new(source).parse
Copy link
Contributor

@HertzDevil HertzDevil Aug 13, 2022

Choose a reason for hiding this comment

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

Here any \{{ or \{% MacroLiterals from source will be unescaped. Is there a situation where a macro's body is valid Crystal code if it has the escaped forms, but becomes invalid when they are unescaped? (This should only affect a very small number of macros though.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that this might be already handled. I tried formatting this:

macro foo
  puts(\{{@type}}         )
end

And it actually produced this:

macro foo
  puts(\{{@type}})
end

There's a write_macro_slashes method. I didn't take a look at how it works (I might have written that but now I can't remember.)

I'd say, let's first find a case where it fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

On further thought, this is probably fine because it just means the macro contents are invalid after any interpolation. This is what I got:

{\{{ 1 }}}   # a TupleLiteral wrapping an escaped MacroLiteral
# becomes:
{{{ 1 }}}    # a MacroExpression wrapping a TupleLiteral

{ \{{ 1 }} } # a TupleLiteral wrapping an escaped MacroLiteral
# becomes:
{ {{ 1 }} }  # a TupleLiteral wrapping a MacroExpression

{\{{ 1 }} }  # a TupleLiteral wrapping an escaped MacroLiteral
# becomes:
{{{ 1 }} }   # invalid code

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I'm not understanding. Is there something to fix? Based on the comment above, it looks so but I'm not sure 😅

Even if the macro contents are invalid, I don't think we should format those to something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can leave this check as is.

@straight-shoota straight-shoota added this to the 1.6.0 milestone Sep 6, 2022
@asterite asterite merged commit 17361d3 into master Sep 8, 2022
@asterite asterite deleted the formatter/simple-macros branch September 8, 2022 11:05
@zw963
Copy link
Contributor

zw963 commented Sep 8, 2022

Thanks for fix this issue, it works!

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.

crystal tool format no-op if indentation error on macro
4 participants