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

Try a hand-written recursive descent parser #4646

Closed
rmosolgo opened this issue Sep 21, 2023 · 2 comments · Fixed by #4718
Closed

Try a hand-written recursive descent parser #4646

rmosolgo opened this issue Sep 21, 2023 · 2 comments · Fixed by #4718

Comments

@rmosolgo
Copy link
Owner

@tenderlove demonstrated that YJIT can make a minimal Ruby parser faster than a C one: https://railsatscale.com/2023-08-29-ruby-outperforms-c/

It'd be worth exploring that in this context, to see if this gem's racc-based parser could be replaced by a hand-written one. The demo is here: https://github.com/tenderlove/tinygql

But I'd like to preserve total compatibility, so it'd need line and column handling.

There's also a big possibility for improving GraphQL-Ruby's parser error messages. I asked about this in Racc but never tried it myself (ruby/racc#88).

@tenderlove
Copy link
Contributor

@rmosolgo TinyGQL keeps the position of the node in the document, so column / line can be calculated later. The catch though is that you need to keep the original source around in order to derive them.

Example from the TinyGQL README:

require "tinygql"

doc = <<-eod
mutation {
  likeStory(sturyID: 12345) {
    story {
      likeCount
    }
  }
}

eod

parser = TinyGQL::Parser.new doc
ast = parser.parse

ast.find_all(&:field?).each { |node|
  p node.name => node.line(doc)
}

Output from this code is:

$ ruby -I lib test.rb
{"likeStory"=>2}
{"story"=>3}
{"likeCount"=>4}

There's no column method implemented, but since we have the position, we could easily add a column method.

Another problem is that the parser in TinyGQL relies on the lexer being "lazy". It pulls tokens from the lexer, but the lexer in GraphQL is eager, it lexes all tokens in advance and hands an array to the parser.

Requiring the source to be kept in order to derive location information plus the laziness of the lexer seems like fairly large changes. I think we could provide basically the same end goals (ast + source code info etc), but I'm not 100% sure how to do it in a backwards compatible way.

@rmosolgo
Copy link
Owner Author

Done in #4718

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 a pull request may close this issue.

2 participants