Fix broken behavior for several options on the same node (#105)

## Summary:
Previously, we actually allowed you to put several genqlient directives
on the same node, but the semantics were undocumented (and somewhat
confusing, when it comes to `typename`).  In order to support directives
on input options, we're actually going to be encouraging this usage (see
notes in #14), so it's time to fix it.  To avoid confusion, I just had
conflicting directives be an error, rather than defining which one
"wins".  The same applies to specifying the same option several
times in one directive.

I also fixed two small bugs:
- `typename` on an operation would incorrectly cascade down to
  all input types in a query (causing conflicts).
- directive parse errors had useless positions, now they're correct

## Test plan:
make check


Author: benjaminjkraft

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

Required Reviewers: 

Approved By: StevenACoffman, dnerdy

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: #105

Author: benjaminjkraft

docs/CHANGELOG.md

@@ -28,6 +28,7 @@ When releasing a new version:
 ### New features:
 
 - The new `bindings.marshaler` and `bindings.unmarshaler` options in `genqlient.yaml` allow binding to a type without using its standard JSON serialization; see the [documentation](genqlient.yaml) for details.
+- Multiple genqlient directives may now be applied to the same node, as long as they don't conflict; see the [directive documentation](genqlient_directive.graphql) for details.
 
 ### Bug fixes:
 

docs/genqlient_directive.graphql

@@ -21,15 +21,25 @@
 #  # @genqlient(n: "c")
 #  query MyQuery(arg1: String,
 #    # @genqlient(n: "d")
-#    arg2: String, arg3: String,
+#    arg2: String, arg3: MyInput,
 #    arg4: String,
 #  ) {
 #    # @genqlient(n: "e")
 #    field1, field2
-#    field3
+#    # @genqlient(n: "f")
+#    field3 {
+#      field4
+#    }
 #  }
 # the directive "a" is ignored, "b" and "c" apply to all relevant nodes in the
-# query, "d" applies to arg2 and arg3, and "e" applies to field1 and field2.
+# query, "d" applies to arg2 and arg3, "e" applies to field1 and field2, and
+# "f" applies to field3.
+#
+# Except as noted below, directives on nodes take precedence over ones on the
+# 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.
 directive genqlient(
 
   # If set, this argument will be omitted if it has an empty value, defined
@@ -125,7 +135,9 @@ directive genqlient(
   # down to all child fields (which would cause conflicts).
   typename: String
 
-) on
+# Multiple genqlient directives are allowed in the same location, as long as
+# they don't have conflicting options.
+) repeatable on
   # genqlient directives can go almost anywhere, although some options are only
   # applicable in certain locations as described above.
   | QUERY

generate/convert.go

@@ -290,7 +290,7 @@ func (g *generator) convertDefinition(
 	if options.TypeName != "" {
 		// If the user specified a name, use it!
 		name = options.TypeName
-		if namePrefix.head == name && namePrefix.tail == nil {
+		if namePrefix != nil && namePrefix.head == name && namePrefix.tail == nil {
 			// Special case: if this name is also the only component of the
 			// name-prefix, append the type-name anyway.  This happens when you
 			// assign a type name to an interface type, and we are generating
@@ -367,10 +367,12 @@ func (g *generator) convertDefinition(
 
 		for i, field := range def.Fields {
 			goName := upperFirst(field.Name)
-			// Several of the arguments don't really make sense here
+			// 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)
-			// - no field-specific options can apply, because this is
-			//   a field in the type, not in the query (see also #14).
 			// - namePrefix is ignored for input types and enums (see
 			//   names.go) and for scalars (they use client-specified
 			//   names)
@@ -381,7 +383,7 @@ func (g *generator) convertDefinition(
 			// will be ignored?  We know field.Type is a scalar, enum, or input
 			// type.  But plumbing that is a bit tricky in practice.
 			fieldGoType, err := g.convertType(
-				namePrefix, field.Type, nil, queryOptions, queryOptions)
+				namePrefix, field.Type, nil, fieldOptions, queryOptions)
 			if err != nil {
 				return nil, err
 			}

generate/genqlient_directive.go

@@ -19,66 +19,84 @@ type genqlientDirective struct {
 	TypeName  string
 }
 
+func newGenqlientDirective(pos *ast.Position) *genqlientDirective {
+	return &genqlientDirective{
+		pos: pos,
+	}
+}
+
 func (dir *genqlientDirective) GetOmitempty() bool { return dir.Omitempty != nil && *dir.Omitempty }
 func (dir *genqlientDirective) GetPointer() bool   { return dir.Pointer != nil && *dir.Pointer }
 func (dir *genqlientDirective) GetStruct() bool    { return dir.Struct != nil && *dir.Struct }
 
-func setBool(dst **bool, v *ast.Value) error {
+func setBool(optionName string, dst **bool, v *ast.Value, pos *ast.Position) error {
+	if *dst != nil {
+		return errorf(pos, "conflicting values for %v", optionName)
+	}
 	ei, err := v.Value(nil) // no vars allowed
 	if err != nil {
-		return errorf(v.Position, "invalid boolean value %v: %v", v, err)
+		return errorf(pos, "invalid boolean value %v: %v", v, err)
 	}
 	if b, ok := ei.(bool); ok {
 		*dst = &b
 		return nil
 	}
-	return errorf(v.Position, "expected boolean, got non-boolean value %T(%v)", ei, ei)
+	return errorf(pos, "expected boolean, got non-boolean value %T(%v)", ei, ei)
 }
 
-func setString(dst *string, v *ast.Value) error {
+func setString(optionName string, dst *string, v *ast.Value, pos *ast.Position) error {
+	if *dst != "" {
+		return errorf(pos, "conflicting values for %v", optionName)
+	}
 	ei, err := v.Value(nil) // no vars allowed
 	if err != nil {
-		return errorf(v.Position, "invalid string value %v: %v", v, err)
+		return errorf(pos, "invalid string value %v: %v", v, err)
 	}
 	if b, ok := ei.(string); ok {
 		*dst = b
 		return nil
 	}
-	return errorf(v.Position, "expected string, got non-string value %T(%v)", ei, ei)
+	return errorf(pos, "expected string, got non-string value %T(%v)", ei, ei)
 }
 
-func fromGraphQL(dir *ast.Directive, pos *ast.Position) (*genqlientDirective, error) {
-	if dir.Name != "genqlient" {
+// add adds to this genqlientDirective struct the settings from then given
+// GraphQL directive.
+//
+// If there are multiple genqlient directives are applied to the same node,
+// e.g.
+//	# @genqlient(...)
+//	# @genqlient(...)
+// add will be called several times.  In this case, conflicts between the
+// options are an error.
+func (dir *genqlientDirective) add(graphQLDirective *ast.Directive, pos *ast.Position) error {
+	if graphQLDirective.Name != "genqlient" {
 		// Actually we just won't get here; we only get here if the line starts
 		// with "# @genqlient", unless there's some sort of bug.
-		return nil, errorf(pos, "the only valid comment-directive is @genqlient, got %v", dir.Name)
+		return errorf(pos, "the only valid comment-directive is @genqlient, got %v", graphQLDirective.Name)
 	}
 
-	var retval genqlientDirective
-	retval.pos = pos
-
 	var err error
-	for _, arg := range dir.Arguments {
+	for _, arg := range graphQLDirective.Arguments {
 		switch arg.Name {
-		// TODO: reflect and struct tags?
+		// TODO(benkraft): Use reflect and struct tags?
 		case "omitempty":
-			err = setBool(&retval.Omitempty, arg.Value)
+			err = setBool("omitempty", &dir.Omitempty, arg.Value, pos)
 		case "pointer":
-			err = setBool(&retval.Pointer, arg.Value)
+			err = setBool("pointer", &dir.Pointer, arg.Value, pos)
 		case "struct":
-			err = setBool(&retval.Struct, arg.Value)
+			err = setBool("struct", &dir.Struct, arg.Value, pos)
 		case "bind":
-			err = setString(&retval.Bind, arg.Value)
+			err = setString("bind", &dir.Bind, arg.Value, pos)
 		case "typename":
-			err = setString(&retval.TypeName, arg.Value)
+			err = setString("typename", &dir.TypeName, arg.Value, pos)
 		default:
-			return nil, errorf(pos, "unknown argument %v for @genqlient", arg.Name)
+			return errorf(pos, "unknown argument %v for @genqlient", arg.Name)
 		}
 		if err != nil {
-			return nil, err
+			return err
 		}
 	}
-	return &retval, nil
+	return nil
 }
 
 func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) error {
@@ -185,11 +203,16 @@ func (dir *genqlientDirective) merge(other *genqlientDirective) *genqlientDirect
 	return &retval
 }
 
+// parsePrecedingComment looks at the comment right before this node, and
+// returns the genqlient directive applied to it (or an empty one if there is
+// none), the remaining human-readable comment (or "" if there is none), and an
+// error if the directive is invalid.
 func (g *generator) parsePrecedingComment(
 	node interface{},
 	pos *ast.Position,
 ) (comment string, directive *genqlientDirective, err error) {
-	directive = new(genqlientDirective)
+	directive = newGenqlientDirective(pos)
+	hasDirective := false
 	if pos == nil || pos.Src == nil { // node was added by genqlient itself
 		return "", directive, nil // treated as if there were no comment
 	}
@@ -200,26 +223,30 @@ func (g *generator) parsePrecedingComment(
 		line := strings.TrimSpace(sourceLines[i-1])
 		trimmed := strings.TrimSpace(strings.TrimPrefix(line, "#"))
 		if strings.HasPrefix(line, "# @genqlient") {
-			graphQLDirective, err := parseDirective(trimmed, pos)
+			hasDirective = true
+			var graphQLDirective *ast.Directive
+			graphQLDirective, err = parseDirective(trimmed, pos)
 			if err != nil {
 				return "", nil, err
 			}
-			genqlientDirective, err := fromGraphQL(graphQLDirective, pos)
+			err = directive.add(graphQLDirective, pos)
 			if err != nil {
 				return "", nil, err
 			}
-			err = genqlientDirective.validate(node, g.schema)
-			if err != nil {
-				return "", nil, err
-			}
-			directive = directive.merge(genqlientDirective)
 		} else if strings.HasPrefix(line, "#") {
 			commentLines = append(commentLines, trimmed)
 		} else {
 			break
 		}
 	}
 
+	if hasDirective { // (else directive is empty)
+		err = directive.validate(node, g.schema)
+		if err != nil {
+			return "", nil, err
+		}
+	}
+
 	reverse(commentLines)
 
 	return strings.TrimSpace(strings.Join(commentLines, "\n")), directive, nil

generate/testdata/errors/ConflictingDirectiveArguments.graphql

@@ -0,0 +1,2 @@
+# @genqlient(pointer: true, pointer: false)
+query ConflictingDirectiveArguments { f }

generate/testdata/errors/ConflictingDirectiveArguments.schema.graphql

@@ -0,0 +1,3 @@
+type Query {
+  f: String
+}

generate/testdata/errors/ConflictingDirectives.graphql

@@ -0,0 +1,3 @@
+# @genqlient(pointer: true)
+# @genqlient(pointer: false)
+query ConflictingDirectives { f }

generate/testdata/errors/ConflictingDirectives.schema.graphql

@@ -0,0 +1,3 @@
+type Query {
+  f: String
+}

generate/testdata/queries/MultipleDirectives.graphql

@@ -0,0 +1,12 @@
+# @genqlient(typename: "MyMultipleDirectivesResponse")
+# @genqlient(omitempty: true)
+# @genqlient(pointer: true)
+query MultipleDirectives(
+  # @genqlient(pointer: false)
+  # @genqlient(typename: "MyInput")
+  $query: UserQueryInput,
+  $queries: [UserQueryInput],
+) {
+  user(query: $query) { id }
+  users(query: $queries) { id }
+}