-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
doc: require auto-generated DO NOT EDIT comment at top of file #41196
Comments
I think requiring the "// Code generated ... DO NOT EDIT." comment to be near the top of the file rather than anywhere, if it can be done, is a change that would benefit both humans and machines reading .go files. I agree with the bullet points under rationale. It also seems to me that this change wouldn't be too disruptive or too late, and hopefully people can chime in if they believe otherwise. There are not that many generators/generated files that'll need updating, and many will be "fixed" by the change anyway. There are likely more projects that haven't been updated to use the existing https://golang.org/s/generatedcode convention yet (either because they didn't get around to it yet, or they don't have a need for it), so they can just target the latest version of the convention when they get around to it. I have a minor clarification question to understand an edge case around the proposed change better:
How are whitespace characters meant to be treated? Such characters would be an unlikely situation, but I'm hoping the proposed convention is clear about it. It can happen primarily in non-gofmted files (e.g., a stray trailing space in a generated file, or someone evaluating all edge cases of this proposal). For example:
I tried reading the placement section in the
It seems like whitespace should probably be allowed too, but that text doesn't include it explicitly. Is it included implicitly or not at all? |
Spaces and tabs before the magic line don't matter. |
I think many people (myself included) likely believed this to already be the case (which is why so many generators make that mistake). Here's a question: under the current system, if your generator was based off of a |
I understand that the convention defined as a result of proposal #13560 was meant primarily for .go source files, so a This is a good point to consider and potentially clarify. It's likely reasonable to use the same convention for non-.go source files that are supported by the Go build system, but from what I can see, that was not really discussed (other than in #13560 (comment)) or considered in scope of #13560. See https://golang.org/cmd/go/#hdr-File_types, which currently says:
I don't think it's viable for the Go project to define a convention to mark generated files of any arbitrary type. For example, some programming languages may not support line comments, so requiring a line that begins with "//" cannot work everywhere. |
I think the most likely problem scenario would be someone using a text editor mode made for go files on a |
It's true that the primary focus for these comments is Go code and more generally Go package sources. <Imagine snarky comment about "DO NOT EDIT" being implied by program being written in Fortran.> |
This comment has been minimized.
This comment has been minimized.
Based on the discussion above, this seems like a likely accept. |
No change in consensus, so accepted |
Should this be closed? |
@cristaloleg No. The proposal has been accepted, but it has not been implemented. At least, not as far as I know. For example, https://golang.org/s/generatedcode needs to point to the updated rule. |
I updated golang.org/s/generatedcode to point to the relevant cmd/go docs. |
Change https://golang.org/cl/283633 mentions this issue: |
Issue #13560 defined the exact form for a comment marking a file as auto-generated.
As linked at https://golang.org/s/generatedcode and then repeated at
https://golang.org/cmd/go/#hdr-Generate_Go_files_by_processing_source,
the rule is:
@robpike's original suggestion for placement was to match the rule for
// +build
:[Narrator: Good idea; I am going to suggest matching the rule for
//go:build
below.]After some bikeshedding about other comments it changed to:
@bradfitz then raised the question of non-Go source files saying:
@myitcv raised a question about Go files with syntax errors and suggested anywhere in the file is fine, comparing to
go generate
.@robpike then revised to the “text may appear anywhere in the file” rule, which is what was finally accepted.
I was writing a generator the other day and @dmitshur helpfully pointed out that I'd accidentally marked the generator itself as generated by writing this code in the generator:
If the rule we've defined flags this program as auto-generated, because it is itself a generator, that seems to me more like a bug in the rule than a bug in the generator.
I propose we change the rule to match the new
//go:build
rules (#41184).Specifically, the comment must appear before the first non-blank line, non-comment text.
/* */
comments before the// Code generated ...
comment would be allowed.The rationale is:
//go:build
one).//go:build
rule applies equally well to Go and non-Go files, addressing @bradfitz's concern.//go:build
rule avoids any problem with syntax errors, because the lines must be before any syntax, addressing @myitcv's concern.//go:generate
does not really apply. The lax placement there is because we want to allow putting the//go:generate
line next to what it applies to, such as putting a//go:generate stringer
line above the type that is being stringered. In this case, the// Code generated ...
applies to the entire file. It should be above the file.It's reasonable to ask: Isn't it too late to change this? What about generators that put the comment later in the file? Their output won't be recognized as generated anymore.
I looked into that using the module mirror-based Go corpus I assembled back in March. Of all the Go source files I have in that corpus, I found:
// Code generated
comment.So making this change would require 0.825% of generated files to be updated. And until they are updated, no big deal - people will still see the comment, and only a few tools care. If we fix the top five generators causing these lines (xo, cdproto-gen, minimock, msgp, chromedp-gen), we'd be left with only 1,008 mismatched files, or 0.15%. In any event, when we first adopted the rule we had to update essentially 100% of generated files. Now we're talking about under 1/100 of that, so the impact here is not large.
On the other hand, consider generators. I had the same scan look for the magic text inside string literals. It found 2,272 instances of string literals containing the text but not at the start/end of a line; those are correctly skipped by the current rule. It also found 2,350 instances of multiline string literals like the one I'd written; all those generators are incorrectly flagged as themselves auto-generated by the current rule.
That is, just over half of the generators I found are doing it wrong by the current rule.
This strongly suggests the rule should be changed.
I've attached the non-top-of-file results as autogen.txt if anyone wants to look into the details here.
autogen.txt
The text was updated successfully, but these errors were encountered: