mbj / unparser

Turn Ruby AST into semantically equivalent Ruby source
MIT License
310 stars 52 forks source link

Fixes #350 by adding missing HEREDOC handling #351

Closed vidarh closed 1 year ago

vidarh commented 1 year ago

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

vidarh commented 1 year ago

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

mbj commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

I can reproduce it via: https://github.com/mbj/unparser/issues/350#issue-1941098356, which is odd.

mbj commented 1 year ago
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 commented 1 year ago

@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: https://github.com/mbj/unparser/pull/353

mbj commented 1 year ago

here was the coverage hole detected by mutant https://github.com/mbj/unparser/actions/runs/6700137747/job/18205557341#step:4:623