C Parser incompatibility with AST nodes
swalkinshaw opened this issue · 6 comments
Describe the bug
I believe #4949 created an incompatibility with the C parser since the @source is expected to exist on every language node which points to the parser instance.
Versions
graphql version: >= 2.3.3
Steps to reproduce
Use the C parser to parse a document that causes a FragmentSpreadsArePossibleError (and possibly other validation errors)
Expected behavior
It parses 😄
Actual behavior
NoMethodError · undefined method 'line_at' for nil
gems/graphql-2.3.10/lib/graphql/language/nodes.rb:37 GraphQL::Language::Nodes::AbstractNode#line
gems/graphql-2.3.10/lib/graphql/static_validation/error.rb:39 block in GraphQL::StaticValidation::Error#locations
gems/graphql-2.3.10/lib/graphql/static_validation/error.rb:38 Array#map
gems/graphql-2.3.10/lib/graphql/static_validation/error.rb:38 GraphQL::StaticValidation::Error#locations
gems/graphql-2.3.10/lib/graphql/static_validation/error.rb:29 GraphQL::StaticValidation::Error#to_h
gems/graphql-2.3.10/lib/graphql/static_validation/rules/fragment_spreads_are_possible_error.rb:25 GraphQL::StaticValidation::FragmentSpreadsArePossibleError#to_h
Additional context
Did the C parser set source_string before? It does look like there was better conditional checking if source_string existed vs always assuming that @source does now.
It's supposed to work by the C parser assigning @line and @col during parsing, causing the ||= @source.line_at(...) to never evaluate. But it must be missing a spot 🤔
Could you share an example string that causes this error? All of GraphQL-Ruby's tests run with with GraphQL::CParser on CI and don't encounter this error, including the tests for this validation (which aren't very extensive, admittedly: https://github.com/rmosolgo/graphql-ruby/blob/master/spec/graphql/static_validation/rules/fragment_spreads_are_possible_spec.rb)
But I added some debugging with
line || raise("No line given: #{line.inspect}")and couldn't find any cases where it wasn't passed in by the parser 😖
Okay I mistakenly assumed this had to be caused by the C parser and it's not. Sorry for sending you down this rabbit hole 😓
The root cause of this is programmatically creating an invalid document from AST nodes. Now, we shouldn't be creating invalid documents of course but I'm kind of curious if this exposes an assumption that nodes always come from a parsed source or not.
Ah, that makes sense. I guess the best thing would be to return nil in this case -- certainly not to raise this error 🤦
For now I'm working around it by setting both pos attributes to 0 but I do think preserving the nil behaviour makes sense
Thank you ❤️