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

docs/CHANGELOG.md

@@ -29,11 +29,13 @@ Note that genqlient is now tested 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 {