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

Avoid parse ambiguity on types & extensions #598

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

leebyron
Copy link
Collaborator

Partial fix to #564, adds lookahead constraints to type system extensions to remove ambiguity or inefficiency from the grammar.

The GraphQL grammar should be parsed in linear type with at most one lookahead. Since extensions have an optional { } body, finding the token { should always attempt to parse the relevant portion of the type extension rather than completing the type extension and attempting to parse the query shorthand SelectionSet.

@leebyron leebyron added the ✏️ Editorial PR is non-normative or does not influence implementation label Jul 23, 2019
@leebyron leebyron added 🤷‍♀️ Ambiguity An issue/PR which identifies or fixes spec ambiguity and removed ✏️ Editorial PR is non-normative or does not influence implementation labels Jul 23, 2019
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leebyron I think it's the best solution that possible without breaking changes.

P.S. In retrospective, legacy SDL syntax that always requires curly braces (even if empty) offered a better solution to this problem:
https://github.com/graphql/graphql-js/blob/670bbaca5ed897d0ee2caadbc8ac3a0bbf281b87/src/language/parser.js#L73-L81

@leebyron leebyron changed the title Avoid parse ambiguity on type extensions Avoid parse ambiguity on types & extensions Jul 30, 2019
@leebyron
Copy link
Collaborator Author

P.S. In retrospective, legacy SDL syntax that always requires curly braces (even if empty) offered a better solution to this problem

Yeah, I agree. I believe I made a mistake with that change for type definitions.

@leebyron
Copy link
Collaborator Author

Updated this change to use a single token lookahead since that's more accurate to how we expect parser implementations to implement this restriction, and it reinforces the idea that parsing should be linear time.

Also includes both type extensions and type definitions, and updates the grammar appendix

Base automatically changed from master to main February 3, 2021 04:50
@leebyron leebyron added this to the May2021 milestone Apr 6, 2021
@leebyron leebyron added the 🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md) label Apr 8, 2021
Partial fix to #564, adds lookahead constraints to type system extensions to remove ambiguity or inefficiency from the grammar.

The GraphQL grammar should be parsed in linear type with at most one lookahead. Since extensions have an optional `{ }` body, finding the token `{` should always attempt to parse the relevant portion of the type extension rather than completing the type extension and attempting to parse the query shorthand SelectionSet.
@leebyron
Copy link
Collaborator Author

leebyron commented Apr 8, 2021

Here's a parser test case:

interface Foo

{ alpha: beta }

Before this change, this could be interpreted either as:

  1. An incompletely defined interface Foo interface Foo followed by a query shorthand querying the field "beta" with the alias "alpha" { alpha: beta }
  2. An interface "Foo" with a single field "alpha" of type "beta" interface Foo { alpha: beta }

After this change, option 2. is the only legal result. 1. is not possible since the incompletely defined interface must not be followed by {.

@leebyron leebyron merged commit d616285 into main Apr 8, 2021
@leebyron leebyron deleted the extension-parse-ambiguity branch April 8, 2021 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md) 🤷‍♀️ Ambiguity An issue/PR which identifies or fixes spec ambiguity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants