search

nrc / graphql

A Rust GraphQL server framework
Apache License 2.0
236 stars 12 forks source link

Use graphql-parser for parser and AST #36

Open tailhook opened 6 years ago

tailhook commented 6 years ago

Hi,

I've just published graphq-parser crate. The idea is that it's just plain query language parser, that can be used for many things including this library.

It's based on quite good combine library for parsing, and has custom tokenizer/lexer. The library works quite well without macros. And the grammar of graphql is quite good so I could implement it with combinators being very close to what is written in spec. The performance is quite close to what juniper has with custom parser (it's hard to compare to this crate because this crate lacks features).

Comparing to implementation here the library implements all the graphql spec and includes line numbers for AST nodes.

It's still early in development so we may change few things to make it more useful for this crate and juniper. Also, it was weekend project so may have some edge cases not covered yet (but at least we can parse the kitchen sink example from graphql-js repo. IDL is not yet started, but looks like no bigger than another weekend project :)

Thoughts?

tailhook commented 6 years ago

I've also pushed SDL implementation to graphql-parser v0.2.0. It also has full grammar implemented.

nrc commented 6 years ago

Hey, this looks like a pretty cool crate! It would be nice to 'outsource' parsing to another crate, and it would be even nicer to get complete parsing. However, I am in general pretty cynical about generated parsers of any kind, they tend to have many disadvantages which only show up very late in compiler development:

There is also usually an increased barrier to entry for contributors - a recursive decent parser is pretty easy to understand, even if it is often quote verbose, understanding a generated parser usually requires learning another language or a very abstract/indirect coding style). Because of all this, it is still very uncommon to see generated parsers in industrial compilers.

So, I'm not sure what to do here. In particular, GraphQL is not a regular programming language and the constraints on the GraphQL server are very different to a regular compiler, so I'm not which of the above issues are relevant.

What would be quite nice I think, is to be able to plug-in either your generated parser or the custom parser which already exists so that we quickly experiment with new features without implementing parsing and can compare the outputs of the two parsers for testing. Does that sound like an attractive solution to you?

nrc commented 6 years ago

Oh, and it means that in the future, if we're sure the above issues don't apply, then it makes it easy to remove all the custom code and just use the graphql-parser crate/

tailhook commented 6 years ago

I have to note that combine isn't parser-generator library like lalrpop or pest. It's parser combinator library. I.e. it uses regular rust code without codegen or even macros.

There is also usually an increased barrier to entry for contributors - a recursive decent parser is pretty easy to understand

Well, I often find it confusing and time-consuming to find bugs in custom parsers. When using parser-combinators its mostly grammar as is. But that's probably a matter of taste and experience.

What would be quite nice I think, is to be able to plug-in either your generated parser or the custom parser which already exists so that we quickly experiment with new features without implementing parsing and can compare the outputs of the two parsers for testing. Does that sound like an attractive solution to you?

That sounds easy if we agree on the format of the AST. But I don't think that splitting AST from grammar (parser) into a separate crate is a good idea. So I'm sceptical for it.

Also, I'm not sure what are you're expecting to compare, because current parser here has very limited functionality and doesn't even track line numbers. I don't it's a good idea to invest more in the handwritten parser unless it's shown to be better in some respect.

As I've already noted above, the performance of graphql-parser is within 0-20% of the one of handwritten juniper parser (actually I'm not confinced that it isn't just random jitter when benchmarking).

Also, I've noted that I've implemented full grammar. And there are no weird cases in the current grammar. The language is quite new and grammar is specified quite well.

Maybe we can work out error reporting? I have not much tests for error code paths, here is how errors generaly look like and how I expect to test them (I've not spent time to format them carefully, it's just what we have out of the box).

May be you can show few examples which you expect to be difficult to report carefully so we can check if it's really hard?

derekdreery commented 6 years ago

What's to stop just splitting out the parser that's in this crate and working on it separately?