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

Fixes #350 by adding missing HEREDOC handling #351

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

vidarh
Copy link
Contributor

@vidarh vidarh commented Oct 13, 2023

Unparser::Emitter::OpAssign was lacking an emit_heredoc_reminders method.

@vidarh vidarh changed the title Fixes #360 by adding missing HEREDOC handling Fixes #350 by adding missing HEREDOC handling Oct 13, 2023
@vidarh
Copy link
Contributor Author

vidarh commented Oct 13, 2023

Sorry, got the issue number wrong. Fixed the title.

@mbj
Copy link
Owner

mbj commented Oct 13, 2023

@vidarh Your fix appears valid. And I'm going to merge it.

But I'd still love if unparser would only generate heredocs if the AST can only be produced by a heredoc. There is a duality where:

A) Some dstr nodes can be produced by complex string literals, but still string literals.
B) Some dstr nodes can only be produced by heredocs when parsed.

I've yet to find a rule about the signagure in the dstr nodes to differentiate A) and B) to only emit heredoc if its absolutely required.

Also I think heredoc vs not heredoc should be a different AST node, and I hope prism (former YARP), and parser may change this to be discoverable AST level without looking at source locations.

I'll do another OSS cycle this weekend to look deeper into your PR. Thanks so far!

@mbj
Copy link
Owner

mbj commented Oct 30, 2023

@vidarh I'm coming back to this now, your test is not failing without the code fix:

$ bundle exec unparser --verbose -e 'y += "{42}\n"'
(string)
Original-Source:
y += "{42}\n"
Generated-Source:
y += "{42}\n"
Original-Node:
(op-asgn
  (lvasgn :y) :+
  (str "{42}\n"))
Generated-Node:
(op-asgn
  (lvasgn :y) :+
  (str "{42}\n"))
Success: (string)

Can you tell me a piece of source that shows the bug? (I'm blind, my repro did not interpolate).

@mbj
Copy link
Owner

mbj commented Oct 31, 2023

I can reproduce it via: #350 (comment), which is odd.

@mbj
Copy link
Owner

mbj commented Oct 31, 2023

undle exec unparser --verbose -e 'y += "#{42}\n"'
(string)
Original-Source:
y += "#{42}\n"
Generated-Source:
y += <<-HEREDOC
Original-Node:
(op-asgn
  (lvasgn :y) :+
  (dstr
    (begin
      (int 42))
    (str "\n")))
Generated-Node:
#<Parser::SyntaxError: unexpected token tLSHFT>
/home/mutant-dev/.gem/ruby/3.2.2/gems/parser-3.2.2.4/lib/parser/diagnostic/engine.rb:72:in `process'
/home/mutant-dev/.gem/ruby/3.2.2/gems/parser-3.2.2.4/lib/parser/base.rb:286:in `on_error'
/home/mutant-dev/.gem/ruby/3.2.2/gems/racc-1.7.1/lib/racc/parser.rb:265:in `_racc_do_parse_c'
/home/mutant-dev/.gem/ruby/3.2.2/gems/racc-1.7.1/lib/racc/parser.rb:265:in `do_parse'
/home/mutant-dev/.gem/ruby/3.2.2/gems/parser-3.2.2.4/lib/parser/base.rb:190:in `parse'
/home/mutant-dev/devel/unparser/lib/unparser.rb:116:in `block in parse_either'
/home/mutant-dev/devel/unparser/lib/unparser/either.rb:34:in `wrap_error'
/home/mutant-dev/devel/unparser/lib/unparser.rb:115:in `parse_either'
/home/mutant-dev/devel/unparser/lib/unparser/either.rb:115:in `bind'
/home/mutant-dev/devel/unparser/lib/unparser/validation.rb:65:in `from_string'
/home/mutant-dev/devel/unparser/lib/unparser/cli.rb:40:in `validation'
/home/mutant-dev/devel/unparser/lib/unparser/cli.rb:144:in `public_send'
/home/mutant-dev/devel/unparser/lib/unparser/cli.rb:144:in `process_target'
/home/mutant-dev/devel/unparser/lib/unparser/cli.rb:134:in `block in exit_status'
/home/mutant-dev/devel/unparser/lib/unparser/cli.rb:133:in `each'
/home/mutant-dev/devel/unparser/lib/unparser/cli.rb:133:in `exit_status'
/home/mutant-dev/devel/unparser/lib/unparser/cli.rb:64:in `run'
/home/mutant-dev/devel/unparser/bin/unparser:10:in `<top (required)>'
/home/mutant-dev/.gem/ruby/3.2.2/bin/unparser:25:in `load'
/home/mutant-dev/.gem/ruby/3.2.2/bin/unparser:25:in `<top (required)>'
Error: (string)

@mbj mbj merged commit 62902ea into mbj:main Oct 31, 2023
@mbj
Copy link
Owner

mbj commented Oct 31, 2023

@vidarh BTW I forgot to make sure test suite ran on CI and your change had a coverage hole, I fixed it in a followup: #353

@mbj
Copy link
Owner

mbj commented Oct 31, 2023

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

Successfully merging this pull request may close these issues.

2 participants