Descriptions/Comments/Locations rework

[sungam3r]

See #132 as a starting point. Fixes #141. Also now parser respects ignore-locations option for comments. Also fixes exceptions caused by placing comments in "unexpected" positions in query/SDL.

[codecov-commenter]

Codecov Report

Merging #142 (f44ea9c) into master (8c41c52) will decrease coverage by 0.75%.
The diff coverage is 94.83%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
- Coverage   92.66%   91.90%   -0.76%     
==========================================
  Files          49       50       +1     
  Lines        2317     2496     +179     
  Branches      273      278       +5     
==========================================
+ Hits         2147     2294     +147     
- Misses        143      180      +37     
+ Partials       27       22       -5     

Impacted Files	Coverage Δ
src/GraphQLParser/AST/GraphQLTypeDefinition.cs	100.00% <ø> (ø)
src/GraphQLParser/AST/GraphQLArgument.cs	63.63% <50.00%> (-7.80%)	⬇️
src/GraphQLParser/AST/GraphQLComment.cs	50.00% <50.00%> (ø)
src/GraphQLParser/AST/GraphQLDescription.cs	50.00% <50.00%> (ø)
src/GraphQLParser/AST/GraphQLFragmentDefinition.cs	60.00% <50.00%> (-6.67%)	⬇️
src/GraphQLParser/AST/GraphQLObjectField.cs	54.54% <50.00%> (-2.60%)	⬇️
...raphQLParser/AST/GraphQLTypeExtensionDefinition.cs	60.00% <50.00%> (-6.67%)	⬇️
src/GraphQLParser/AST/GraphQLName.cs	80.00% <66.66%> (-20.00%)	⬇️
...QLParser/AST/GraphQLRootOperationTypeDefinition.cs	72.72% <72.72%> (ø)
...rc/GraphQLParser/AST/GraphQLDirectiveDefinition.cs	75.00% <75.00%> (ø)
... and 35 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c41c52...f44ea9c. Read the comment docs.

[Shane32]

Note that GraphQLLocation is a struct, and takes almost no processing power to compute (since the location is already known). So if there are no optimized classes for skipping only locations, then running the parser with only IgnoreOptions.Locations is pointless, as it will use the same memory, and probably an identical amount of processing time. The reason the option existed previously is because when ignoring both comments and locations (neither of which are necessary for execution of a query by GraphQL.NET), optimized classes can be used that save memory requirements of both fields, and the heap allocations and processing power to process comment fields (which are often written in the blockstring format, although this is not a requirement).

I also think (if I remember right) that if an error occurs during parsing, an exception is generated within the parser that utilizes the location of the error, even if location is specified to be ignored when parsing.

So while flags are a better design, it may be a bit misleading to users in that ignoring locations (but not comments) will actually do anything beneficial. Just because of that reason, I'd probably keep the options values like they were, especially as we have no intention of adding additional flags or adding additional classes to options for ignoring only locations. Assume that we proceed with switching it to flags, I might add a comment to this effect - either in the xml comment for the enum option, or in the readme file.

[sungam3r]

So if there are no optimized classes for skipping only locations, then running the parser with only IgnoreOptions.Locations is pointless, as it will use the same memory, and probably an identical amount of processing time
So while flags are a better design, it may be a bit misleading to users in that ignoring locations (but not comments) will actually do anything beneficial.

I did so that every option gave effect. I have to write tests and put some code for reading comments in 3-4 places. When I worked on this PR, I realized that the processing of comments is a very unpleasant process. They may be everywhere! We are far from properly handling comment. I found that if you put a comment after the name of the arg but before its value, the parser generates an exception. Boom! See CommentsOnValues.graphql file. I have to add ParseComment() call into more places.

[sungam3r]

First three methods differ from others. GraphQLComment, GraphQLDocument and GraphQLDescription have no comments. All other methods are completely the same. Perhaps when I will write tests, I will see that comments are not applicable to some other AST nodes.

[Shane32]

