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

Proposal: cmd/go: scan Go assembly files for go:generate directives #25737

Closed
mvdan opened this issue Jun 5, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@mvdan
Copy link
Member

commented Jun 5, 2018

go help generate shows:

Generate runs commands described by directives within existing files. Those commands can run any process but the intent is to create or update Go source files.
[...]
Go generate scans the file for directives, which are lines of the form,
//go:generate command argument...
[...]
Note that go generate does not parse the file, so lines that look like directives in comments or multiline strings will be treated as directives.

It seems clear that go generate only scans Go files by design, as can be seen by the first paragraph. However, it seems like the tool could support other source files, such as assembly files, just fine - since it reads lines, and doesn't parse Go source code.

In particular, I need this for x/tools/cmd/buildtags, a build tag line generator I'm trying to write to solve #25348. The idea is that the same files that need the build tags would contain go:generate files to generate them. Which works great for Go files, but not assembly files.

The alternatives are all a bit hacky - such as forcing the user to use one single go:generate line in a Go file per package, separate from the build tags, or to have every invocation of the generator generate all the files in its package.

I can't think of any downside to doing this. Since the line prefix contains the keyword go, I doubt this will cause false positives with other languages or within strings in those other files.

Another point in this proposal's favor is that we already scan all source files for build constraints, so there's already some precedent:

Constraints may appear in any kind of source file (not just Go), but they must appear near the top of the file, preceded only by blank lines and other line comments.

/cc @myitcv @josharian

@gopherbot gopherbot added this to the Proposal milestone Jun 5, 2018

@gopherbot gopherbot added the Proposal label Jun 5, 2018

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

So in a sentence, the proposal is to also look for “go:generate” comments in Go assembly files?

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2018

Yes. But also in all other source files that can be part of a Go package, which would include .c, .cc, .h, .f, and so on. We can restrict the addition to just assembly files if that's preferrable, although it would seem a bit arbitrary to me.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

I think it should be in Go files only, since the go:generate syntax is actually a form of Go comment. If you want to generate something in another file, check in a Go file called generate.go that contains nothing but a package clause and various go:generate commands.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

I think it should be in Go files only, since the go:generate syntax is actually a form of Go comment.

True, although to push back a bit, Go assembly is Go-specific, and the comment form is identical. (I'm personally a bit less clear about .h, .c, .f, etc., since they are "foreign" sources.)

And part of the reason that go:generate comments may be located anywhere is that the go:generate comment can be placed somewhere semantically appropriate, which seems just as relevant to assembly as Go.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

I'm not opposed to looking at .s files, but I'd draw the line there.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2018

I think it should be in Go files only, since the go:generate syntax is actually a form of Go comment.

The // +build lines are parsed in all source files, including C and other languages. It seems to me like //go:generate lines fall under the same category.

The only difference is that // +build must be preceded only by other comments and empty lines. We can add the same restriction to generate directives oustside of Go source files, if that makes the proposal more consistent and conservative.

I'm not opposed to looking at .s files, but I'd draw the line there.

To give some numbers for build constraint comments in this git repository:

$ git grep -l '^// +build' | sed 's/.*\././' | sort | uniq -c
     27 .c
   1044 .go
      2 .h
      2 .html
     10 .pl
    142 .s
      3 .S

On one hand, yes, only adding support for assembly files would be a step forward. On the other, it wouldn't solve my issue completely.

If you want to generate something in another file, check in a Go file called generate.go that contains nothing but a package clause and various go:generate commands.

Like Josh said, it's all about keeping the directives in the right places. That makes them much easier to write, find, and maintain. If one is forced to put a single generate directive per package that uses this generator, some problems arise, such as:

  • Did you start using buildtags comments, but forgot to add its go:generate line?
  • Did you remove all buildtags comments, but forgot to remove the now unnecessary go:generate line in a separate file?
  • Does your package not have a package-wide file like generate.go or doc.go? Then you need to add the extra file.

I'll grant that none of these are dealbreakers, but they do make go generate unnecessarily complex to use in my opinion.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

I've changed my mind about the generator after all, so I'm going to use separate generate directives. So, as far as I am concerned, I'm happy if this proposal only covers assembly files. Extending the rules to apply to other source files can be done as another proposal building on this one.

I'm going to edit the title, but I'm going to leave the original post untouched, to leave the discussion untouched.

@mvdan mvdan changed the title Proposal: cmd/go: scan all source files for go:generate directives Proposal: cmd/go: scan Go assembly files for go:generate directives Jun 11, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

Especially given the last comment, this doesn't seem like a necessary feature. While occasionally it might be nice to put a go:generate comment in an assembler file, they can always be put in a corresponding Go file, and it doesn't seem like a common problem in any case. Supporting go:generate comments in assembler file invites the question of why they are not supported in other files. Supporting them only in Go files is easy to explain.

Proposal declined -- iant for @golang/proposal-review

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