Reject the use of both typename and bind (#172)

In #133, Craig added support for a new use of typename, where it applies
to a scalar and means that genqlient should generate a named type, e.g.
`# @genqlient(typename: "MyString")` on a node of type string will
generate and use `type MyString string`.  But this gets a bit confusing
if you mix it with `bind`; should
`typename: "MyString", bind: "int32"` generate `type MyString int32`, or
should one override the other, or what?  Of course in practice you're
not likely to write that all in one place, but you could via a global
binding, or a `for` directive, and in that case probably it was a
mistake.  In #138, we looked at making them work together correctly, but
it added complexity and got even more confusing.

So instead, here, we just ban it; we can always add it back if it proves
useful.  (Or, you can make the `typename` win over a global binding by
locally unbinding it via `bind: "-"`.)  This required changes in
surprisingly many places; I already knew the directive-validation code
was due for a refactor but that will happen some other day.  The tests
show that it works, in any case.

Interestingly, this problem actually could have arisen for a struct
binding already, before #133.  But all the same reasons it's confusing
seem to apply, so I just banned it there too.  This is technically a
breaking change although I doubt anyone will hit it.

Test plan: make check

Author: benjaminjkraft

docs/CHANGELOG.md

@@ -23,6 +23,7 @@ When releasing a new version:
 ### Breaking changes:
 
 - The `Config` fields `Schema` and `Operations` are now both of type `StringList`.  This does not affect configuration via `genqlient.yaml`, only via the Go API.
+- The `typename` and `bind` options may no longer be combined; doing so will now result in an error.  In practice, any such use was likely in error (and the rules for which would win were confusing and undocumented).
 
 ### New features:
 

docs/genqlient_directive.graphql

@@ -226,6 +226,10 @@ directive genqlient(
   # Note that unlike most directives, if applied to the entire operation,
   # typename affects the overall response type, rather than being propagated
   # down to all child fields (which would cause conflicts).
+  #
+  # To avoid confusion, typename may not be combined with local or global
+  # bindings; to use typename instead of a global binding do
+  # `typename: "MyTypeName", bind: "-"`.
   typename: String
 
 # Multiple genqlient directives are allowed in the same location, as long as

generate/convert.go

@@ -289,6 +289,11 @@ func (g *generator) convertDefinition(
 	// unless the binding is "-" which means "ignore the global binding".
 	globalBinding, ok := g.Config.Bindings[def.Name]
 	if ok && options.Bind != "-" {
+		if options.TypeName != "" {
+			return nil, errorf(pos,
+				"typename option conflicts with global binding for %s; "+
+					"use `bind: \"-\"` to override it", def.Name)
+		}
 		if def.Kind == ast.Object || def.Kind == ast.Interface || def.Kind == ast.Union {
 			err := g.validateBindingSelection(
 				def.Name, globalBinding, pos, selectionSet)

generate/genqlient_directive.go

@@ -213,6 +213,10 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er
 			if fieldDir.Omitempty != nil && field.Type.NonNull {
 				return errorf(fieldDir.pos, "omitempty may only be used on optional arguments")
 			}
+
+			if fieldDir.TypeName != "" && fieldDir.Bind != "" && fieldDir.Bind != "-" {
+				return errorf(fieldDir.pos, "typename and bind may not be used together")
+			}
 		}
 	}
 
@@ -255,6 +259,10 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er
 			return errorf(dir.pos, "for is only applicable to operations and arguments")
 		}
 
+		if dir.TypeName != "" && dir.Bind != "" && dir.Bind != "-" {
+			return errorf(dir.pos, "typename and bind may not be used together")
+		}
+
 		return nil
 	case *ast.Field:
 		if dir.Omitempty != nil {
@@ -278,6 +286,10 @@ func (dir *genqlientDirective) validate(node interface{}, schema *ast.Schema) er
 			return errorf(dir.pos, "for is only applicable to operations and arguments")
 		}
 
+		if dir.TypeName != "" && dir.Bind != "" && dir.Bind != "-" {
+			return errorf(dir.pos, "typename and bind may not be used together")
+		}
+
 		return nil
 	default:
 		return errorf(dir.pos, "invalid @genqlient directive location: %T", node)
@@ -490,6 +502,15 @@ func (g *generator) parsePrecedingComment(
 	if queryOptions != nil {
 		// If we are part of an operation/fragment, merge its options in.
 		directive.mergeOperationDirective(node, parentIfInputField, queryOptions)
+
+		// TODO(benkraft): Really we should do all the validation after
+		// merging, probably?  But this is the only check that can fail only
+		// after merging, and it's a bit tricky because the "does not apply"
+		// checks may need to happen before merging so we know where the
+		// directive "is".
+		if directive.TypeName != "" && directive.Bind != "" && directive.Bind != "-" {
+			return "", nil, errorf(directive.pos, "typename and bind may not be used together")
+		}
 	}
 
 	reverse(commentLines)

generate/testdata/errors/ConflictingTypeNameAndForFieldBind.graphql

@@ -0,0 +1,7 @@
+# @genqlient(for: "User.name", bind: "[]byte")
+query ConflictingTypeNameAndGlobalBind {
+  user {
+    # @genqlient(typename: "MyID")
+    name
+  }
+}

generate/testdata/errors/ConflictingTypeNameAndForFieldBind.schema.graphql

@@ -0,0 +1,8 @@
+type Query {
+  user: User
+}
+
+type User {
+  id: ID!
+  name: String!
+}

generate/testdata/errors/ConflictingTypeNameAndGlobalBind.graphql

@@ -0,0 +1,8 @@
+query ConflictingTypeNameAndGlobalBind {
+  user {
+    # @genqlient(typename: "OtherScalar")
+    name
+  }
+}
+
+

generate/testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql

@@ -0,0 +1,10 @@
+type Query {
+  user: User
+}
+
+type User {
+  id: ID!
+  name: ValidScalar!
+}
+
+scalar ValidScalar

generate/testdata/errors/ConflictingTypeNameAndLocalBind.graphql

@@ -0,0 +1,6 @@
+query ConflictingTypeNameAndGlobalBind {
+  user {
+    # @genqlient(bind: "[]byte", typename: "MyID")
+    name
+  }
+}

generate/testdata/errors/ConflictingTypeNameAndLocalBind.schema.graphql

@@ -0,0 +1,8 @@
+type Query {
+  user: User
+}
+
+type User {
+  id: ID!
+  name: String!
+}