Improve package-sniffing and bind correctly to types in the same pack…

…age (#316)

The original motivation here is to fix #283, which is that if you try to
use `bindings` to bind to a type in the same package as the generated
code, we generate a self-import, which Go doesn't allow. Fixing that is
easy -- the three lines in `imports.go` -- once you know the
package-path of the generated code. (The test that that all fits
together is in integration-tests because that was the easiest place to
set up the right situation.)

Determining the package-path is not too much harder: you ask
`go/packages`, which we already use for `package_bindings`. But once
we're doing that, and handling errors, it's kinda silly that we ask you
to specify the package your generated code will use, because we have
that too, usually. So I rewrote that handling too, making `package` now
rarely necessary (see for example the `example` config), and warning if
it looks wrong. This is the changes in `config.go`, and is the more
substantial non-test change. (I also renamed some of the testdata dirs
to be valid package-names, to exercise more of that code, or in the case
of `find-config`, just for consistency.)

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable (and checked that
the test for the bugfix fails without the rest of the changes)
- [x] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features
- [x] Added ~an entry~ two entries to the changelog

docs/CHANGELOG.md

@@ -31,11 +31,13 @@ Note that genqlient is now tested from Go 1.20 through Go 1.22.
 - The new `optional: generic` allows using a generic type to represent optionality. See the [documentation](genqlient.yaml) for details.
 - For schemas with enum values that differ only in casing, it's now possible to disable smart-casing in genqlient.yaml; see the [documentation](genqlient.yaml) for `casing` for details.
 - Support .graphqls and .gql file extensions
+- More accurately guess the package name for generated code (and warn if the config option -- now almost never needed -- looks wrong).
 
 ### Bug fixes:
 - The presence of negative pointer directives, i.e., `# @genqlient(pointer: false)` are now respected even in the when `optional: pointer` is set in the configuration file.
 - Made name collisions between query/mutation arguments and local function variables less likely.
 - Fix generation issue related to golang type implementation of complex graphql union fragments
+- Bind correctly to types in the same package as the generated code.
 
 ## v0.6.0
 

docs/genqlient.yaml

@@ -29,8 +29,12 @@ operations:
 # genqlient.yaml. Default: generated.go.
 generated: generated/genqlient.go
 
-# The package name for the output code; defaults to the directory name of
-# the generated-code file.
+# The package name for the output code; defaults to the package-name
+# corresponding to the setting of `generated`, above.
+#
+# This is rarely needed: only if you want the package-name to differ from the
+# suffix of the package-path, and there are no other Go files in the package
+# already.
 package: mygenerated
 
 # If set, a file at this path (relative to genqlient.yaml) will be generated
@@ -139,6 +143,9 @@ optional_generic_type: github.com/organisation/repository/example.Type
 # guarantees that the fields requested in the query match those present in
 # the Go type.
 #
+# Note: if binding to types in the same package as the generated code, make
+# sure you don't bind to generated types! Otherwise, things get very circular.
+#
 # To get equivalent behavior in just one query, use @genqlient(bind: ...);
 # see genqlient_directive.graphql for more details.
 bindings:
@@ -224,6 +231,9 @@ bindings:
 # to the bindings map, above, for each exported type in the package.  Multiple
 # packages may be specified, and later ones take precedence over earlier ones.
 # Explicit entries in bindings take precedence over all package bindings.
+#
+# Note: make sure this isn't the package with your generated code, or things
+# will get circular very fast.
 package_bindings:
 - package: github.com/you/yourpkg/models
 

example/genqlient.yaml

@@ -2,8 +2,6 @@ schema: schema.graphql
 operations:
 - genqlient.graphql
 generated: generated.go
-# needed since it doesn't match the directory name:
-package: main
 
 # We bind github's DateTime scalar type to Go's time.Time (which conveniently
 # already defines MarshalJSON and UnmarshalJSON).  This means genqlient will

generate/config.go

@@ -47,6 +47,8 @@ type Config struct {
 	// The directory of the config-file (relative to which all the other paths
 	// are resolved).  Set by ValidateAndFillDefaults.
 	baseDir string
+	// The package-path into which we are generating.
+	pkgPath string
 }
 
 // A TypeBinding represents a Go type to which genqlient will bind a particular
@@ -132,6 +134,52 @@ func pathJoin(a, b string) string {
 	return filepath.Join(a, b)
 }
 
+// Try to figure out the package-name and package-path of the given .go file.
+//
+// Returns a best-guess pkgName if possible, even on error.
+func getPackageNameAndPath(filename string) (pkgName, pkgPath string, err error) {
+	abs, err := filepath.Abs(filename)
+	if err != nil { // path is totally bogus
+		return "", "", err
+	}
+
+	dir := filepath.Dir(abs)
+	// If we don't get a clean answer from go/packages, we'll use the
+	// directory-name as a backup guess, as long as it's a valid identifier.
+	pkgNameGuess := filepath.Base(dir)
+	if !token.IsIdentifier(pkgNameGuess) {
+		pkgNameGuess = ""
+	}
+
+	pkgs, err := packages.Load(&packages.Config{Mode: packages.NeedName}, dir)
+	if err != nil { // e.g. not in a Go module
+		return pkgNameGuess, "", err
+	} else if len(pkgs) != 1 { // probably never happens?
+		return pkgNameGuess, "", fmt.Errorf("found %v packages in %v, expected 1", len(pkgs), dir)
+	}
+
+	pkg := pkgs[0]
+	// TODO(benkraft): Can PkgPath ever be empty while in a module? If so, we
+	// could warn.
+	if pkg.Name != "" { // found a good package!
+		return pkg.Name, pkg.PkgPath, nil
+	}
+
+	// Package path is valid, but name is empty: probably an empty package
+	// (within a valid module). If the package-path-suffix is a valid
+	// identifier, that's a better guess than the directory-suffix, so use it.
+	pathSuffix := filepath.Base(pkg.PkgPath)
+	if token.IsIdentifier(pathSuffix) {
+		pkgNameGuess = pathSuffix
+	}
+
+	if pkgNameGuess != "" {
+		return pkgNameGuess, pkg.PkgPath, nil
+	} else {
+		return "", "", fmt.Errorf("no package found in %v", dir)
+	}
+}
+
 // ValidateAndFillDefaults ensures that the configuration is valid, and fills
 // in any options that were unspecified.
 //
@@ -167,29 +215,40 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {
 			"\nExample: \"github.com/Org/Repo/optional.Value\"")
 	}
 
