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

benjaminjkraft · 2024-02-18T02:15:19Z

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:

Written a clear PR title and description (above)
Signed the Khan Academy CLA
Added tests covering my changes, if applicable (and checked that the test for the bugfix fails without the rest of the changes)
Included a link to the issue fixed, if applicable
Included documentation, for new features
Added ~~an entry~~ two entries to the changelog

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.)

…rated

csilvers

This looks good to me! I like figuring out the package name automatically; that makes a lot of sense to me. Some style comments below but nothing requiring another go-around.

csilvers · 2024-02-18T21:38:15Z

generate/config.go

+	pkgNameGuess := filepath.Base(dir)
+	if !token.IsIdentifier(pkgNameGuess) {
+		pkgNameGuess = ""
+	}


It seems like this could profitably be later down in the function, maybe lines 166- could be something like:

packageNameGuess := "" if token.IsIdentifier(filepath.Base(pkg.PkgPath)) { pkgNameGuess = filepath.Base(pkg.PkgPath) } else if token.IsIdentifier(filepath.Base(dir)) { pkgNameGuess = filepath.Base(dir) }

But I don't really understand if/how that else case could fire. Maybe add some documentation here.

csilvers · 2024-02-18T21:39:14Z

generate/config.go

+	// TODO(benkraft): Can PkgPath ever be empty without error? If so, we could
+	// warn.


Yeah, I don't know the semantics of packages.Load, but it seems like Name is empty if the package doesn't exist, but PkgPath is still set in that case?

csilvers · 2024-02-18T21:39:47Z

generate/config.go

+		return pkg.Name, pkg.PkgPath, nil
+	}
+
+	// e.g. empty package yet to be created, see if we can just guess a


Thanks, this comment is very helpful!

csilvers · 2024-02-18T21:45:00Z

generate/config.go

-				"\nSet package name in genqlient.yaml"+
-				"\nExample: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6", base)
+	pkgName, pkgPath, err := c.getPackageNameAndPath()
+	if err == nil {


It took me a while to figure out what was going on here, because I'm so used to the Go convention of checking for the error case in the if's, not the success case.

I wonder if there's a way to reorganize this code to keep the current semantics but perhaps adhere more closely to Go convention. Maybe not.

csilvers · 2024-02-18T21:46:44Z

generate/config.go

+		// binding against the generated package, so at least warn.
+		warn(errorf(nil, "warning: unable to identify current package-path "+
+			"(using 'package' config '%v'): %v\n", c.Package, err))
+	} else if pkgName != "" {


It's unusual, though I guess not unheard of, for the value of the other return-values to matter when err is not nil. It may be worth emphasizing this in the relevant parts of getPackageNameAndPath, though.

csilvers · 2024-02-18T21:47:34Z

generate/config.go

-			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)
+	pkgName, pkgPath, err := c.getPackageNameAndPath()


You may want to comment that this ignores c.Package, and that below you'll check to make sure they're in agreement (or, at least, not in disagreement.)

csilvers · 2024-02-18T23:11:35Z

generate/config.go

@@ -201,6 +256,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 will probably cause circularity issues", c.pkgPath))


"probably cause circular-import issues"?

benjaminjkraft added 3 commits February 17, 2024 18:10

lint

6549706

Merge remote-tracking branch 'origin/main' into benkraft.bind-to-gene…

852b48c

…rated

benjaminjkraft requested review from csilvers, dnerdy and StevenACoffman as code owners February 18, 2024 02:15

This was referenced Feb 18, 2024

feat: allow imports from same generated package #289

Closed

feat: support binding to generated package #285

Closed

benjaminjkraft added 2 commits February 17, 2024 18:24

warn against self-binding circularity

a9dba06

reword

5190f51

csilvers approved these changes Feb 18, 2024

View reviewed changes

review comments, mostly documentation and a little refactoring

83d2bab

benjaminjkraft merged commit 5bdd7fd into main Feb 19, 2024
10 checks passed

benjaminjkraft deleted the benkraft.bind-to-generated branch February 19, 2024 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benjaminjkraft commented Feb 18, 2024

csilvers left a comment

csilvers Feb 18, 2024

csilvers Feb 18, 2024

csilvers Feb 18, 2024

csilvers Feb 18, 2024

csilvers Feb 18, 2024

csilvers Feb 18, 2024

csilvers Feb 18, 2024

		// TODO(benkraft): Can PkgPath ever be empty without error? If so, we could
		// warn.

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

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

Conversation

benjaminjkraft commented Feb 18, 2024

csilvers left a comment

Choose a reason for hiding this comment

csilvers Feb 18, 2024

Choose a reason for hiding this comment

csilvers Feb 18, 2024

Choose a reason for hiding this comment

csilvers Feb 18, 2024

Choose a reason for hiding this comment

csilvers Feb 18, 2024

Choose a reason for hiding this comment

csilvers Feb 18, 2024

Choose a reason for hiding this comment

csilvers Feb 18, 2024

Choose a reason for hiding this comment

csilvers Feb 18, 2024

Choose a reason for hiding this comment