Support for GraphQL comments

[comatory]

Resolves #4905

This PR is in draft state, we want to get feedback early so we know we're on the right track.

The idea is that we expanded methods that construct the schema with new comment parameter (we should cover most of them like types, unions, enums). By passing string value to this new parameter, the resulting schema will contain comments such as this:

# we added support for comments
type MyType {
  name: String!
}

The reasons for adding this are mentioned in the linked issue but in a nutshell, we want to leverage comments for tooling (eslint), using descriptions is not desireable because those would leak into documentation.

The next steps are to add support for comments to every possible place according to GraphQL specification, e.g. on definitions for enums, inputs, unions etc.
At the moment, we are fine with having single-line comments and expand to multi-line comments in subsequent PRs, which means eventually we want this to be possible:

# ❌ not yet implemented
# first line
# second line
type MyType {
  name: String!
}

We haven't yet looked into edge cases such as mixing descriptions with comments, but eventually those should be supported as well.

[rmosolgo]

👍 I think this is absolutely on the right track!

Let me know if I can do anything particular to help this work along.

[comatory]

@rmosolgo could you please approve CI pipeline run? Thanks

Next steps for us is to implement support for multi-line comments, we'd need to be able to parse them. For printing (output), we were thinking of for allowing newline characters if user wished to have multiline output. This would mean the comment argument could accept value such as: "first line\nsecond line" and the output would be:

# first line
# second line

We think it's good enough API for users to format it this way instead of some complex solution, such as allowing to pass lists or having some kind of automated mechanism (e.g. break after 80 chars).

If you think that printing multiple line comments is something that's not needed or wanted, we're OK with not having it.

[comatory]

This code is not very easy to read, to be honest we added it because one of the tests was failing and this one worked.

[rmosolgo]

