Fix type-naming in the presence of interfaces, and refactor it a lot (#…

…71)

## Summary:
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong.  But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.)  In this commit, in
any case, I fix it.

To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer.  In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy.  Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code  is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover.  (I even had some fun using
a linked list to implement the prefix-stack!)

This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up.  As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.

Issue: #8

## Test plan:
make tesc


Author: benjaminjkraft

Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo

Required Reviewers: 

Approved by: dnerdy

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

Pull request URL: #71

DESIGN.md

@@ -108,6 +108,8 @@ We'll do something similar to Apollo's naming scheme.  Specifically:
 - Fragments will have some naming scheme TBD but starting at the fragment.
 - Input objects will have a name starting at the type, since they always have the same fields, and often have naming schemes like "MyFieldInput" already.
 
+See `generate/names.go` for the precise algorithm.
+
 All of this may be configurable later.
 
 ### How to represent interfaces

generate/convert.go

@@ -11,7 +11,6 @@ package generate
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/vektah/gqlparser/v2/ast"
 )
@@ -51,20 +50,26 @@ func (g *generator) convertOperation(
 		return nil, errorf(operation.Position, "%v", err)
 	}
 
-	goTyp, err := g.convertDefinition(
-		name, operation.Name, baseType, operation.Position,
-		operation.SelectionSet, queryOptions, queryOptions)
-
-	if structType, ok := goTyp.(*goStructType); ok {
-		// Override the ordinary description; the GraphQL documentation for
-		// Query/Mutation is unlikely to be of value.
-		// TODO(benkraft): This is a bit awkward/fragile.
-		structType.Description =
-			fmt.Sprintf("%v is returned by %v on success.", name, operation.Name)
-		structType.Incomplete = false
+	// Instead of calling out to convertType/convertDefinition, we do our own
+	// thing, because we want to do a few things differently, and because we
+	// know we have an object type, so we can include only that case.
+	fields, err := g.convertSelectionSet(
+		newPrefixList(operation.Name), operation.SelectionSet, baseType, queryOptions)
+	if err != nil {
+		return nil, err
 	}
 
-	return goTyp, err
+	goType := &goStructType{
+		GoName: name,
+		Description: fmt.Sprintf(
+			"%v is returned by %v on success.", name, operation.Name),
+		GraphQLName: baseType.Name,
+		Fields:      fields,
+		Incomplete:  false,
+	}
+	g.typeMap[name] = goType
+
+	return goType, nil
 }
 
 var builtinTypes = map[string]string{
@@ -76,66 +81,23 @@ var builtinTypes = map[string]string{
 	"ID":      "string",
 }
 
-// typeName computes the name, in Go, that we should use for the given
-// GraphQL type definition.  This is dependent on its location within the query
-// (see DESIGN.md for more on why we generate type-names this way), which is
-// determined by the prefix argument; the nextPrefix result should be passed to
-// calls to typeName on any child types.
-func (g *generator) typeName(prefix string, typ *ast.Definition) (name, nextPrefix string) {
-	typeGoName := upperFirst(typ.Name)
-	if typ.Kind == ast.Enum || typ.Kind == ast.InputObject {
-		// If we're an enum or an input-object, there is only one type we
-		// will ever possibly generate for this type, so we don't need any
-		// of the qualifiers.  This is especially helpful because the
-		// caller is very likely to need to reference these types in their
-		// code.
-		return typeGoName, typeGoName
-	}
-
-	name = prefix
-	if !strings.HasSuffix(prefix, typeGoName) {
-		// If the field and type names are the same, we can avoid the
-		// duplication.  (We include the field name in case there are
-		// multiple fields with the same type, and the type name because
-		// that's the actual name (the rest are really qualifiers); but if
-		// they are the same then including it once suffices for both
-		// purposes.)
-		name += typeGoName
-	}
-
-	if typ.Kind == ast.Interface || typ.Kind == ast.Union {
-		// for interface/union types, we do not add the type name to the
-		// name prefix; we want to have QueryFieldType rather than
-		// QueryFieldInterfaceType.  So we just use the input prefix.
-		return name, prefix
-	}
-
-	// Otherwise, the name will also be the prefix for the next type.
-	return name, name
-}
-
 // convertInputType decides the Go type we will generate corresponding to an
 // argument to a GraphQL operation.
 func (g *generator) convertInputType(
-	opName string,
 	typ *ast.Type,
 	options, queryOptions *GenqlientDirective,
 ) (goType, error) {
-	// Sort of a hack: case the input type name to match the op-name.
-	// TODO(benkraft): this is another thing that breaks the assumption that we
-	// only need one of an input type, albeit in a relatively safe way.
-	name := matchFirst(typ.Name(), opName)
 	// note prefix is ignored here (see generator.typeName), as is selectionSet
 	// (for input types we use the whole thing)).
-	return g.convertType(name, "", typ, nil, options, queryOptions)
+	return g.convertType(nil, typ, nil, options, queryOptions)
 }
 
 // convertType decides the Go type we will generate corresponding to a
 // particular GraphQL type.  In this context, "type" represents the type of a
 // field, and may be a list or a reference to a named type, with or without the
 // "non-null" annotation.
 func (g *generator) convertType(
-	name, namePrefix string,
+	namePrefix *prefixList,
 	typ *ast.Type,
 	selectionSet ast.SelectionSet,
 	options, queryOptions *GenqlientDirective,
@@ -152,14 +114,14 @@ func (g *generator) convertType(
 	if typ.Elem != nil {
 		// Type is a list.
 		elem, err := g.convertType(
-			name, namePrefix, typ.Elem, selectionSet, options, queryOptions)
+			namePrefix, typ.Elem, selectionSet, options, queryOptions)
 		return &goSliceType{elem}, err
 	}
 
 	// If this is a builtin type or custom scalar, just refer to it.
 	def := g.schema.Types[typ.Name()]
 	goTyp, err := g.convertDefinition(
-		name, namePrefix, def, typ.Position, selectionSet, options, queryOptions)
+		namePrefix, def, typ.Position, selectionSet, options, queryOptions)
 
 	if options.GetPointer() {
 		// Whatever we get, wrap it in a pointer.  (Because of the way the
@@ -177,7 +139,7 @@ func (g *generator) convertType(
 // *ast.Definition, which represents the definition of a type in the GraphQL
 // schema, which may be referenced by a field-type (see convertType).
 func (g *generator) convertDefinition(
-	name, namePrefix string,
+	namePrefix *prefixList,
 	def *ast.Definition,
 	pos *ast.Position,
 	selectionSet ast.SelectionSet,
@@ -192,7 +154,7 @@ func (g *generator) convertDefinition(
 	if ok && options.Bind != "-" {
 		if def.Kind == ast.Object || def.Kind == ast.Interface || def.Kind == ast.Union {
 			err := g.validateBindingSelection(
-				name, globalBinding, pos, selectionSet)
+				def.Name, globalBinding, pos, selectionSet)
 			if err != nil {
 				return nil, err
 			}
@@ -207,6 +169,8 @@ func (g *generator) convertDefinition(
 
 	switch def.Kind {
 	case ast.Object:
+		name := makeTypeName(namePrefix, def.Name)
+
 		fields, err := g.convertSelectionSet(
 			namePrefix, selectionSet, def, queryOptions)
 		if err != nil {
@@ -224,6 +188,12 @@ func (g *generator) convertDefinition(
 		return goType, nil
 
 	case ast.InputObject:
+		// If we're an input-object, there is only one type we will ever
+		// possibly generate for this type, so we don't need any of the
+		// qualifiers.  This is especially helpful because the caller is very
+		// likely to need to reference these types in their code.
+		name := upperFirst(def.Name)
+
 		goType := &goStructType{
 			GoName:      name,
 			Description: def.Description,
@@ -243,7 +213,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(
-				field.Type.Name(), "", field.Type, nil, queryOptions, queryOptions)
+				namePrefix, field.Type, nil, queryOptions, queryOptions)
 			if err != nil {
 				return nil, err
 			}
@@ -259,6 +229,8 @@ func (g *generator) convertDefinition(
 		return goType, nil
 
 	case ast.Interface, ast.Union:
+		name := makeTypeName(namePrefix, def.Name)
+
 		sharedFields, err := g.convertSelectionSet(
 			namePrefix, selectionSet, def, queryOptions)
 		if err != nil {
@@ -276,24 +248,13 @@ func (g *generator) convertDefinition(
 		g.typeMap[name] = goType
 
 		for i, implDef := range implementationTypes {
-			// Note for shared fields we propagate forward the interface's
-			// name-prefix: that is, the implementations will have fields with
-			// types like
-			//	MyInterfaceMyFieldMyType
-			// not
-			//	MyInterfaceMyImplMyFieldMyType
-			//             ^^^^^^
-			// In particular, this means that the Go type of MyField will be
-			// the same across all the implementations; this is important so
-			// that we can write a method GetMyField() that returns it!
-			implName, _ := g.typeName(namePrefix, implDef)
 			// TODO(benkraft): In principle we should skip generating a Go
 			// field for __typename each of these impl-defs if you didn't
 			// request it (and it was automatically added by
 			// preprocessQueryDocument).  But in practice it doesn't really
 			// hurt, and would be extra work to avoid, so we just leave it.
 			implTyp, err := g.convertDefinition(
-				implName, namePrefix, implDef, pos, selectionSet, options, queryOptions)
+				namePrefix, implDef, pos, selectionSet, options, queryOptions)
 			if err != nil {
 				return nil, err
 			}
@@ -309,6 +270,10 @@ func (g *generator) convertDefinition(
 		return goType, nil
 
 	case ast.Enum:
+		// Like with InputObject, there's only one type we will ever generate
+		// for an enum.
+		name := upperFirst(def.Name)
+
 		goType := &goEnumType{
 			GoName:      name,
 			Description: def.Description,
@@ -342,7 +307,7 @@ func (g *generator) convertDefinition(
 // convertSelectionSet once for the interface, and once for each
 // implementation.
 func (g *generator) convertSelectionSet(
-	namePrefix string,
+	namePrefix *prefixList,
 	selectionSet ast.SelectionSet,
 	containingTypedef *ast.Definition,
 	queryOptions *GenqlientDirective,
@@ -464,7 +429,7 @@ func fragmentMatches(containingTypedef, fragmentTypedef *ast.Definition) bool {
 // parent selection-set (except of course they are only included in types the
 // fragment matches); see DESIGN.md for more.
 func (g *generator) convertInlineFragment(
-	namePrefix string,
+	namePrefix *prefixList,
 	fragment *ast.InlineFragment,
 	containingTypedef *ast.Definition,
 	queryOptions *GenqlientDirective,
@@ -486,7 +451,7 @@ func (g *generator) convertInlineFragment(
 // convertDefinition), because they come from the type-definition, not the
 // operation.
 func (g *generator) convertField(
-	namePrefix string,
+	namePrefix *prefixList,
 	field *ast.Field,
 	fieldOptions, queryOptions *GenqlientDirective,
 ) (*goStructField, error) {
@@ -497,24 +462,11 @@ func (g *generator) convertField(
 			field.Position, "undefined field %v", field.Alias)
 	}
 
-	// Needs to be exported for JSON-marshaling
 	goName := upperFirst(field.Alias)
+	namePrefix = nextPrefix(namePrefix, field)
 
-	typ := field.Definition.Type
-	fieldTypedef := g.schema.Types[typ.Name()]
-
-	// Note we don't deduplicate suffixes here -- if our prefix is GetUser and
-	// the field name is User, we do GetUserUser.  This is important because if
-	// you have a field called user on a type called User we need
-	// `query q { user { user { id } } }` to generate two types, QUser and
-	// QUserUser.  Note also this is named based on the GraphQL alias (Go
-	// name), not the field-name, because if we have
-	// `query q { a: f { b }, c: f { d } }` we need separate types for a and c,
-	// even though they are the same type in GraphQL, because they have
-	// different fields.
-	name, namePrefix := g.typeName(namePrefix+goName, fieldTypedef)
 	fieldGoType, err := g.convertType(
-		name, namePrefix, typ, field.SelectionSet,
+		namePrefix, field.Definition.Type, field.SelectionSet,
 		fieldOptions, queryOptions)
 	if err != nil {
 		return nil, err

generate/generate.go

@@ -117,7 +117,6 @@ func (g *generator) Types() (string, error) {
 }
 
 func (g *generator) getArgument(
-	opName string,
 	arg *ast.VariableDefinition,
 	operationDirective *GenqlientDirective,
 ) (argument, error) {
@@ -127,8 +126,7 @@ func (g *generator) getArgument(
 	}
 
 	graphQLName := arg.Variable
-	goTyp, err := g.convertInputType(
-		opName, arg.Type, directive, operationDirective)
+	goTyp, err := g.convertInputType(arg.Type, directive, operationDirective)
 	if err != nil {
 		return argument{}, err
 	}
@@ -231,7 +229,7 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error {
 
 	args := make([]argument, len(op.VariableDefinitions))
 	for i, arg := range op.VariableDefinitions {
-		args[i], err = g.getArgument(op.Name, arg, directive)
+		args[i], err = g.getArgument(arg, directive)
 		if err != nil {
 			return err
 		}