Add support for abstract-typed named fragments (#79)

## Summary:
In previous commits I added support to genqlient for interfaces,
inline fragments, and, most recently, named fragments of concrete
(object) type.  This leaves only named fragments of interface type!
Like other named fragments, these are useful for code-sharing,
especially if you want some code that can handle the same fields of
several different types.

As seems to be inevitable with genqlient, this was mostly pretty
straightforward, although there turned out to be surprisingly many
places we needed to add some handling; almost anywhere that touches
interfaces *or* named fragments needed some updates.  But it's all
hopefully fairly clear code.

As a part of this change I made three semi-related improvements:
1. I refactored the handling of descriptions (i.e. GoDoc), because it
   was getting more and more confusing and duplicative.  I'm still not
   sure how much of it it makes sense to inline vs. separate, but I
   think this is better than it was.  This resulted in some minor
   changes to descriptions, generally in the direction of making things
   more consistent.
2. I bumped the minimum Go version to 1.14 so we can guarantee support
   for duplicate interface methods.  These are useful for
   abstract-in-absstract spreads; we generate an interface for the
   fragment, and (if the fragment-type implements the scope-type) we
   embed it into the interface we generate for its spread-context, and
   if the two have a duplicated field we thus duplicate the method.  It
   wouldn't be impossible to support this on 1.13 (maybe just by
   omitting said embed) but it didn't seem worth it.  This also removes
   a few special-cases in tests.
3. I added a bunch of code to better format syntax errors in the
   generated code (which we see from `gofmt`).  This is mostly just an
   internal improvement; I wrote it because I got annoyed while hunting
   down a few such errors..

Fixes, at last, #8.

Issue: #8

## Test plan:
make check


Author: benjaminjkraft

Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo

Required Reviewers: 

Approved By: dnerdy

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

Pull Request URL: #79

.github/workflows/go.yml

@@ -12,7 +12,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        go: [ '1.13', '1.14', '1.15', '1.16', '1.17' ]
+        go: [ '1.14', '1.15', '1.16', '1.17' ]
 
     steps:
     - name: Set up Go

generate/convert.go

@@ -61,11 +61,13 @@ func (g *generator) convertOperation(
 
 	goType := &goStructType{
 		GoName: name,
-		Description: fmt.Sprintf(
-			"%v is returned by %v on success.", name, operation.Name),
-		GraphQLName: baseType.Name,
-		Fields:      fields,
-		Incomplete:  false,
+		descriptionInfo: descriptionInfo{
+			CommentOverride: fmt.Sprintf(
+				"%v is returned by %v on success.", name, operation.Name),
+			GraphQLName: baseType.Name,
+			// omit the GraphQL description for baseType; it's uninteresting.
+		},
+		Fields: fields,
 	}
 	g.typeMap[name] = goType
 
@@ -167,6 +169,12 @@ func (g *generator) convertDefinition(
 		return &goOpaqueType{goBuiltinName}, nil
 	}
 
+	desc := descriptionInfo{
+		// TODO(benkraft): Copy any comment above this selection-set?
+		GraphQLDescription: def.Description,
+		GraphQLName:        def.Name,
+	}
+
 	switch def.Kind {
 	case ast.Object:
 		name := makeTypeName(namePrefix, def.Name)
@@ -178,11 +186,9 @@ func (g *generator) convertDefinition(
 		}
 
 		goType := &goStructType{
-			GoName:      name,
-			Description: def.Description,
-			GraphQLName: def.Name,
-			Fields:      fields,
-			Incomplete:  true,
+			GoName:          name,
+			Fields:          fields,
+			descriptionInfo: desc,
 		}
 		g.typeMap[name] = goType
 		return goType, nil
@@ -195,10 +201,10 @@ func (g *generator) convertDefinition(
 		name := upperFirst(def.Name)
 
 		goType := &goStructType{
-			GoName:      name,
-			Description: def.Description,
-			GraphQLName: def.Name,
-			Fields:      make([]*goStructField, len(def.Fields)),
+			GoName:          name,
+			Fields:          make([]*goStructField, len(def.Fields)),
+			descriptionInfo: desc,
+			IsInput:         true,
 		}
 		g.typeMap[name] = goType
 
@@ -240,10 +246,9 @@ func (g *generator) convertDefinition(
 		implementationTypes := g.schema.GetPossibleTypes(def)
 		goType := &goInterfaceType{
 			GoName:          name,
-			Description:     def.Description,
-			GraphQLName:     def.Name,
 			SharedFields:    sharedFields,
 			Implementations: make([]*goStructType, len(implementationTypes)),
+			descriptionInfo: desc,
 		}
 		g.typeMap[name] = goType
 
@@ -354,14 +359,31 @@ func (g *generator) convertSelectionSet(
 	//	{ id, id, id, ... on SubType { id } }
 	// (which, yes, is legal) we'll treat that as just { id }.
 	uniqFields := make([]*goStructField, 0, len(selectionSet))
+	fragmentNames := make(map[string]bool, len(selectionSet))
 	fieldNames := make(map[string]bool, len(selectionSet))
 	for _, field := range fields {
+		// If you embed a field twice via a named fragment, we keep both, even
+		// if there are complicated overlaps, since they are separate types to
+		// us.  (See also the special handling for IsEmbedded in
+		// unmarshal.go.tmpl.)
+		//
+		// But if you spread the samenamed fragment twice, e.g.
+		//	{ ...MyFragment, ... on SubType { ...MyFragment } }
+		// we'll still deduplicate that.
+		if field.JSONName == "" {
+			name := field.GoType.Reference()
+			if fragmentNames[name] {
+				continue
+			}
+			uniqFields = append(uniqFields, field)
+			fragmentNames[name] = true
+			continue
+		}
+
 		// GraphQL (and, effectively, JSON) requires that all fields with the
 		// same alias (JSON-name) must be the same (i.e. refer to the same
 		// field), so that's how we deduplicate.
-		// It's fine to have duplicate embeds (i.e. via named fragments), even
-		// ones with complicated overlaps, since they are separate types to us.
-		if field.JSONName != "" && fieldNames[field.JSONName] {
+		if fieldNames[field.JSONName] {
 			// GraphQL (and, effectively, JSON) forbids you from having two
 			// fields with the same alias (JSON-name) that refer to different
 			// GraphQL fields.  But it does allow you to have the same field
@@ -475,46 +497,99 @@ func (g *generator) convertFragmentSpread(
 		}
 	}
 
+	iface, ok := typ.(*goInterfaceType)
+	if ok && containingTypedef.Kind == ast.Object {
+		// If the containing type is concrete, and the fragment spread is
+		// abstract, refer directly to the appropriate implementation, to save
+		// the caller having to do type-assertions that will always succeed.
+		//
+		// That is, if you do
+		//	fragment F on I { ... }
+		//  query Q { a { ...F } }
+		// for the fragment we generate
+		//  type F interface { ... }
+		//  type FA struct { ... }
+		//  // (other implementations)
+		// when you spread F into a context of type A, we embed FA, not F.
+		for _, impl := range iface.Implementations {
+			if impl.GraphQLName == containingTypedef.Name {
+				typ = impl
+			}
+		}
+	}
+
 	return &goStructField{GoName: "" /* i.e. embedded */, GoType: typ}, nil
 }
 
 // convertNamedFragment converts a single GraphQL named fragment-definition
 // (`fragment MyFragment on MyType { ... }`) into a Go struct.
 func (g *generator) convertNamedFragment(fragment *ast.FragmentDefinition) (goType, error) {
 	typ := g.schema.Types[fragment.TypeCondition]
-	if !g.Config.AllowBrokenFeatures &&
-		(typ.Kind == ast.Interface || typ.Kind == ast.Union) {
-		return nil, errorf(fragment.Position, "not implemented: abstract-typed fragments")
-	}
 
-	description, directive, err := g.parsePrecedingComment(fragment, fragment.Position)
+	comment, directive, err := g.parsePrecedingComment(fragment, fragment.Position)
 	if err != nil {
 		return nil, err
 	}
 
-	// If the user included a comment, use that.  Else make up something
-	// generic; there's not much to say though.
-	if description == "" {
-		description = fmt.Sprintf(
-			"%v includes the GraphQL fields of %v requested by the fragment %v.",
-			fragment.Name, fragment.TypeCondition, fragment.Name)
+	desc := descriptionInfo{
+		CommentOverride:    comment,
+		GraphQLName:        typ.Name,
+		GraphQLDescription: typ.Description,
+		FragmentName:       fragment.Name,
 	}
 
+	// The rest basically follows how we convert a definition, except that
+	// things like type-names are a bit different.
+
 	fields, err := g.convertSelectionSet(
 		newPrefixList(fragment.Name), fragment.SelectionSet, typ, directive)
 	if err != nil {
 		return nil, err
 	}
 
-	goType := &goStructType{
-		GoName:      fragment.Name,
-		Description: description,
-		GraphQLName: fragment.TypeCondition,
-		Fields:      fields,
-		Incomplete:  false,
+	switch typ.Kind {
+	case ast.Object:
+		goType := &goStructType{
+			GoName:          fragment.Name,
+			Fields:          fields,
+			descriptionInfo: desc,
+		}
+		g.typeMap[fragment.Name] = goType
+		return goType, nil
+	case ast.Interface, ast.Union:
+		implementationTypes := g.schema.GetPossibleTypes(typ)
+		goType := &goInterfaceType{
+			GoName:          fragment.Name,
+			SharedFields:    fields,
+			Implementations: make([]*goStructType, len(implementationTypes)),
+			descriptionInfo: desc,
+		}
+		g.typeMap[fragment.Name] = goType
+
+		for i, implDef := range implementationTypes {
+			implFields, err := g.convertSelectionSet(
+				newPrefixList(fragment.Name), fragment.SelectionSet, implDef, directive)
+			if err != nil {
+				return nil, err
+			}
+
+			implDesc := desc
+			implDesc.GraphQLName = implDef.Name
+
+			implTyp := &goStructType{
+				GoName:          fragment.Name + upperFirst(implDef.Name),
+				Fields:          implFields,
+				descriptionInfo: implDesc,
+			}
+			goType.Implementations[i] = implTyp
+			g.typeMap[implTyp.GoName] = implTyp
+		}
+
+		return goType, nil
+	default:
+		return nil, errorf(fragment.Position, "invalid type for fragment: %v is a %v",
+			fragment.TypeCondition, typ.Kind)
 	}
-	g.typeMap[fragment.Name] = goType
-	return goType, nil
 }
 
 // convertField converts a single GraphQL operation-field into a Go

generate/description.go

@@ -0,0 +1,81 @@
+package generate
+
+// Code relating to generating GoDoc from GraphQL descriptions.
+//
+// For fields, and types where we just copy the "whole" type (enum and
+// input-object), this is easy: we just use the GraphQL description.  But for
+// struct types, there are often more useful things we can say.
+
+import (
+	"fmt"
+	"strings"
+)
+
+// descriptionInfo is embedded in types whose descriptions may be more complex
+// than just a copy of the GraphQL doc.
+type descriptionInfo struct {
+	// user-specified comment for this type
+	CommentOverride string
+	// name of the corresponding GraphQL type
+	GraphQLName string
+	// GraphQL description of the type .GraphQLName, if any
+	GraphQLDescription string
+	// name of the corresponding GraphQL fragment (on .GraphQLName), if any
+	FragmentName string
+}
+
+func maybeAddTypeDescription(info descriptionInfo, description string) string {
+	if info.GraphQLDescription == "" {
+		return description
+	}
+	return fmt.Sprintf(
+		"%v\nThe GraphQL type's documentation follows.\n\n%v",
+		description, info.GraphQLDescription)
+}
+
+func fragmentDescription(info descriptionInfo) string {
+	return maybeAddTypeDescription(info, fmt.Sprintf(
+		"%v includes the GraphQL fields of %v requested by the fragment %v.",
+		info.FragmentName, info.GraphQLName, info.FragmentName))
+}
+
+func structDescription(typ *goStructType) string {
+	switch {
+	case typ.CommentOverride != "":
+		return typ.CommentOverride
+	case typ.IsInput:
+		// Input types have all their fields, just use the GraphQL description.
+		return typ.GraphQLDescription
+	case typ.FragmentName != "":
+		return fragmentDescription(typ.descriptionInfo)
+	default:
+		// For types where we only have some fields, note that, along with
+		// the GraphQL documentation (if any).  We don't want to just use
+		// the GraphQL documentation, since it may refer to fields we
+		// haven't selected, say.
+		return maybeAddTypeDescription(typ.descriptionInfo, fmt.Sprintf(
+			"%v includes the requested fields of the GraphQL type %v.",
+			typ.GoName, typ.GraphQLName))
+	}
+}
+
+func interfaceDescription(typ *goInterfaceType) string {
+	goImplNames := make([]string, len(typ.Implementations))
+	for i, impl := range typ.Implementations {
+		goImplNames[i] = impl.Reference()
+	}
+	implementationList := fmt.Sprintf(
+		"\n\n%v is implemented by the following types:\n\t%v",
+		typ.GoName, strings.Join(goImplNames, "\n\t"))
+
+	switch {
+	case typ.CommentOverride != "":
+		return typ.CommentOverride + implementationList
+	case typ.FragmentName != "":
+		return fragmentDescription(typ.descriptionInfo) + implementationList
+	default:
+		return maybeAddTypeDescription(typ.descriptionInfo, fmt.Sprintf(
+			"%v includes the requested fields of the GraphQL interface %v.%v",
+			typ.GoName, typ.GraphQLName, implementationList))
+	}
+}

generate/errors.go

@@ -1,8 +1,11 @@
 package generate
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
+	"go/scanner"
+	"math"
 	"strconv"
 	"strings"
 
@@ -130,3 +133,49 @@ func errorf(pos *ast.Position, msg string, args ...interface{}) error {
 		wrapped: wrapped,
 	}
 }
+
+// goSourceError processes the error(s) returned by go tooling (gofmt, etc.)
+// into a nice error message.
+//
+// In practice, such errors are genqlient internal errors, but it's still
+// useful to format them nicely for debugging.
+func goSourceError(
+	failedOperation string, // e.g. "gofmt", for the error message
+	source []byte,
+	err error,
+) error {
+	var errTexts []string
+	var scanErrs scanner.ErrorList
+	var scanErr *scanner.Error
+	var badLines map[int]bool
+
+	if errors.As(err, &scanErrs) {
+		errTexts = make([]string, len(scanErrs))
+		badLines = make(map[int]bool, len(scanErrs))
+		for i, scanErr := range scanErrs {
+			errTexts[i] = err.Error()
+			badLines[scanErr.Pos.Line] = true
+		}
+	} else if errors.As(err, &scanErr) {
+		errTexts = []string{scanErr.Error()}
+		badLines = map[int]bool{scanErr.Pos.Line: true}
+	} else {
+		errTexts = []string{err.Error()}
+	}
+
+	lines := bytes.SplitAfter(source, []byte("\n"))
+	lineNoWidth := int(math.Ceil(math.Log10(float64(len(lines) + 1))))
+	for i, line := range lines {
+		prefix := "  "
+		if badLines[i] {
+			prefix = "> "
+		}
+		lineNo := strconv.Itoa(i + 1)
+		padding := strings.Repeat(" ", lineNoWidth-len(lineNo))
+		lines[i] = []byte(fmt.Sprintf("%s%s%s | %s", prefix, padding, lineNo, line))
+	}
+
+	return errorf(nil,
+		"genqlient internal error: failed to %s code:\n\t%s---source code---\n%s",
+		failedOperation, strings.Join(errTexts, "\n\t"), bytes.Join(lines, nil))
+}

generate/generate.go

@@ -382,13 +382,11 @@ func Generate(config *Config) (map[string][]byte, error) {
 	unformatted := buf.Bytes()
 	formatted, err := format.Source(unformatted)
 	if err != nil {
-		return nil, errorf(nil, "could not gofmt code: %v\n---unformatted code---\n%v",
-			err, string(unformatted))
+		return nil, goSourceError("gofmt", unformatted, err)
 	}
 	importsed, err := imports.Process(config.Generated, formatted, nil)
 	if err != nil {
-		return nil, errorf(nil, "could not goimports code: %v\n---unimportsed code---\n%v",
-			err, string(formatted))
+		return nil, goSourceError("goimports", formatted, err)
 	}
 
 	retval := map[string][]byte{