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

Fix block parameter unpacking inside macros #13813

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Sep 19, 2023

Fixes #13742 (regression on master).

This effectively reverts #13709; it seems the compiler doesn't need normalizations on all those kinds of macro nodes every time.

@HertzDevil HertzDevil added kind:bug topic:lang:macros kind:regression Something that used to correctly work but no longer works labels Sep 19, 2023
@straight-shoota
Copy link
Member

FTR: The regression was introduced in #11597 (not yet released)

@straight-shoota straight-shoota added this to the 1.10.0 milestone Sep 19, 2023
@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Sep 20, 2023

I applied this patch on latest commit on master and tried the Athena specs again and now got:

In src/components/framework/src/ext/routing/annotation_route_loader.cr:231:128

 231 | [{ATHA::QueryParam, "ATH::Params::QueryParam".id}, {ATHA::RequestParam, "ATH::Params::RequestParam".id}].each do |(param_ann, param_class)|
                                                                                                                          ^---------
Error: undefined macro variable '__temp_113'

So there still seems to be another regression. I'll see about reducing it and will create another ticket.

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Sep 20, 2023

Actually no, reproduction for this is the same as #13742, but just call test macro twice. Because of that I don't think it needs its own ticket?

macro test
  {%
    data = [{1, 2}, {3, 4}]

    data.each do |(k, v)|
      pp k, v
    end
  %}
end

test
test
$ ccrystal test.cr --error-trace
Using compiled compiler at /Users/georgedietrich/bsmoke/crystal/.build/crystal
1
2
3
4
Showing last frame. Use --error-trace for full trace.

In test.cr:5:20

 5 | data.each do |(k, v)|
                    ^--------
Error: undefined macro variable '__temp_49'

@straight-shoota straight-shoota removed this from the 1.10.0 milestone Sep 20, 2023
@HertzDevil
Copy link
Contributor Author

Apparently the normalizer mutates Block nodes in-place if they contain unpacked parameters, without emptying Block#unpacks, so calling the same macro twice will also create the multi-assign expression twice, but overwrite the previous temporary packed parameter:

do |__temp_4|
  k, v = __temp_4
  k, v = __temp_1
  k
end

@straight-shoota straight-shoota added this to the 1.10.0 milestone Sep 20, 2023
@Blacksmoke16
Copy link
Member

Awesome! Testing again with that latest commit and all Athena specs pass. Thanks!

@straight-shoota
Copy link
Member

Specs are failing though.

@straight-shoota straight-shoota removed this from the 1.10.0 milestone Sep 20, 2023
@HertzDevil
Copy link
Contributor Author

So Block#unpacks is needed by the main visitor it seems. I have replaced that with a different check

@Blacksmoke16 Blacksmoke16 added this to the 1.10.0 milestone Sep 21, 2023
@straight-shoota straight-shoota merged commit d2929d0 into crystal-lang:master Sep 24, 2023
53 checks passed
@HertzDevil HertzDevil deleted the bug/macro-block-param-unpack branch September 25, 2023 12:29
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:lang:macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in macro block unpacking within macro
3 participants