-	if c.Package != "" {
-		if !token.IsIdentifier(c.Package) {
-			// No need for link here -- if you're already setting the package
-			// you know where to set the package.
-			return errorf(nil, "invalid package in genqlient.yaml: '%v' is not a valid identifier", c.Package)
-		}
-	} else {
-		abs, err := filepath.Abs(c.Generated)
-		if err != nil {
+	if c.Package != "" && !token.IsIdentifier(c.Package) {
+		// No need for link here -- if you're already setting the package
+		// you know where to set the package.
+		return errorf(nil, "invalid package in genqlient.yaml: '%v' is not a valid identifier", c.Package)
+	}
+
+	pkgName, pkgPath, err := getPackageNameAndPath(c.Generated)
+	if err != nil {
+		// Try to guess a name anyway (or use one you specified) -- pkgPath
+		// isn't always needed. (But you'll run into trouble binding against
+		// the generated package, so at least warn.)
+		if c.Package != "" {
+			warn(errorf(nil, "warning: unable to identify current package-path "+
+				"(using 'package' config '%v'): %v\n", c.Package, err))
+		} else if pkgName != "" {
+			warn(errorf(nil, "warning: unable to identify current package-path "+
+				"(using directory name '%v': %v\n", pkgName, err))
+			c.Package = pkgName
+		} else {
 			return errorf(nil, "unable to guess package-name: %v"+
 				"\nSet package name in genqlient.yaml"+
 				"\nExample: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6", err)
 		}
-
-		base := filepath.Base(filepath.Dir(abs))
-		if !token.IsIdentifier(base) {
-			return errorf(nil, "unable to guess package-name: '%v' is not a valid identifier"+
-				"\nSet package name in genqlient.yaml"+
-				"\nExample: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6", base)
+	} else { // err == nil
+		if c.Package == pkgName || c.Package == "" {
+			c.Package = pkgName
+		} else {
+			warn(errorf(nil, "warning: package setting in genqlient.yaml '%v' looks wrong "+
+				"('%v' is in package '%v') but proceeding with '%v' anyway\n",
+				c.Package, c.Generated, pkgName, c.Package))
 		}
-
-		c.Package = base
 	}
+	// This is a no-op in some of the error cases, but it still doesn't hurt.
+	c.pkgPath = pkgPath
 
 	if len(c.PackageBindings) > 0 {
 		for _, binding := range c.PackageBindings {
@@ -201,6 +260,11 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {
 					binding.Package)
 			}
 
+			if binding.Package == c.pkgPath {
+				warn(errorf(nil, "warning: package_bindings set to the same package as your generated "+
+					"code ('%v'); this may cause nondeterministic output due to circularity", c.pkgPath))
+			}
+
 			mode := packages.NeedDeps | packages.NeedTypes
 			pkgs, err := packages.Load(&packages.Config{
 				Mode: mode,

generate/config_test.go

@@ -11,9 +11,9 @@ import (
 )
 
 const (
-	findConfigDir    = "testdata/find-config"
-	validConfigDir   = "testdata/valid-config"
-	invalidConfigDir = "testdata/invalid-config"
+	findConfigDir    = "testdata/findConfig"
+	validConfigDir   = "testdata/validConfig"
+	invalidConfigDir = "testdata/invalidConfig"
 )
 
 func TestFindCfg(t *testing.T) {

generate/imports.go

@@ -99,6 +99,9 @@ func (g *generator) ref(fullyQualifiedName string) (qualifiedName string, err er
 
 	pkgPath := nameToImport[:i]
 	localName := nameToImport[i+1:]
+	if pkgPath == g.Config.pkgPath {
+		return prefix + localName, nil
+	}
 	alias, ok := g.imports[pkgPath]
 	if !ok {
 		if g.importsLocked {

generate/main.go

@@ -14,6 +14,11 @@ import (
 	"github.com/alexflint/go-arg"
 )
 
+// TODO(benkraft): Make this mockable for tests?
+func warn(err error) {
+	fmt.Println(err)
+}
+
 func readConfigGenerateAndWrite(configFilename string) error {
 	var config *Config
 	var err error

generate/testdata/snapshots/TestInvalidConfigs-InvalidCasing.yaml

@@ -1 +1 @@
-invalid config file testdata/invalid-config/InvalidCasing.yaml: unknown casing algorithm: bogus
+invalid config file testdata/invalidConfig/InvalidCasing.yaml: unknown casing algorithm: bogus

generate/testdata/snapshots/TestInvalidConfigs-InvalidOptional.yaml

@@ -1 +1 @@
-invalid config file testdata/invalid-config/InvalidOptional.yaml: optional must be one of: 'value' (default), 'pointer', or 'generic'
+invalid config file testdata/invalidConfig/InvalidOptional.yaml: optional must be one of: 'value' (default), 'pointer', or 'generic'

generate/testdata/snapshots/TestInvalidConfigs-InvalidPackage.yaml

@@ -1 +1 @@
-invalid config file testdata/invalid-config/InvalidPackage.yaml: invalid package in genqlient.yaml: 'bogus-package-name' is not a valid identifier
+invalid config file testdata/invalidConfig/InvalidPackage.yaml: invalid package in genqlient.yaml: 'bogus-package-name' is not a valid identifier

generate/testdata/snapshots/TestValidConfigs-Simple.yaml → generate/testdata/snapshots/TestValidConfigs-Empty.yml

@@ -1,7 +1,7 @@
 (*generate.Config)({
   Schema: (generate.StringList) <nil>,
   Operations: (generate.StringList) <nil>,
-  Generated: (string) (len=34) "testdata/valid-config/generated.go",
+  Generated: (string) (len=33) "testdata/validConfig/generated.go",
   Package: (string) (len=11) "validConfig",
   ExportOperations: (string) "",
   ContextType: (string) (len=15) "context.Context",
@@ -17,5 +17,6 @@
   StructReferences: (bool) false,
   Extensions: (bool) false,
   AllowBrokenFeatures: (bool) false,
-  baseDir: (string) (len=21) "testdata/valid-config"
+  baseDir: (string) (len=20) "testdata/validConfig",
+  pkgPath: (string) (len=55) "github.com/Khan/genqlient/generate/testdata/validConfig"
 })

generate/testdata/snapshots/TestValidConfigs-Lists.yaml

@@ -1,13 +1,13 @@
 (*generate.Config)({
   Schema: (generate.StringList) (len=2) {
-    (string) (len=42) "testdata/valid-config/first_schema.graphql",
-    (string) (len=43) "testdata/valid-config/second_schema.graphql"
+    (string) (len=41) "testdata/validConfig/first_schema.graphql",
+    (string) (len=42) "testdata/validConfig/second_schema.graphql"
   },
   Operations: (generate.StringList) (len=2) {
-    (string) (len=46) "testdata/valid-config/first_operations.graphql",
-    (string) (len=47) "testdata/valid-config/second_operations.graphql"
+    (string) (len=45) "testdata/validConfig/first_operations.graphql",
+    (string) (len=46) "testdata/validConfig/second_operations.graphql"
   },
-  Generated: (string) (len=34) "testdata/valid-config/generated.go",
+  Generated: (string) (len=33) "testdata/validConfig/generated.go",
   Package: (string) (len=11) "validConfig",
   ExportOperations: (string) "",
   ContextType: (string) (len=15) "context.Context",
@@ -23,5 +23,6 @@
   StructReferences: (bool) false,
   Extensions: (bool) false,
   AllowBrokenFeatures: (bool) false,
-  baseDir: (string) (len=21) "testdata/valid-config"
+  baseDir: (string) (len=20) "testdata/validConfig",
+  pkgPath: (string) (len=55) "github.com/Khan/genqlient/generate/testdata/validConfig"
 })

generate/testdata/snapshots/TestValidConfigs-Strings.yaml

@@ -1,11 +1,11 @@
 (*generate.Config)({
   Schema: (generate.StringList) (len=1) {
-    (string) (len=36) "testdata/valid-config/schema.graphql"
+    (string) (len=35) "testdata/validConfig/schema.graphql"
   },
   Operations: (generate.StringList) (len=1) {
-    (string) (len=40) "testdata/valid-config/operations.graphql"
+    (string) (len=39) "testdata/validConfig/operations.graphql"
   },
-  Generated: (string) (len=34) "testdata/valid-config/generated.go",
+  Generated: (string) (len=33) "testdata/validConfig/generated.go",
   Package: (string) (len=11) "validConfig",
   ExportOperations: (string) "",
   ContextType: (string) (len=15) "context.Context",
@@ -21,5 +21,6 @@
   StructReferences: (bool) false,
   Extensions: (bool) false,
   AllowBrokenFeatures: (bool) false,
-  baseDir: (string) (len=21) "testdata/valid-config"
+  baseDir: (string) (len=20) "testdata/validConfig",
+  pkgPath: (string) (len=55) "github.com/Khan/genqlient/generate/testdata/validConfig"
 })

generate/testdata/valid-config/Lists.yaml → generate/testdata/validConfig/Lists.yaml

@@ -5,5 +5,3 @@ schema:
 operations:
 - first_operations.graphql
 - second_operations.graphql
-
-package: validConfig

generate/testdata/valid-config/Strings.yaml → generate/testdata/validConfig/Strings.yaml

@@ -1,3 +1,2 @@
 schema: schema.graphql
 operations: operations.graphql
-package: validConfig

internal/integration/genqlient.yaml

@@ -8,3 +8,5 @@ bindings:
     type: time.Time
     marshaler: "github.com/Khan/genqlient/internal/testutil.MarshalDate"
     unmarshaler: "github.com/Khan/genqlient/internal/testutil.UnmarshalDate"
+  MyGreatScalar:
+    type: github.com/Khan/genqlient/internal/integration.MyGreatScalar

internal/integration/integration_test.go

@@ -20,7 +20,7 @@ import (
 
 func TestSimpleQuery(t *testing.T) {
 	_ = `# @genqlient
-	query simpleQuery { me { id name luckyNumber } }`
+	query simpleQuery { me { id name luckyNumber greatScalar } }`
 
 	ctx := context.Background()
 	server := server.RunServer()

internal/integration/schema.graphql

@@ -1,4 +1,5 @@
 scalar Date
+scalar MyGreatScalar
 
 type Query {
   me: User
@@ -23,6 +24,7 @@ type User implements Being & Lucky {
   hair: Hair
   birthdate: Date
   friends: [User!]!
+  greatScalar: MyGreatScalar
 }
 
 input NewUser {