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: https://github.com/Khan/genqlient/pull/105

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,19 +223,16 @@ 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 {
@@ -220,6 +240,13 @@ func (g *generator) parsePrecedingComment(
 		}
 	}
 
+	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 }
+}

generate/testdata/snapshots/TestGenerate-MultipleDirectives.graphql-MultipleDirectives.graphql.go

@@ -0,0 +1,178 @@
+package test
+
+// Code generated by github.com/Khan/genqlient, DO NOT EDIT.
+
+import (
+	"encoding/json"
+	"fmt"
+	"time"
+
+	"github.com/Khan/genqlient/graphql"
+	"github.com/Khan/genqlient/internal/testutil"
+)
+
+// UserQueryInput is the argument to Query.users.
+//
+// Ideally this would support anything and everything!
+// Or maybe ideally it wouldn't.
+// Really I'm just talking to make this documentation longer.
+type MyInput struct {
+	Email *string `json:"email"`
+	Name  *string `json:"name"`
+	// id looks the user up by ID.  It's a great way to look up users.
+	Id         *testutil.ID      `json:"id"`
+	Role       *Role             `json:"role"`
+	Names      []*string         `json:"names"`
+	HasPokemon *testutil.Pokemon `json:"hasPokemon"`
+	Birthdate  *time.Time        `json:"-"`
+}
+
+func (v *MyInput) MarshalJSON() ([]byte, error) {
+
+	var fullObject struct {
+		*MyInput
+		Birthdate json.RawMessage `json:"birthdate"`
+		graphql.NoUnmarshalJSON
+	}
+	fullObject.MyInput = v
+
+	{
+
+		dst := &fullObject.Birthdate
+		src := v.Birthdate
+		var err error
+		*dst, err = testutil.MarshalDate(
+			src)
+		if err != nil {
+			return nil, fmt.Errorf(
+				"Unable to marshal MyInput.Birthdate: %w", err)
+		}
+	}
+
+	return json.Marshal(&fullObject)
+}
+
+// MyMultipleDirectivesResponse is returned by MultipleDirectives on success.
+type MyMultipleDirectivesResponse struct {
+	// user looks up a user by some stuff.
+	//
+	// See UserQueryInput for what stuff is supported.
+	// If query is null, returns the current user.
+	User  *MyMultipleDirectivesResponseUser        `json:"user"`
+	Users []*MyMultipleDirectivesResponseUsersUser `json:"users"`
+}
+
+// MyMultipleDirectivesResponseUser includes the requested fields of the GraphQL type User.
+// The GraphQL type's documentation follows.
+//
+// A User is a user!
+type MyMultipleDirectivesResponseUser struct {
+	// id is the user's ID.
+	//
+	// It is stable, unique, and opaque, like all good IDs.
+	Id *testutil.ID `json:"id"`
+}
+
+// MyMultipleDirectivesResponseUsersUser includes the requested fields of the GraphQL type User.
+// The GraphQL type's documentation follows.
+//
+// A User is a user!
+type MyMultipleDirectivesResponseUsersUser struct {
+	// id is the user's ID.
+	//
+	// It is stable, unique, and opaque, like all good IDs.
+	Id *testutil.ID `json:"id"`
+}
+
+// Role is a type a user may have.
+type Role string
+
+const (
+	// What is a student?
+	//
+	// A student is primarily a person enrolled in a school or other educational institution and who is under learning with goals of acquiring knowledge, developing professions and achieving employment at desired field. In the broader sense, a student is anyone who applies themselves to the intensive intellectual engagement with some matter necessary to master it as part of some practical affair in which such mastery is basic or decisive.
+	//
+	// (from [Wikipedia](https://en.wikipedia.org/wiki/Student))
+	RoleStudent Role = "STUDENT"
+	// Teacher is a teacher, who teaches the students.
+	RoleTeacher Role = "TEACHER"
+)
+
+// UserQueryInput is the argument to Query.users.
+//
+// Ideally this would support anything and everything!
+// Or maybe ideally it wouldn't.
+// Really I'm just talking to make this documentation longer.
+type UserQueryInput struct {
+	Email *string `json:"email"`
+	Name  *string `json:"name"`
+	// id looks the user up by ID.  It's a great way to look up users.
+	Id         *testutil.ID      `json:"id"`
+	Role       *Role             `json:"role"`
+	Names      []*string         `json:"names"`
+	HasPokemon *testutil.Pokemon `json:"hasPokemon"`
+	Birthdate  *time.Time        `json:"-"`
+}
+
+func (v *UserQueryInput) MarshalJSON() ([]byte, error) {
+
+	var fullObject struct {
+		*UserQueryInput
+		Birthdate json.RawMessage `json:"birthdate"`
+		graphql.NoUnmarshalJSON
+	}
+	fullObject.UserQueryInput = v
+
+	{
+
+		dst := &fullObject.Birthdate
+		src := v.Birthdate
+		var err error
+		*dst, err = testutil.MarshalDate(
+			src)
+		if err != nil {
+			return nil, fmt.Errorf(
+				"Unable to marshal UserQueryInput.Birthdate: %w", err)
+		}
+	}
+
+	return json.Marshal(&fullObject)
+}
+
+// __MultipleDirectivesInput is used internally by genqlient
+type __MultipleDirectivesInput struct {
+	Query   MyInput           `json:"query,omitempty"`
+	Queries []*UserQueryInput `json:"queries,omitempty"`
+}
+
+func MultipleDirectives(
+	client graphql.Client,
+	query MyInput,
+	queries []*UserQueryInput,
+) (*MyMultipleDirectivesResponse, error) {
+	__input := __MultipleDirectivesInput{
+		Query:   query,
+		Queries: queries,
+	}
+	var err error
+
+	var retval MyMultipleDirectivesResponse
+	err = client.MakeRequest(
+		nil,
+		"MultipleDirectives",
+		`
+query MultipleDirectives ($query: UserQueryInput, $queries: [UserQueryInput]) {
+	user(query: $query) {
+		id
+	}
+	users(query: $queries) {
+		id
+	}
+}
+`,
+		&retval,
+		&__input,
+	)
+	return &retval, err
+}
+

generate/testdata/snapshots/TestGenerate-MultipleDirectives.graphql-MultipleDirectives.graphql.json

@@ -0,0 +1,9 @@
+{
+  "operations": [
+    {
+      "operationName": "MultipleDirectives",
+      "query": "\nquery MultipleDirectives ($query: UserQueryInput, $queries: [UserQueryInput]) {\n\tuser(query: $query) {\n\t\tid\n\t}\n\tusers(query: $queries) {\n\t\tid\n\t}\n}\n",
+      "sourceLocation": "testdata/queries/MultipleDirectives.graphql"
+    }
+  ]
+}

generate/testdata/snapshots/TestGenerateErrors-ConflictingDirectiveArguments-graphql

@@ -0,0 +1 @@
+testdata/errors/ConflictingDirectiveArguments.graphql:2: conflicting values for pointer

generate/testdata/snapshots/TestGenerateErrors-ConflictingDirectives-graphql

@@ -0,0 +1 @@
+testdata/errors/ConflictingDirectives.graphql:3: conflicting values for pointer