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

Handle properly stringifying multiline macro expressions #15305

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Blacksmoke16
Copy link
Member

@Blacksmoke16 Blacksmoke16 commented Dec 21, 2024

Given the following code:

{%
  a = 1
%}

Its to_s representation before this PR:

{% a = 1 %}

and after:

{%
  a = 1
%}

This PR also addresses #15307:

macro finished
  {% verbatim do %}
    {%
      10

      # Foo

      20
    %}
  {% end %}
end

is now properly stringified as:

macro finished
  {% verbatim do %}
    {%
      10



      20
    %}
  {% end %}
end

instead of:

macro finished
  {% verbatim do %}
    {%
      10
      20
    %}
  {% end %}
end

@straight-shoota
Copy link
Member

straight-shoota commented Dec 21, 2024

Hm, but if the expression only has a newline at one end it'll be different, right?

{%
  a = 1 %}
{% b = 1
%}

This presumably stringifies to:

{%
  a = 1
%}
{% b = 1 %}

Also other whitespace isn't repeated truthfully either.
So sure this improves some situations. And maybe that means the most common ones are covered correctly. But there are still lots of holes and I'm wondering if it's worth doing this change when it's clearly insufficient to the full extend of the problem space.

@Blacksmoke16
Copy link
Member Author

Hm, but if the expression only has a newline at one end it'll be different, right?

If you have code like:

{%
  a = 1 %}
{% b = 1
%}

and you run the formatter on it, you get:

{%
  a = 1
%}
{% b = 1 %}

So this at least makes things consistent with that.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Dec 22, 2024

Pushed up another commit to better handle indentation within the stringified nodes. E.g.

macro finished
  {% verbatim do %}
    {%
      10
      20
    %}
  {% end %}
end

Its string representation is now the same, instead of:

macro finished
  {% verbatim do %}
    {%
  10
  20
%}
  {% end %}
end

@Blacksmoke16
Copy link
Member Author

Ended up refactoring things one last time. Realized I could do this without touching the AST representation at all and just handle it based on the start/end location of the nodes themselves. So this PR only touches ToSVisitor now.

@straight-shoota
Copy link
Member

I wonder if it could be feasible to keep track of line numbers in the ToSVisitor and add newlines accordingly...? 🤔
This should be possible by memorizing the reference position in @str and counting the \n characters in between.

The implementation would consider each node's location relative to each other. If node.location is on line x and node.exp.location is on line x + n, there need to be n newlines in between. We need to add as many as necessary.
Similarly, after stringifying node.exp, count the number of newlines and add more to match the difference of line numbers between node.location and node.end_location.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Dec 23, 2024

@straight-shoota I think it should definitely be possible, but would you be opposed to leaving that for another PR to address #15307? This PR is already a good start and is the basis of some other PRs I have in the works.

EDIT: Actually one problem might be that @str in some context doesn't support #pos=. But as long as we don't need to do a #pos= or anything we should be okay.

@straight-shoota
Copy link
Member

I'm not super convinced about the implementation in this PR because it just addresses some symptoms, not the problem from the bottom up. It's basically just shifting error cases around, not really solving anything.
It has some benefit because it moves towards better behaviour for well-formatted code which is supposedly more common.
I'd really much more like to resolve the full problem instead of a cosmetic change. IMO this isn't super urgent so we could take some time to explore more effective alternatives.

@Blacksmoke16
Copy link
Member Author

👍 Alright, I'll see about doing another refactor to address #15307 in this PR in that case. I'll mark it as draft for now in that case.

@Blacksmoke16 Blacksmoke16 marked this pull request as draft December 23, 2024 14:23
@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Dec 23, 2024

Alright @straight-shoota, pushed up another commit that handles significant whitespace as well. Tested against to_s_spec.cr as well as within my other macro code coverage branch's test suite and everything seems to pass so 👍?

EDIT: Nevermind, spoke too soon. Found some additional edge cases I'll need to address.
EDIT2: Okay it was a quick fix 😅.

@Blacksmoke16 Blacksmoke16 marked this pull request as ready for review December 23, 2024 17:49
@Blacksmoke16 Blacksmoke16 marked this pull request as draft December 23, 2024 17:54
@Blacksmoke16 Blacksmoke16 marked this pull request as ready for review December 23, 2024 17:59
Update other specs to use new format
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.

Retaining significant whitespace within MacroExpressions within MacroVerbatim
2 participants