Add a mechanism to specify options on input-type fields (#124)

## Summary:
This has been a bit of a thorn since we started using genqlient in
production: just as you might want to specify, say, `omitempty` on an
argument, you might equally want to specify it on an input-type field.
But there's no obvious syntax to do that, because the input-type field
does not appear in the query (only the schema) so there's nowhere to put
the `# @genqlient` directive.

This commit, at last, fixes that problem, via a new option, `for`, which
you use in an option applied to the entire operation (or fragment), and
says, "actually, apply this directive to the given field, not the entire
operation".  (It's mainly useful for input types, but I allowed it for
output types too; I could imagine it being convenient if you want to say
you always use a certain type or type-name for a certain field.)  It
works basically like you expect: the inline options take precedence over
`for` take precedence over query-global options.

The implementation was fairly straightforward once I did a little
refactoring, mostly in the directive-parsing and directive-merging
(which are now combined, since merging is now a bit more complicated).
With that in place, and extended to support `for`, we need only add the
same wiring to input-fields that we have for other places you can put
directives.  I did not attempt to solve the issue I've now documented
as #123, wherein conflicting options can lead to confusing behavior;
the new `for` is a new and perhaps more attractive avenue to cause it
but the issue remains the same and requires nontrivial refactoring
(described in the issue) to solve.  (The breakage isn't horrible for the
most part; the option will just apply, or not apply, where you don't
expect it to.)

But while applying that logic, I noticed a problem, which is that we
were inconsistently cascading operation-level options down to
input-object fields.  (I think this came out of the fact that initially
I thought to cascade them, then realized that this could cause problems
like #123 and intended to walk them back, but then accidentally only
"fixed" it for `omitempty`.  I guess until this change, operation-level
options were rare enough, and input-field options messy enough, that no
one noticed.)  So in this commit I bring things back into consistency,
by saying that they do cascade: with at least a sketch of a path forward
to solve #123 via better validation, I think that's by far the clearest
behavior.

Issue: #14

## Test plan:
make check


Author: benjaminjkraft

Reviewers: csilvers, StevenACoffman, benjaminjkraft, aberkan, dnerdy, jvoll, mahtabsabet, MiguelCastillo

Required Reviewers: 

Approved By: csilvers, StevenACoffman

Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint

Pull Request URL: #124

Author: benjaminjkraft

docs/CHANGELOG.md

@@ -22,10 +22,13 @@ When releasing a new version:
 
 ### Breaking changes:
 
