mbj / unparser

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

Symbol literals with interpolation are not allowed in hash patterns #340

Open kddnewton opened 1 year ago

kddnewton commented 1 year ago

Found this while importing your tests into YARP. On this line:

https://github.com/mbj/unparser/blob/8260882beae39cd3d88a29625d07dd31bbc66ece/test/corpus/literal/pattern.rb#L20

It's actually a syntax error.

case foo
in {"#{"a"}": 1}
end

Running ruby -c gives:

test.rb: test.rb:2: symbol literal with interpolation is not allowed (SyntaxError)
in {"#{"a"}": 1}
    ^~~~~~~~~
mbj commented 1 year ago

Interesting, whitequark/parser seems to accept it right now, producing this AST, its accepted in 2.7, 3.0, 3.1 and 3.2 here.

(case-match
  (send nil :foo)
  (in-pattern
    (hash-pattern
      (pair
        (dsym
          (begin
            (str "a")))
        (int 1))) nil nil) nil)

On MRI: Ruby 2.7 accepts it. But 3.0, 3.1 and 3.2 not.

Unparsers goal was to be able to unparser whitequark/parser so it does the job "right". This pattern tests are extracted from whitequark parser corpus. So you should have found it also via your test extraction.

I think we have to file this issue in whitequark/parser. Once they remove support unparser will also.

mbj commented 1 year ago

For full visibility, I've reported this in: https://github.com/whitequark/parser/issues/919. I do not want to filter it out unparser side for the moment. I'll do so with a fixing parser release.

mbj commented 1 year ago

@kddnewton if you are importing unparsers tests, you can just ignore this pattern, or would it be better for you I pushed a removal?

kddnewton commented 1 year ago

That's okay! I'm just ignored that one line for now. I appreciate the explanation!

mbj commented 1 year ago

Well I'd be happy to use YARP in the future as source AST for unparser. So thanks to you.

I hope you made dstrings less ambigious in your native AST, I'll check it out soon. Dynamic strings are the only construct that unparser fails on in edge cases right now.

kddnewton commented 1 year ago

YARP has two kinds of strings: StringNode and InterpolatedStringNode. InterpolatedStringNode has a list of child nodes which are interpolated expressions, string nodes, or interpolated variables. Does that answer your question? I'm not entirely sure.

mbj commented 1 year ago

@kddnewton yes helps me already, the story goes a bit deeper. I'll reply here later explaining it.