I agree that working from line breaks is the right way to render multiline comments. Ideally, GraphQL-Ruby would be able to parse SDL with multiline comments and print them back out exactly as they were originally written. (For your use case with es-lint, that might even be a hard requirement -- we don't want those directives getting merged with the explanatory comments above them, right?)

[comatory]

Ideally, GraphQL-Ruby would be able to parse SDL with multiline comments and print them back out exactly as they were originally written. (For your use case with es-lint, that might even be a hard requirement -- we don't want those directives getting merged with the explanatory comments above them, right?)

Yeah exactly, we should respect that during parsing that's for sure. Yes for eslint, it's important the specific comment needs to be placed right above the statement it's related to.

We'll try to polish this first and have the tests pass, which will probably require us to figure out multiline comments in the process 👍

[comatory]

@rmosolgo 👋 hello, is it possible to approve running pipelines on this PR? We're in the process of fixing specs and it'd be nice if we could see results in CI. 🙇

[rmosolgo]

👀 I see the button to allow CI once, but I'm not sure how enable it for a PR. I couldn't find a doc from GitHub on how to do it -- do you know how?

[comatory]

@rmosolgo Hm I thought there might be some settings in repository. I only found this but not sure if it helps. Anyway, thanks!

[rmosolgo]

Ah... yes, there is a setting there: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories

I switched it so only "new GitHub accounts" need me to approve them. Hopefully that will do it 🤞

[rmosolgo]

Wrong button 🤦

[vaclavbohac]

@rmosolgo Hello, we've got to a point where most of the tests are passing (of course the changes themselves are far from final) but we are stuck on the c-parser integration. It's a little mysterious how it works to us and we could use some guidance how to fix it and what the best approach here would be. Could you help us please?

cc @comatory

[rmosolgo]

👋 Sure thing. I'd be happy to take care of the C-Parser changes whenever the Ruby implementation is ready to go. Or, if you're interested in investigating it yourself, I think the main thing is updating the C parser to pass comments when initializing AST nodes.

First, the lexer would need to be updated to actually emit comment tokens. Currently, they're ignored:

graphql-ruby/graphql-c_parser/ext/graphql_c_parser_ext/lexer.rl

Lines 385 to 388 in 843014f

	// COMMENTs are retained as `previous_token` but aren't pushed to the normal token list
	if (tt != COMMENT) {
	rb_ary_push(meta->tokens, token);
	}

That if ... clause should be removed so that comments appear in the list of tokens.

Then, the parser would need to be updated to match comments wherever they appear. Since descriptions are very similar, structurally, I think a similar approach would work. Define a rule for optionally matching a comment (eg, comment_opt), like this one:

graphql-ruby/graphql-c_parser/ext/graphql_c_parser_ext/parser.y

Lines 521 to 525 in 843014f

	description: STRING
	description_opt:
	/* none */ { $$ = Qnil; }
	| description

Then add comment_opt wherever a comment may appear in the document, probably before description_opt in these examples:

graphql-ruby/graphql-c_parser/ext/graphql_c_parser_ext/parser.y

Lines 539 to 540 in 843014f

	object_type_definition:
	description_opt TYPE_LITERAL name implements_opt directives_list_opt field_definition_list_opt {

graphql-ruby/graphql-c_parser/ext/graphql_c_parser_ext/parser.y

Lines 586 to 587 in 843014f

	input_value_definition:
	description_opt name COLON type default_value_opt directives_list_opt {

graphql-ruby/graphql-c_parser/ext/graphql_c_parser_ext/parser.y

Lines 658 to 659 in 843014f

	union_type_definition:
	description_opt UNION name directives_list_opt EQUALS union_members {

Then, since Yacc uses indexes to reference matched tokens, all the indexes in those rules would have to be updated. (comment_opt would be $1, so description_opt would now be $2, and all the others would increment also.)

Finally, you'd have to pass $1 to the MAKE_AST_NODE macro so that the node would be initialized with the comment. This macro calls .from_a:

graphql-ruby/graphql-c_parser/ext/graphql_c_parser_ext/parser.y

Line 15 in 843014f

#define MAKE_AST_NODE(node_class_name, nargs, ...) rb_funcall(GraphQL_Language_Nodes_##node_class_name, rb_intern("from_a"), nargs + 1, filename,__VA_ARGS__)

Finally, you'd have to make sure that the .from_a method was ready to receive the new comment input. That method is usually generated here:

graphql-ruby/lib/graphql/language/nodes.rb

Lines 320 to 322 in 843014f

	def self.from_a(filename, line, col, #{all_method_names.join(", ")})
	self.new(filename: filename, line: line, col: col, #{keywords.join(", ")})
	end

But a couple of nodes have custom definitions for one reason or another, so those would need to be manually updated to receive comment and pass it as comment: comment:

graphql-ruby/lib/graphql/language/nodes.rb

Line 411 in 843014f

def self.from_a(filename, line, col, field_alias, name, arguments, directives, selections) # rubocop:disable Metrics/ParameterLists

graphql-ruby/lib/graphql/language/nodes.rb

Line 448 in 843014f

def self.from_a(filename, line, col, name, type, directives, selections)

I think that after that, the C Parser would be making ASTs identical to the Ruby parser's ASTs, so we'd be done. But there are always surprises 😅

[vaclavbohac]

@rmosolgo Wow, thanks for such a detailed answer. The required work is more or less what I expected we'll need to do, and with the guidance of what you wrote above, I think I will be able to do it.

[rmosolgo]

Happy to! Thanks for all your work on this feature so far 🍻

[rmosolgo]

Ahh... I realize I missed a spot when I described the changes required here. We also need a YACC token type for COMMENT.

The token types are configured in two places. First, they're laid out in an enum in lexer.rl:

graphql-ruby/graphql-c_parser/ext/graphql_c_parser_ext/lexer.rl

Lines 150 to 193 in 18fdf6f

	typedef enum TokenType {
	AMP,
	BANG,
	COLON,
	DIRECTIVE,
	DIR_SIGN,
	ENUM,
	ELLIPSIS,
	EQUALS,
	EXTEND,
	FALSE_LITERAL,
	FLOAT,
	FRAGMENT,
	IDENTIFIER,
	INPUT,
	IMPLEMENTS,
	INT,
	INTERFACE,
	LBRACKET,
	LCURLY,
	LPAREN,
	MUTATION,
	NULL_LITERAL,
	ON,
	PIPE,
	QUERY,
	RBRACKET,
	RCURLY,
	REPEATABLE,
	RPAREN,
	SCALAR,
	SCHEMA,
	STRING,
	SUBSCRIPTION,
	TRUE_LITERAL,
	TYPE_LITERAL,
	UNION,
	VAR_SIGN,
	BLOCK_STRING,
	QUOTED_STRING,
	UNKNOWN_CHAR,
	COMMENT,
	BAD_UNICODE_ESCAPE
	} TokenType;

It looks like COMMENT is already on the list there 👍

Then, the enum values are mapped to YACC token types here:

graphql-ruby/graphql-c_parser/ext/graphql_c_parser_ext/parser.y

Lines 61 to 98 in 18fdf6f

	// YACC Declarations
	%token AMP 200
	%token BANG 201
	%token COLON 202
	%token DIRECTIVE 203
	%token DIR_SIGN 204
	%token ENUM 205
	%token ELLIPSIS 206
	%token EQUALS 207
	%token EXTEND 208
	%token FALSE_LITERAL 209
	%token FLOAT 210
	%token FRAGMENT 211
	%token IDENTIFIER 212
	%token INPUT 213
	%token IMPLEMENTS 214
	%token INT 215
	%token INTERFACE 216
	%token LBRACKET 217
	%token LCURLY 218
	%token LPAREN 219
	%token MUTATION 220
	%token NULL_LITERAL 221
	%token ON 222
	%token PIPE 223
	%token QUERY 224
	%token RBRACKET 225
	%token RCURLY 226
	%token REPEATABLE 227
	%token RPAREN 228
	%token SCALAR 229
	%token SCHEMA 230
	%token STRING 231
	%token SUBSCRIPTION 232
	%token TRUE_LITERAL 233
	%token TYPE_LITERAL 234
	%token UNION 235
	%token VAR_SIGN 236

COMMENT is not on the list there, since it was previously excluded from the token stream. It needs to be added. Those integer values are based on their position in the enum declaration in lexer.rl, so I think we need to add %token COMMENT 240.

After that, you could define this rule as comment: COMMENT.

[rmosolgo]

I did some updates to the C parser to accept comments in a70db53 (I would have pushed it to your branch but it looks like "allow commits from maintainers" isn't turned on.)

It parses again, but there's a new challenge. When I suggested parsing comments as "real" tokens (instead of ignoring them), I didn't realize that it would require expecting comments anywhere that they could occur. They could occur anywhere, so it would be very hard to go back and account for that possibility. Here's an example where a comment breaks the current implementation: 18dd7af

-        query ($sizes: [ImageSize]) # Anonymous inline comment
+        query # Comment between parts of a query
+          ($sizes: [ImageSize]) # Anonymous inline comment

I think we've got to go back to ignoring comments, but keeping a reference to the last one that we parsed (or something?) so that we can tack it on to parsed nodes. 🤔 I'll give something a try next week.

Thanks so much for all your work on this change! I'm going to help get it across the finish line next week so we can get int released.

[vaclavbohac]

@rmosolgo Hi, and thank you very much for helping us with this!

I was think about it over the weekend and I thought whether we need to parse the comments at all. I mean I don't see the use case for it.

What we were trying to achieve here is to be able to generate comments into the schema. We never thought we would need it other way around.

We went down the rabbit hole of parsing only because there were certain tests parsing and printing the schema and expecting the structure would be the same. If only we ignored comments there, it would work again.

The thing is the parsing as you mentioned is incredibly difficult to be done right, the comments as you said could be anywhere.

Regarding the contributes, we sent you an invite to the repo, if it doesn't work. I'll make the fork to my personal github and reopen the PR from there as I will have a full control over the settings. Let us know :-)

[rmosolgo]

👍 Sounds good to me to reduce the scope of this change to only go Ruby-to-GraphQL. I forgot that was the real goal here 😅 If we want to add parsing later, we can always do it.

[vaclavbohac]

Excellent, I'm on it 👍

[vaclavbohac]

FYI I'm working on the changes in a separate branch. I will open the PR once I have most of the work done.

[vaclavbohac]

This PR can be closed then as #5067 has been merged.

cc @rmosolgo @comatory

[rmosolgo]

Thanks again for all your work here ... and sorry I sent you down a road to nowhere 😖!