Minor fixes (Ruby Next compatibility) #7

palkan · 2023-12-05T14:08:29Z

Thanks for your on Prism and this translator!

I've started experimenting with it in Ruby Next and encountered several minor issues (fixed in this PR, also see comments).

There are still some problems left in my build (see https://github.com/ruby-next/ruby-next/actions/workflows/ruby-prism-test.yml):

Rationals with hex/binary values are not supported:

Parser::Prism.parse("0xffr")

/opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.18.0/lib/prism/node_ext.rb:55:in `Rational': invalid value for convert(): "0xff" (ArgumentError)

      Rational(slice.chomp("r"))
               ^^^^^^^^^^^^^^^^
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.18.0/lib/prism/node_ext.rb:55:in `value'
        from /Users/palkan/dev/parser-prism/lib/parser/prism/compiler.rb:1223:in `visit_rational_node'

Nil keyword pattern (**nil) is missing location information:

Parser::Prism.parse('case i
        in **nil
        else
          true
        end'
)


/opt/homebrew/lib/ruby/gems/3.2.0/gems/parser-3.2.2.3/lib/parser/source/range.rb:211:in `join': undefined method `begin_pos' for nil:NilClass (NoMethodError)

            [@begin_pos, other.begin_pos].min,
                              ^^^^^^^^^^
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/parser-3.2.2.3/lib/parser/builders/default.rb:2115:in `keyword_map'
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/parser-3.2.2.3/lib/parser/builders/default.rb:1484:in `in_pattern'
        from /Users/palkan/dev/parser-prism/lib/parser/prism/compiler.rb:760:in `visit_in_node'
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.18.0/lib/prism/node.rb:7876:in `accept'
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.18.0/lib/prism/compiler.rb:34:in `block in visit_all'
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.18.0/lib/prism/compiler.rb:34:in `map'
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.18.0/lib/prism/compiler.rb:34:in `visit_all'
        from /Users/palkan/dev/parser-prism/lib/parser/prism/compiler.rb:308:in `visit_case_match_node'

Here args.last.loc.expression is nil: https://github.com/ruby-next/parser/blob/0af323526e73b3e8319e856863666e70f57838e0/lib/parser/builders/default.rb#L2117

kddnewton · 2023-12-05T14:10:57Z

Hey @palkan thanks for checking it out! I'll review this today and get back to you

kddnewton · 2024-01-03T14:53:36Z

Oh gosh I completely lost track of this PR. I'm sorry! I'll review today!

kddnewton · 2024-01-03T17:16:57Z

Hey @palkan -

So I've pushed fixes for the rational thing (this turned out to be a bug in the prism Ruby API) and the **nil thing (this was a bug in this compiler). I've also pushed a fix for the Prism.parse options.

The other thing in this PR appears to be related to hash key/value pairs. Could you point me to a snippet that does that? I'm not sure I understand it.

palkan · 2024-01-10T01:02:09Z

Hey! Thanks for the updates. I checked it (see the latest build, I now run CI against your main branch), and here is what we have now:

The ArgumentError: invalid value for convert(): "0xff" is still there (Prism 0.19, Ruby 3.2).
This change fixes parsing the following code:

case {a: 1}
in {"a" => 1}
end

Currently fails with:

/Users/palkan/dev/parser-prism/lib/parser/prism/compiler.rb:88:in `visit_assoc_node': undefined method `parts' for @ StringNode (location: (2,16)-(2,19)) (NoMethodError)
├── flags: ∅
├── opening_loc: (2,16)-(2,17) = "\""
├── content_loc: (2,17)-(2,18) = "a"
├── closing_loc: (2,18)-(2,19) = "\""
└── unescaped: "a"
:Prism::StringNode

            visit_all(node.key.parts)
                              ^^^^^^
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.19.0/lib/prism/node.rb:864:in `accept'
        from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.19.0/lib/prism/compiler.rb:34:in `block in visit_all'

The following pattern matching code fails:

case [0, 1, 2, 3]
  in [0, 1,] # <---- trailing comma is the cause
    true
 end

with

/Users/palkan/dev/parser-prism/lib/parser/prism/compiler.rb:763:in `visit_implicit_rest_node': Cannot compile implicit rest nodes (Prism::ParserCompiler::CompilationError)
       from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.19.0/lib/prism/node.rb:8058:in `accept'
       from /opt/homebrew/lib/ruby/gems/3.2.0/gems/prism-0.19.0/lib/prism/compiler.rb:34:in `block in visit_all'

kddnewton · 2024-01-17T17:52:17Z

Great, thanks for the response.

Ahh shoot, this was fixed in prism main but not parser-prism main. I need to release a new version of prism, then this will be fixed.
Interesting. That's actually invalid code, but it looks like we're not raising syntax errors at all. Whoops! I'll make it so that we properly raise syntax errors in the way parser expects.
Oh no! I'll get that fixed.

palkan · 2024-01-17T23:49:15Z

Interesting. That's actually invalid code...

Yeah; I guess, that's what this change was for: https://github.com/kddnewton/parser-prism/pull/7/files#diff-4e7038ac7082a00aeeabcd6f11ffb2494661b7274c9d6bc3f3a257e91a821daaR90

kddnewton · 2024-01-29T16:05:19Z

Hey @palkan — over the weekend I merged this work into prism itself. You can see the doc here: https://github.com/ruby/prism/blob/main/docs/parser_translation.md. I've fixed all 3 of these issues over there. Could you give it a shot over there?

palkan · 2024-01-29T21:22:09Z

Hey @kddnewton! Cool!

I checked locally and see the following issue with find patterns:

# parsing the following code:
#
#   case x; in [*, 1, *]; true; else; false; end

/Users/palkan/dev/prism/lib/prism/translation/parser/compiler.rb:616:in `visit_find_pattern_node': undefined method `rest' for @ FindPatternNode (location: (1,11)-(1,20)) (NoMethodError)
├── constant: ∅
├── left:
│   @ SplatNode (location: (1,12)-(1,13))
│   ├── operator_loc: (1,12)-(1,13) = "*"
│   └── expression: ∅
├── requireds: (length: 1)
│   └── @ IntegerNode (location: (1,15)-(1,16))
│       └── flags: decimal
├── right:
│   @ SplatNode (location: (1,18)-(1,19))
│   ├── operator_loc: (1,18)-(1,19) = "*"
│   └── expression: ∅
├── opening_loc: (1,11)-(1,12) = "["
└── closing_loc: (1,19)-(1,20) = "]"
:Prism::FindPatternNode

          elements << node.rest if !node.rest.nil? && !node.rest.is_a?(ImplicitRestNode)
                                        ^^^^^
        from /Users/palkan/dev/prism/lib/prism/node.rb:6268:in `accept'
        from /Users/palkan/dev/prism/lib/prism/translation/parser/compiler.rb:821:in `block in visit_in_node'
        from /Users/palkan/dev/prism/lib/prism/translation/parser/compiler.rb:1806:in `within_pattern'
        from /Users/palkan/dev/prism/lib/prism/translation/parser/compiler.rb:821:in `visit_in_node'
        from /Users/palkan/dev/prism/lib/prism/node.rb:8311:in `accept'

With Parser, I get this AST:

(case-match
  (send nil :x)
  (in-pattern
    (find-pattern
      (match-rest)
      (int 1)
      (match-rest)) nil
    (true))
  (false))

Also, I found that parsing it differs (I guess, Prism follows Ruby 3.4 spec, so that's fine):

PRISM=true bin/parse '-> { it }.call("a")'

(send
  (block
    (lambda)
    (args)
    (lvar :"0it")) :call
  (str "a"))


bin/parse '-> { it }.call("a")'

(send
  (block
    (lambda)
    (args)
    (send nil :it)) :call
  (str "a"))

That's fine, I can adjust my code. However, there is another problem with using Prism: it disallows me to use _1, etc. in assignments:

/Users/palkan/dev/prism/lib/prism/translation/parser.rb:105:in `unwrap': _1 is reserved for numbered parameters (SyntaxError)

I need this to support numparams in Ruby <2.7 (where using _N is legal); for Parser, I just disable the check in the builder class: https://github.com/ruby-next/ruby-next/blob/4e24bd856f3bb57eac9500d46db9f06f47e0a67b/lib/ruby-next/language/parser.rb#L30

Is there a similar workaround for Prism?

kddnewton · 2024-02-01T17:52:55Z

Hey @palkan —

Ahh that's unfortunate. I've fixed the find pattern bug. It turns out the fixture we have for pattern matching has something the parser gem doesn't support (_1 being pinned) so the whole thing was being ignored. That's all fixed now.

For the it local variable you can pass a version to Prism.parse (though we need to wire it through the parser class). I think we should probably do that, because it will allow you to turn that off.

For the numbered parameter write, it's a little more difficult. We don't really have a way to turn that off. I've added a hook at the Ruby layer in ruby/prism#2335 where you could inherit from the parser class and then define a custom valid_error? method. You could use it to reject those errors by checking the message/location. It's not a great solution, but hopefully that works for your use case?

palkan · 2024-02-01T23:43:38Z

Thanks! I think, the valid_error? could do the trick; will try. Right now I'm facing this (using the current main):

$ RUBY_NEXT_PROPOSED=0 PRISM=true be bin/parse '-> { _1 }.call("a")'

bundler: failed to load command: bin/parse (bin/parse)
/Users/palkan/dev/prism/lib/prism/translation/parser/compiler.rb:1011:in `visit_lambda_node': undefined method `parameters' for @ NumberedParametersNode (location: (1,0)-(1,9)) (NoMethodError)
└── maximum: 1
:Prism::NumberedParametersNode

            node.body&.accept(copy_compiler(forwarding: find_forwarding(node.parameters&.parameters))),
                                                                                       ^^^^^^^^^^^^
        from /Users/palkan/dev/prism/lib/prism/node.rb:10836:in `accept'
        from /Users/palkan/dev/prism/lib/prism/compiler.rb:29:in `visit'
        from /Users/palkan/dev/prism/lib/prism/translation/parser/compiler.rb:283:in `visit_call_node'
        from /Users/palkan/dev/prism/lib/prism/node.rb:2308:in `accept'
        from /Users/palkan/dev/prism/lib/prism/compiler.rb:34:in `block in visit_all'
        from /Users/palkan/dev/prism/lib/prism/compiler.rb:34:in `map'
        from /Users/palkan/dev/prism/lib/prism/compiler.rb:34:in `visit_all'

kddnewton · 2024-02-02T17:56:14Z

@palkan oh man — thank you for your patience. I just merged a fix for this, could you try main on prism now?

palkan · 2024-02-02T20:27:04Z

Thanks! This issue is fixed now and the #valid_error? does the tick for it. We're almost there 🙂

Found another difference in parsing pattern matching:

$ be bin/parse '
case _
  in "a":
 end'

(case-match
  (send nil :_)
  (in-pattern
    (hash-pattern
      (match-var :a)) nil nil) nil)

$ PRISM=true be bin/parse '
case _
  in "a":
 end'

(case-match
  (send nil :_)
  (in-pattern
    (hash-pattern
      (pair
        (sym :"\"a\"")
        (const nil :"\"a\""))) nil nil) nil)

Parser uses a special node for hash pattern pairs (match-var), while Prism treats them similarly to regular hashes (pair).
That's true not only for string-like keys but for in a: as well.

Is it intentional or a bug? It fails for me because I use pair nodes to perform shorthand Hash syntax transpiling.

kddnewton · 2024-02-02T21:16:22Z

That was a bug — should be fixed now. Can you try main again?

palkan · 2024-02-02T21:38:41Z

It's green 🎉! https://github.com/ruby-next/ruby-next/actions/runs/7761821014/job/21171037498

kddnewton · 2024-02-05T18:08:23Z

@palkan I just released v0.21.0 that you should be able to use. I'll close this ticket for now, because I'm going to deprecate this gem.

palkan added 2 commits December 4, 2023 20:09

fix: Prism.parse accepts only source

4d9fc00

fix: some pattern matching edge cases

a394b64

kddnewton closed this Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor fixes (Ruby Next compatibility) #7

Minor fixes (Ruby Next compatibility) #7

palkan commented Dec 5, 2023

kddnewton commented Dec 5, 2023

kddnewton commented Jan 3, 2024

kddnewton commented Jan 3, 2024

palkan commented Jan 10, 2024

kddnewton commented Jan 17, 2024

palkan commented Jan 17, 2024

kddnewton commented Jan 29, 2024

palkan commented Jan 29, 2024

kddnewton commented Feb 1, 2024

palkan commented Feb 1, 2024

kddnewton commented Feb 2, 2024

palkan commented Feb 2, 2024

kddnewton commented Feb 2, 2024

palkan commented Feb 2, 2024

kddnewton commented Feb 5, 2024

Minor fixes (Ruby Next compatibility) #7

Minor fixes (Ruby Next compatibility) #7

Conversation

palkan commented Dec 5, 2023

kddnewton commented Dec 5, 2023

kddnewton commented Jan 3, 2024

kddnewton commented Jan 3, 2024

palkan commented Jan 10, 2024

kddnewton commented Jan 17, 2024

palkan commented Jan 17, 2024

kddnewton commented Jan 29, 2024

palkan commented Jan 29, 2024

kddnewton commented Feb 1, 2024

palkan commented Feb 1, 2024

kddnewton commented Feb 2, 2024

palkan commented Feb 2, 2024

kddnewton commented Feb 2, 2024

palkan commented Feb 2, 2024

kddnewton commented Feb 5, 2024