Why not just discard all comments (according to spec) for v8? Eliminate the options for parsing it, eliminate the complexity of the classes to hold comments, etc.

Comments are Ignored like white space and may appear after any token, or before a LineTerminator, and have no significance to the semantic meaning of a GraphQL Document.

Just an idea...

[Shane32]

I renamed GraphQLCommentFull into GraphQLCommentWithLocation. I will soon make a commit, where I added a lot of new internal classes to align the behavior. All optimizations will be in place. I will need some time to finish and write tests. I want to show in advance what it turns out.

I really think you are wasting a lot of time here, and making the project unnecessarily complex with "a lot of new internal classes" that are not necessary when the only project that uses this parser -- GraphQL.NET -- only does two things: (a) skip comments for execution, and (b) load comments for backwards compatibility when creating a schema-first schema. And as for the second point, descriptions have long been in the spec now to be done by strings, not comments. So even that is not necessary for GraphQL.NET v5.

[Shane32]

(Edit: oops GraphQL always uses locations)

We should really eliminate optimization of parsing without locations, since GraphQL requires the use of locations in error messages.

[sungam3r]

Why not just discard all comments (according to spec) for v8? Eliminate the options for parsing it, eliminate the complexity of the classes to hold comments, etc.

Of course, we can do it. But from the point of view of the syntactic parsing it will be a step back. People may need comments on completely different reasons. The most obvious of them is the documentation provision through the introspection. We will force everyone to use descriptions. At best, consumers will have to change their SDLs. In the worst case, they will not be able to do this at all, because they do not own these schemas. Then they have to change these schemas (SDL) dynamically, which will require ... the syntactic analysis that we have deleted 😄 .

[sungam3r]

We should really eliminate optimization of parsing without locations, since GraphQL requires the use of locations in error messages.

I'm not sure that this requirement somehow limits us. The specification describes what should occur between the client and the server. Parser is a general mechanism that can work without the presence of any client. For example, it may be the transformation from one SDL to another SDL or to some different file format. Getting SDL statistics, whatever.

[Shane32]

None of those scenarios require memory optimization. Only execution does, which requires location information.

[sungam3r]

when the only project that uses this parser -- GraphQL.NET

Why so 😄 ? It is better to say "the only public project that uses this parser" although I doubt even that it is indeed the only public. I use the parser in my private projects for example. In any case, we need to eliminate exceptions caused by the placing comments in "unexpected" positions.

[Shane32]

You can write all that extra code if you want; I wouldn't 🤷‍♂️.

[Shane32]

In any case, we need to eliminate exceptions caused by the placing comments in "unexpected" positions.

For sure.

[Shane32]

I use the parser in my private projects for example.

Where (a) you skip parsing location information, and (b) in situations that require hyper-optimized memory allocations?

[Shane32]

Hey, go ahead 👍 ... maybe it's useful to someone; maybe you.

[sungam3r]

OK. By the way, if we talk about the complexity of the code, then after the changes made code became easier. I deleted all the ternary operators around AST creation and initializing. All properties are set once now. The number of codes in the "main" parser file decreased. I look positively on changes.

[Shane32]

Is it ready for another review?

[sungam3r]

Not yet. I'll let you know.

[sungam3r]

@Shane32 I'm ready for review. It turned out to be a big one PR.

What you should pay attention to:

1. Single ParseComment() call inside Advance so now we handle all comments anywhere.
2. All nodes parse and set their comments now.
3. New tests verify additional comments in new places. I can add additional tests for comments in awkward positions like before : or = tokens. Now all of them should reside into unattached comments. No more exceptions!
4. ParserContext ctor calls CreateGraphQLDocument and Advance.
5. I renamed some public classes to align their names to ones from the spec: GraphQLFieldSelection -> GraphQLField, GraphQLOperationTypeDefinition -> GraphQLRootOperationTypeDefinition

I also walked through all the ParseXXX methods and added comment to each with reference to the corresponding part of the specification (grammar).

[sungam3r]

Now it will be easier to work on the new features from the October spec.