Repository: Khan/genqlient
Pull request #172 (2022-02-10 00:27:14)

Reject the use of both typename and bind
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

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla) (hehe I realized I actually should go and sign this now!)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable (n/a)
- [x] Included documentation, for new features
- [x] Added an entry to the changelog


benjaminjkraft: Yeah, I'm only 90% sure I thought of all the ways it could happen, but I decided that's way better than 0%!

Patch:
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index f1bd00d..52aafc0 100644
--- a/docs/CHANGELOG.md
+++ b/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:
 
diff --git a/docs/genqlient_directive.graphql b/docs/genqlient_directive.graphql
index 57e8b2f..e06945d 100644
--- a/docs/genqlient_directive.graphql
+++ b/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
diff --git a/generate/convert.go b/generate/convert.go
index d4a21f1..fb256a2 100644
--- a/generate/convert.go
+++ b/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)
diff --git a/generate/genqlient_directive.go b/generate/genqlient_directive.go
index 06bc24b..886215a 100644
--- a/generate/genqlient_directive.go
+++ b/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)


Test patch:
diff --git a/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.graphql b/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.graphql
new file mode 100644
index 0000000..9bf6484
--- /dev/null
+++ b/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.graphql
@@ -0,0 +1,7 @@
+# @genqlient(for: "User.name", bind: "[]byte")
+query ConflictingTypeNameAndGlobalBind {
+  user {
+    # @genqlient(typename: "MyID")
+    name
+  }
+}
diff --git a/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.schema.graphql b/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.schema.graphql
new file mode 100644
index 0000000..ac8ac54
--- /dev/null
+++ b/generate/testdata/errors/ConflictingTypeNameAndForFieldBind.schema.graphql
@@ -0,0 +1,8 @@
+type Query {
+  user: User
+}
+
+type User {
+  id: ID!
+  name: String!
+}
diff --git a/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.graphql b/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.graphql
new file mode 100644
index 0000000..3491072
--- /dev/null
+++ b/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.graphql
@@ -0,0 +1,8 @@
+query ConflictingTypeNameAndGlobalBind {
+  user {
+    # @genqlient(typename: "OtherScalar")
+    name
+  }
+}
+
+
diff --git a/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql b/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql
new file mode 100644
index 0000000..42dfe46
--- /dev/null
+++ b/generate/testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql
@@ -0,0 +1,10 @@
+type Query {
+  user: User
+}
+
+type User {
+  id: ID!
+  name: ValidScalar!
+}
+
+scalar ValidScalar
diff --git a/generate/testdata/errors/ConflictingTypeNameAndLocalBind.graphql b/generate/testdata/errors/ConflictingTypeNameAndLocalBind.graphql
new file mode 100644
index 0000000..adb8fbe
--- /dev/null
+++ b/generate/testdata/errors/ConflictingTypeNameAndLocalBind.graphql
@@ -0,0 +1,6 @@
+query ConflictingTypeNameAndGlobalBind {
+  user {
+    # @genqlient(bind: "[]byte", typename: "MyID")
+    name
+  }
+}
diff --git a/generate/testdata/errors/ConflictingTypeNameAndLocalBind.schema.graphql b/generate/testdata/errors/ConflictingTypeNameAndLocalBind.schema.graphql
new file mode 100644
index 0000000..ac8ac54
--- /dev/null
+++ b/generate/testdata/errors/ConflictingTypeNameAndLocalBind.schema.graphql
@@ -0,0 +1,8 @@
+type Query {
+  user: User
+}
+
+type User {
+  id: ID!
+  name: String!
+}
diff --git a/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndForFieldBind-graphql b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndForFieldBind-graphql
new file mode 100644
index 0000000..cfaee0f
--- /dev/null
+++ b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndForFieldBind-graphql
@@ -0,0 +1,1 @@
+testdata/errors/ConflictingTypeNameAndForFieldBind.graphql:5: typename and bind may not be used together
diff --git a/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndGlobalBind-graphql b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndGlobalBind-graphql
new file mode 100644
index 0000000..164bc4c
--- /dev/null
+++ b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndGlobalBind-graphql
@@ -0,0 +1,1 @@
+testdata/errors/ConflictingTypeNameAndGlobalBind.schema.graphql:8: typename option conflicts with global binding for ValidScalar; use `bind: "-"` to override it
diff --git a/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndLocalBind-graphql b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndLocalBind-graphql
new file mode 100644
index 0000000..2ef277a
--- /dev/null
+++ b/generate/testdata/snapshots/TestGenerateErrors-ConflictingTypeNameAndLocalBind-graphql
@@ -0,0 +1,1 @@
+testdata/errors/ConflictingTypeNameAndLocalBind.graphql:4: typename and bind may not be used together