+- Previously, `# @genqlient` directives applied to entire operations applied inconsistently to fields of input types used by those operations.  Specifically, `pointer: true`, when applied to the operation, would affect all input-field arguments, but `omitempty: true` would not.  Now, all options apply to fields of input types; this is a behavior change in the case of `omitempty`.
+
 ### New features:
 
 - genqlient's types are now safe to JSON-marshal, which can be useful for putting them in a cache, for example.  See the [docs](FAQ.md#-let-me-json-marshal-my-response-objects) for details.
 - The new `flatten` option in the `# @genqlient` directive allows for a simpler form of type-sharing using fragment spreads.  See the [docs](FAQ.md#-shared-types-between-different-parts-of-the-query) for details.
+- The new `for` option in the `# @genqlient` directive allows applying options to a particular field anywhere it appears in the query.  This is especially useful for fields of input types, for which there is otherwise no way to specify options; see the [documentation on handling nullable fields](FAQ.md#-nullable-fields) for an example, and the [`# @genqlient` directive reference](genqlient_directive.graphql) for the full details.
 
 ### Bug fixes:
 

docs/FAQ.md

@@ -144,7 +144,7 @@ query MyQuery(
 }
 ```
 
-You can also put the `# @genqlient(omitempty: true)` on the first line, which will apply it to all arguments in the query.
+You can also put the `# @genqlient(omitempty: true)` on the first line, which will apply it to all arguments in the query, or `# @genqlient(for: "MyInput.myField", omitempty: true)` on the first line to apply it to a particular field of a particular input type used by the query (for which there would otherwise be no place to put the directive, as the field never appears explicitly in the query, but only in the schema).
 
 If you need to distinguish null from the empty string (or generally from the Go zero value of your type), you can tell genqlient to use a pointer for the field or argument like this:
 ```graphql
@@ -157,7 +157,40 @@ query MyQuery(
 }
 ```
 
-This will generate a Go field `MyString *string`, and set it to `nil` if the server returns null (and in reverse for arguments).  Such fields can be harder to work with in Go, but allow a clear distinction between null and the Go zero value.  Again, you can put the directive on the first line to apply it to everything in the query, although this usually gets cumbersome.
+This will generate a Go field `MyString *string`, and set it to `nil` if the server returns null (and in reverse for arguments).  Such fields can be harder to work with in Go, but allow a clear distinction between null and the Go zero value.  Again, you can put the directive on the first line to apply it to everything in the query, although this usually gets cumbersome, or use `for` to apply it to a specific input-type field.
+
+As an example of using all these options together:
+```graphql
+# @genqlient(omitempty: true)
+# @genqlient(for: "MyInputType.id", omitempty: false, pointer: true)
+# @genqlient(for: "MyInputType.name", omitempty: false, pointer: true)
+query MyQuery(
+  arg1: MyInputType!,
+  # @genqlient(pointer: true)
+  arg2: String!,
+  # @genqlient(omitempty: false)
+  arg3: String!,
+) {
+  myString(arg1: $arg1, arg2: $arg2, arg3: $arg3)
+}
+```
+This will generate:
+```go
+func MyQuery(
+  ctx context.Context,
+  client graphql.Client,
+  arg1 MyInputType,
+  arg2 *string, // omitempty
+  arg3 string,
+) (*MyQueryResponse, error)
+
+type MyInputType struct {
+  Id    *string `json:"id"`
+  Name  *string `json:"name"`
+  Title string  `json:"title,omitempty"`
+  Age   int     `json:"age,omitempty"`
+}
+```
 
 See [genqlient_directive.graphql](genqlient_directive.graphql) for complete documentation on these options.
 

docs/genqlient_directive.graphql

@@ -7,11 +7,12 @@
 # by the server, not by the client, so it would reject a real @genqlient
 # directive as nonexistent.)
 #
-# Directives may be applied to fields, arguments, or the entire query.
-# Directives on the line preceding the query apply to all relevant nodes in
-# the query; other directives apply to all nodes on the following line.  (In
-# all cases it's fine for there to be other comments in between the directive
-# and the node(s) to which it applies.)  For example, in the following query:
+# Directives may be applied to fields, arguments, or the entire query or named
+# fragment.  Directives on the line preceding the query or a named fragment
+# apply to all relevant nodes in the query; other directives apply to all nodes
+# on the following line.  (In all cases it's fine for there to be other
+# comments in between the directive and the node(s) to which it applies.)  For
+# example, in the following query:
 #  # @genqlient(n: "a")
 #
 #  # @genqlient(n: "b")
@@ -39,16 +40,48 @@
 # entire query (so "d", "e", and "f" take precedence over "b" and "c"), and
 # multiple directives on the same node ("b" and "c") must not conflict.  Note
 # that directives on nodes do *not* apply to their "children", so "d" does not
-# apply to the fields of MyInput, and "f" does not apply to field4.
+# apply to the fields of MyInput, and "f" does not apply to field4.  (But
+# directives on operations and fragments do: both "b" and "c" apply to fields
+# of MyInput and to field4.)
 directive genqlient(
 
-  # If set, this argument will be omitted if it has an empty value, defined
-  # (the same as in encoding/json) as false, 0, a nil pointer, a nil interface
-  # value, and any empty array, slice, map, or string.
+  # If set to a string "MyType.myField", this entire @genqlient directive
+  # will be treated as if it were applied to the specified field of the
+  # specified type.  It must be applied to an entire operation or fragment.
+  #
+  # This is especially useful for input-type options like omitempty and
+  # pointer, which are equally meaningful on input-type fields as on arguments,
+  # but there's no natural syntax to put them on fields.
+  #
+  # Note that for input types, unless the type has the "typename" option set,
+  # all operations and fragments in the same package which use this type should
+  # have matching directives.  (This is to avoid needing to give them more
+  # complex type-names.)  This is not currently validated, but will be
+  # validated in the future (see issue #123).
+  #
+  # For example, given the following query:
+  #  # @genqlient(for: "MyInput.myField", omitempty: true)
+  #  # @genqlient(for: "MyInput.myOtherField", pointer: true)
+  #  # @genqlient(for: "MyOutput.id", bind: "path/to/pkg.MyOutputID")
+  #  query MyQuery($arg: MyInput) { ... }
+  # genqlient will generate a type
+  #  type MyInput struct {
+  #    MyField      <type>  `json:"myField,omitempty"`
+  #    MyOtherField *<type> `json:"myField"`
+  #    MyThirdField <type>  `json:"myThirdField"`
+  #  }
+  # and use it for the argument to MyQuery, and similarly if `MyOutput.id` is
+  # ever requested in the response, it will be set to use the given type.
+  for: String
+
+  # If set, this argument (or input-type field, see "for") will be omitted if
+  # it has an empty value, defined (the same as in encoding/json) as false, 0,
+  # a nil pointer, a nil interface value, and any empty array, slice, map, or
+  # string.
   #
   # For example, given the following query:
   #  # @genqlient(omitempty: true)
-  #  query MyQuery(arg: String) { ... }
+  #  query MyQuery($arg: String) { ... }
   # genqlient will generate a function
   #  MyQuery(ctx context.Context, client graphql.Client, arg string) ...
   # which will pass {"arg": null} to GraphQL if arg is "", and the actual
@@ -161,8 +194,10 @@ directive genqlient(
   # type-name in multiple places unless they request the exact same fields, or
   # if your type-name conflicts with an autogenerated one (again, unless they
   # request the exact same fields).  They must even have the fields in the
-  # same order.  Fragments are often easier to use (see the discussion of
-  # code-sharing in FAQ.md, and the "flatten" option above).
+  # same order.  They should also have matching @genqlient directives, although
+  # this is not currently validated (see issue #123).  Fragments are often
+  # easier to use (see the discussion of code-sharing in FAQ.md, and the
+  # "flatten" option above).
   #
   # Note that unlike most directives, if applied to the entire operation,
   # typename affects the overall response type, rather than being propagated

generate/convert.go

@@ -164,11 +164,10 @@ func (g *generator) convertArguments(
 	name := "__" + operation.Name + "Input"
 	fields := make([]*goStructField, len(operation.VariableDefinitions))
 	for i, arg := range operation.VariableDefinitions {
-		_, directive, err := g.parsePrecedingComment(arg, arg.Position)
+		_, options, err := g.parsePrecedingComment(arg, nil, arg.Position, queryOptions)
 		if err != nil {
 			return nil, err
 		}
-		options := queryOptions.merge(directive)
 
 		goName := upperFirst(arg.Variable)
 		// Some of the arguments don't apply here, namely the name-prefix (see
@@ -386,19 +385,21 @@ func (g *generator) convertDefinition(
 		}
 
 		for i, field := range def.Fields {
+			_, fieldOptions, err := g.parsePrecedingComment(
+				field, def, field.Position, queryOptions)
+			if err != nil {
+				return nil, err
+			}
+
 			goName := upperFirst(field.Name)
-			// There are no field-specific options for inputs (yet, see #14),
-			// but we still need to merge with an empty directive to clear out
-			// any query-options that shouldn't apply here (namely "typename").
-			fieldOptions := queryOptions.merge(newGenqlientDirective(pos))
 			// Several of the arguments don't really make sense here:
 			// (note field.Type is necessarily a scalar, input, or enum)
 			// - namePrefix is ignored for input types and enums (see
 			//   names.go) and for scalars (they use client-specified
 			//   names)
 			// - selectionSet is ignored for input types, because we
 			//   just use all fields of the type; and it's nonexistent
-			//   for scalars and enums, our only other possible types,
+			//   for scalars and enums, our only other possible types
 			// TODO(benkraft): Can we refactor to avoid passing the values that
 			// will be ignored?  We know field.Type is a scalar, enum, or input
 			// type.  But plumbing that is a bit tricky in practice.
@@ -414,8 +415,7 @@ func (g *generator) convertDefinition(
 				JSONName:    field.Name,
 				GraphQLName: field.Name,
 				Description: field.Description,
-				// TODO(benkraft): set Omitempty once we have a way for the
-				// user to specify it.
+				Omitempty:   fieldOptions.GetOmitempty(),
 			}
 		}
 		return goType, nil
@@ -506,12 +506,11 @@ func (g *generator) convertSelectionSet(
 ) ([]*goStructField, error) {
 	fields := make([]*goStructField, 0, len(selectionSet))
 	for _, selection := range selectionSet {
-		_, selectionDirective, err := g.parsePrecedingComment(
-			selection, selection.GetPosition())
+		_, selectionOptions, err := g.parsePrecedingComment(
+			selection, nil, selection.GetPosition(), queryOptions)
 		if err != nil {
 			return nil, err
 		}
-		selectionOptions := queryOptions.merge(selectionDirective)
 
 		switch selection := selection.(type) {
 		case *ast.Field:
@@ -705,6 +704,8 @@ func (g *generator) convertFragmentSpread(
 		}
 	}
 
+	// TODO(benkraft): Set directive here if we ever allow @genqlient
+	// directives on fragment-spreads.
 	return &goStructField{GoName: "" /* i.e. embedded */, GoType: typ}, nil
 }
 
@@ -713,7 +714,7 @@ func (g *generator) convertFragmentSpread(
 func (g *generator) convertNamedFragment(fragment *ast.FragmentDefinition) (goType, error) {
 	typ := g.schema.Types[fragment.TypeCondition]
 
-	comment, directive, err := g.parsePrecedingComment(fragment, fragment.Position)
+	comment, directive, err := g.parsePrecedingComment(fragment, nil, fragment.Position, nil)
 	if err != nil {
 		return nil, err
 	}

generate/generate.go

@@ -244,7 +244,7 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error {
 	f := formatter.NewFormatter(&builder)
 	f.FormatQueryDocument(queryDoc)
 
-	commentLines, directive, err := g.parsePrecedingComment(op, op.Position)
+	commentLines, directive, err := g.parsePrecedingComment(op, nil, op.Position, nil)
 	if err != nil {
 		return err
 	}