Review comments, and a few more test cases

generate/convert.go

@@ -54,7 +54,7 @@ func (g *generator) convertOperation(
 	// 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(
-		&prefixList{last: operation.Name}, operation.SelectionSet, baseType, queryOptions)
+		newPrefixList(operation.Name), operation.SelectionSet, baseType, queryOptions)
 	if err != nil {
 		return nil, err
 	}

generate/names.go

@@ -65,18 +65,21 @@ package generate
 // in `query Q { user { user { id } } }` we do need both QUser and QUserUser --
 // they have different fields.
 //
-// Note that there are (at least) two potential collisions from this algorithm:
+// Note that there are a few potential collisions from this algorithm:
 // - When generating Go types for GraphQL interface types, we generate both
 //   ...MyFieldMyInterfaceType and ...MyFieldMyImplType.  If an interface's
 //   name is a suffix of its implementation's name, and both are suffixes of a
 //   field of that type, we'll shorten both, resulting in a collision.
-// - Given a query like
+// - Names of different casing (e.g. fields `myField` and `MyField`) can
+//   collide (the first is standard usage but both are legal).
+// - We don't put a special character between parts, so fields like
 //		query Q {
-//			ab { ... } # type: C
-//			a { ... }  # type: BC
+//			ab { ... }  # type: C
+//			abc { ... } # type: C
+//			a { ... }   # type: BC
 //		}
-//	 we generate QABC to represent both fields.
-// Both cases seem fairly rare in practice; eventually we'll likely allow users
+//   can collide.
+// All cases seem fairly rare in practice; eventually we'll likely allow users
 // the ability to specify their own names, which they could use to avoid this
 // (see https://github.com/Khan/genqlient/issues/12).
 // TODO(benkraft): We should probably at least try to detect it and bail.
@@ -99,19 +102,27 @@ import (
 	"github.com/vektah/gqlparser/v2/ast"
 )
 
-// Yes, a linked list!  We could use a stack -- it would probably be marginally
+// Yes, a linked list!  Of name-prefixes in *reverse* order, i.e. from end to
+// start.
+//
+// We could use a stack -- it would probably be marginally
 // more efficient -- but then the caller would have to know more about how to
 // manage it safely.  Using a list, and treating it as immutable, makes it
 // easy.
 type prefixList struct {
-	last string // the list goes back-to-front, so this is the *last* prefix
-	rest *prefixList
+	head string // the list goes back-to-front, so this is the *last* prefix
+	tail *prefixList
+}
+
+// creates a new one-element list
+func newPrefixList(item string) *prefixList {
+	return &prefixList{head: item}
 }
 
 func joinPrefixList(prefix *prefixList) string {
 	var reversed []string
-	for ; prefix != nil; prefix = prefix.rest {
-		reversed = append(reversed, prefix.last)
+	for ; prefix != nil; prefix = prefix.tail {
+		reversed = append(reversed, prefix.head)
 	}
 	l := len(reversed)
 	for i := 0; i < l/2; i++ {
@@ -131,10 +142,10 @@ func typeNameParts(prefix *prefixList, typeName string) *prefixList {
 	typeName = upperFirst(typeName)
 	// If the prefix has just one part, that's the operation-name.  There's no
 	// need to add "Query" or "Mutation".  (Zero should never happen.)
-	if prefix == nil || prefix.rest == nil ||
+	if prefix == nil || prefix.tail == nil ||
 		// If the last prefix field ends with this type's name, omit the
 		// type-name (see the "shortened" case in the top-of-file comment).
-		strings.HasSuffix(prefix.last, typeName) {
+		strings.HasSuffix(prefix.head, typeName) {
 		return prefix
 	}
 	return &prefixList{typeName, prefix}

generate/names_test.go

@@ -27,6 +27,16 @@ func TestTypeNames(t *testing.T) {
 		"OperationUser",
 		[]*ast.Field{fakeField("Query", "user")},
 		"User",
+	}, {
+		// We don't shorten field-names.
+		"OperationOperationUser",
+		[]*ast.Field{fakeField("Query", "operationUser")},
+		"User",
+	}, {
+		// We don't shorten across multiple prefixes.
+		"OperationUserOperationUser",
+		[]*ast.Field{fakeField("Query", "user")},
+		"OperationUser",
 	}, {
 		"OperationFavoriteUser",
 		[]*ast.Field{fakeField("Query", "favoriteUser")},
@@ -48,7 +58,7 @@ func TestTypeNames(t *testing.T) {
 	for _, test := range tests {
 		test := test
 		t.Run(test.expectedTypeName, func(t *testing.T) {
-			prefix := &prefixList{last: "Operation"}
+			prefix := newPrefixList("Operation")
 			for _, field := range test.fields {
 				prefix = nextPrefix(prefix, field)
 			}
@@ -60,3 +70,37 @@ func TestTypeNames(t *testing.T) {
 		})
 	}
 }
+
+func TestTypeNameCollisions(t *testing.T) {
+	tests := []struct {
+		fields       []*ast.Field
+		leafTypeName string
+	}{
+		{[]*ast.Field{fakeField("Query", "user")}, "UserInterface"},
+		{[]*ast.Field{fakeField("Query", "user")}, "User"},
+		{[]*ast.Field{fakeField("Query", "user")}, "QueryUser"},
+		{[]*ast.Field{fakeField("Query", "queryUser")}, "User"},
+		// Known issues, described in names.go file-documentation:
+		// Interface/implementation collision:
+		// 	{[]*ast.Field{fakeField("Query", "queryUser")}, "QueryUser"},
+		// Case collision:
+		// 	{[]*ast.Field{fakeField("Query", "QueryUser")}, "User"},
+		// Overlapping-parts collision:
+		//	{[]*ast.Field{fakeField("Query", "userQuery")}, "User"},
+	}
+	seen := map[string]int{} // name -> index of test that had it
+	for i, test := range tests {
+		prefix := newPrefixList("Operation")
+		for _, field := range test.fields {
+			prefix = nextPrefix(prefix, field)
+		}
+		actualTypeName := makeTypeName(prefix, test.leafTypeName)
+
+		otherIndex, ok := seen[actualTypeName]
+		if ok {
+			t.Errorf("name collision:\ncase %2d: %#v\ncase %2d: %#v",
+				i, test, otherIndex, tests[otherIndex])
+		}
+		seen[actualTypeName] = i
+	}
+}