Shopify / packwerk

Good things come in small packages.
MIT License
1.59k stars 110 forks source link

Parser syntax error when using shorthand hash syntax #401

Closed mbaird closed 3 months ago

mbaird commented 5 months ago

What are you trying to accomplish?

Advice on how to approach/fix the failing test reproduction for https://github.com/Shopify/packwerk/issues/278 as suggested.

This isn't a fix, so happy to move this to the issue discussion instead.

What approach did you choose and why?

I'm not sure on the best approach to fix this, so looking for guidance.

This change fails the valid ERB parser test with the error:

Error:
Packwerk::Parsers::ErbTest#test__call_returns_node_with_valid_file:
Packwerk::Parsers::ParseError: Syntax error: unexpected string beginning, expecting end-of-input
    lib/packwerk/parsers/ruby.rb:50:in `rescue in call'
    lib/packwerk/parsers/ruby.rb:40:in `call'
    /Users/michael/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/sorbet-runtime-0.5.11346/lib/types/private/methods/call_validation.rb:167:in `bind_call'
    /Users/michael/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/sorbet-runtime-0.5.11346/lib/types/private/methods/call_validation.rb:167:in `validate_call_skip_block_type'
    /Users/michael/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/sorbet-runtime-0.5.11346/lib/types/private/methods/call_validation.rb:109:in `block in create_validator_slow_skip_block_type'
    lib/packwerk/parsers/erb.rb:57:in `to_ruby_ast'
...

For the code added to valid.erb in this PR, the ruby code extracted from the erb ast in to_ruby_ast is:

 render 'thing', thing:
 render 'thing'

Which understandably fails parsing when attempted via prism.

This code works fine in practice, because ERB parsing results in the following:

"#coding:UTF-8\n_erbout = +''; 
erbout.<<(( render 'thing', thing: ).to_s); 
_erbout.<< \"\\n\".freeze\n; 
_erbout.<<(( render 'thing' ).to_s); 
_erbout"

It seems this issue might be with the ERB parser better-html.

Thanks for looking.

What should reviewers focus on?

Type of Change

Additional Release Notes

Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.

If no additional notes are necessary, delete this section or leave it unchanged.

Checklist

mbaird commented 5 months ago

I have signed the CLA!