Clarify to which nodes genqlient directives apply

[benjaminjkraft]

If you do

# @genqlient(...)
query Q(arg: T!) { field { subfield } }

then the directive applies -- and must be legal for -- not only query Q but also arg: T!, field, and subfield. This is documented but in a somewhat obscure place and has caused quite a bit of confusion (see e.g. #148/#149). It's also not clear how useful it is either.

Instead we could:

1. Warn more clearly if a directive applies multiple places, or at least add text to the error in the case where one of those places is invalid.
2. Only apply @genqlient directives to the first following node (so the above directive would apply to query Q only).
3. Don't allow @genqlient directives which would apply to multiple nodes.
4. Figure out how to make @genqlient directives real directives instead of comments -- this may be more possible now that we're rewriting queries (edit: and now that the new GraphQL spec adds repeatable directives and directives on variable definitions) although it has costs if it means other tools no longer consider the query in source valid (or require a schema addition to do so) -- and then ordinary GraphQL syntax applies.
5. Do (1), (2), or (3), but only for when one of the nodes is the entire query (the others are typically arguments), which has empirically been the most common source of confusion and seems the least useful. (Namely, one can imagine wanting the same directive to apply to several sibling fields, but applying it to both query and a particular argument seems nonsensical.)

I lean towards option 5.3, i.e. forbid this with a clear error if one of the nodes is the entire query; it seems to me that's unlikely to break anyone who wasn't already seeing confusing behavior and it's simple enough to avoid. But I'm open to other options depending on what others think or what's easy to implement!

[nathanstitt]

@csilvers and @benjaminjkraft thanks for both of your help with debugging #148, I've managed to debug the the root cause of the strange errors I was receiving. This discussion likely belongs on this issue since it relates to how comment directives are applied.

While these two statements appear identical, the comment will be applied differently:

This statement works fine.

# @genqlient(for: message_insert_input.created_by_id", omitempty: true)
mutation insertComment(
    $comment: message_insert_input!
) {
  insert_task_comment_one( object: $comment ) { id }
}

But this one fails with either: for is only applicable to operations and arguments or omitempty may only be used on optional arguments

# @genqlient(for: message_insert_input.created_by_id", omitempty: true)
mutation insertComment($comment: message_insert_input!) {
  insert_task_comment_one( object: $comment ) { id }
}

Note that the above statement lacks the newline before the variable declaration. The comment is parsed for both the *ast.OperationDefinition. and *ast.VariableDefinition: blocks in https://github.com/Khan/genqlient/blob/main/generate/genqlient_directive.go#L241

I'm not sure if this is really a bug, maybe the newline behavior just needs to be documented?

[benjaminjkraft]

Yeah, that's the maybe-bug I mean! It is documented but the documentation is very hidden and also clearly not explained well enough. I think what you say makes it clear that what we need to do is change the behavior, not just try to document it.

I'm liking option 3 above, personally, I think that leaves the least room for confusion. If I have time this week I can take a stab at doing that. (Note to self: it may be best to do it as a separate pass so that we can also look for directives that would apply to no nodes, which are also probably an error.)

[nathanstitt]

yes, option 3 would be great from my perspective.

if it had a clear error that the directive was applying to multiple nodes that would make it clear what would need to change in the graphql statement.