Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: generate shouldn't require a complete valid package #36422

Open
robpike opened this issue Jan 7, 2020 · 13 comments
Open

cmd/go: generate shouldn't require a complete valid package #36422

robpike opened this issue Jan 7, 2020 · 13 comments

Comments

@robpike
Copy link
Contributor

@robpike robpike commented Jan 7, 2020

When developing a "go generate" script, it's easy to create an invalid program as the output. When that happens, one can no longer run "go generate" because the package must typecheck before "go generate" is allowed to run. The result can be confusing, and I see no vital reason why this property must be true; moreover, sometimes it could be impossible to satisfy without hand-editing some files to get the package to the state where it could be run.

% go version
go version devel +0377f06168 Tue Dec 17 20:57:06 2019 +0000 darwin/amd64
potoroo=% ls
x.go
% cat x.go
package main

//go:generate sh -c "echo foo > gen.go"

func() main () {
}
% go generate
% go generate
can't load package: package .: 
gen.go:1:1: expected 'package', found foo
% ls
gen.go	x.go
% cat gen.go
foo
% 

Let's lift this requirement.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 7, 2020

I agree that there are many valid use cases for go generate that don't require a valid package. For example, //go:generate go run gen_foo.go which writes a foo.go file.

Other cases like //go:generate stringer -type Foo could fail at a later time, when the tool runs, but I think that's fine.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 7, 2020

the package must typecheck before "go generate" is allowed to run

Is this actually the case?

$ cat a.go
package a

blah
$ cat b.go
package a

$ cat c.go
// +build ignnore

package main

$ go generate

I think go generate only requires that the package clause can be parsed (along with build constraints).

Which I think makes sense because go generate sets GOPACKAGE when it runs?

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 7, 2020

Also related, I think: #36068.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 7, 2020

I think go generate only requires that the package clause can be parsed (along with build constraints).

Yes, we certainly need at least one of the source files to contain a valid package clause in order to set GOPACKAGE. But we only really need one such file: in theory we could skip over the invalid ones rather than erroring out.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 7, 2020

What about if you have conflicting package names? Which one is correct? I ask because no package clause could be considered to be a special case of that could it not?

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 7, 2020

@myitcv I guess you could go with heuristics in that case; choose one or more of:

  • if one of the package names matches the directory name, use that
  • ignore package names on files that have the // Code generated by comment
  • choose the package name used by the majority of files

If the above heuristics fail (e.g. you've got two files with package names unrelated to the directory,
both different, and neither one with the generated comment), then you could fall back to failing.

But I suspect those heuristics would catch almost all cases.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 7, 2020

The only reason I point that out is that go generate is a build command (takes build flags), and any inconsistency with other build commands might be confusing.

@zephyrtronium

This comment has been minimized.

Copy link

@zephyrtronium zephyrtronium commented Jan 7, 2020

Would it work to add a new flag for generate to specify the package and override parsing it from files?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 7, 2020

@zephyrtronium, we already have a way to specify the package name: add a package directive to the same source file that contains the go:generate directive.

Moreover, that seems like the clear tiebreaker, too: when go generate invokes a tool, it should set GOPACKAGE based on the file containing that specific go:generate comment.

@bcmills bcmills added this to the Backlog milestone Jan 7, 2020
@bcmills bcmills added the GoCommand label Jan 7, 2020
@reusee

This comment has been minimized.

Copy link

@reusee reusee commented Jan 8, 2020

Output files with negated build tag will be ignored by go generate with the same tag:

// +build !generating
...invalid output codes...
go generate -tags=generating

Therefore invalid codes will not break the next go generate.

@donovank

This comment has been minimized.

Copy link

@donovank donovank commented Jan 10, 2020

I believe skipping invalid files would work. The GOPACKAGE environment variable is set to the package of the source file with the generate directive already per every call to run.

The patch below adds error checking to the loop responsible for spawning run instances. A file without a proper package header will be in it's own 'package' and thus properly averted by the continue.

--- a/src/cmd/go/internal/generate/generate.go
+++ b/src/cmd/go/internal/generate/generate.go
@@ -168,7 +168,12 @@ func runGenerate(cmd *base.Command, args []string) {
 
 	// Even if the arguments are .go files, this loop suffices.
 	printed := false
-	for _, pkg := range load.Packages(args) {
+
+	for _, pkg := range load.PackagesAndErrors(args) {
+		if pkg.Error != nil {
+			continue
+		}
+
 		if modload.Enabled() && pkg.Module != nil && !pkg.Module.Main {
 			if !printed {
 				fmt.Fprintf(os.Stderr, "go: not generating in packages in dependency modules\n")
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Jan 13, 2020

Moreover, that seems like the clear tiebreaker, too: when go generate invokes a tool, it should set GOPACKAGE based on the file containing that specific go:generate comment.

What should happen to the value of GOPACKAGE environment variable if the //go:generate comment is above the package clause, and the package clause is malformed? For example:

$ cat p.go
//go:generate echo "generate output for package $GOPACKAGE"

package +bad

For reference, the current behavior is the following error:

$ go generate
can't load package: package example.com/p: 
p.go:3:9: expected 'IDENT', found '+'

Edit: Since //go:generate comments can appear anywhere in any file with the extension .go, it's not significant whether the comment is above or below a package clause. It's the same question regardless of where the comment is, as long the package clause is invalid or missing.

@donovank

This comment has been minimized.

Copy link

@donovank donovank commented Jan 14, 2020

@dmitshur The Go spec should be updated in relation to directives to avoid undefined behavior like this. This problem arises from Packages in src/cmd/go/internal/load/....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.