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/vet: warn about "// go:" comments (with space) #43698

Open
bcmills opened this issue Jan 14, 2021 · 16 comments
Open

cmd/vet: warn about "// go:" comments (with space) #43698

bcmills opened this issue Jan 14, 2021 · 16 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 14, 2021

The embed.FS type added for #41191 documents the following behavior:

An FS is a read-only collection of files, usually initialized with a //go:embed directive. When declared without a //go:embed directive, an FS is an empty file system.

I expect that package authors essentially never intend to end up with an empty read-only filesystem in a production package. (In tests, perhaps, but not otherwise.) For the few cases where they do, they should be able to add an explicit //go:embed directive indicating that the filesystem is intentionally empty.

Otherwise, it is too easy for an unintentional typo in the //go:embed directive, such as the one discovered in #43682 (comment), to result in a difficult-to-debug failure at run time.

I propose that cmd/vet should emit a warning when:

  • a package-level variable is declared with type embed.FS,
  • without any corresponding //go:embed directive (not even one with an empty pattern list),
  • in a non-test source file.
@gopherbot gopherbot added this to the Proposal milestone Jan 14, 2021
@bcmills bcmills changed the title proposal: cmd/vet: warn about package variables of type embed.FS that lack go:embed directives proposal: cmd/vet: warn about embed.FS package variables that lack go:embed directives Jan 14, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 14, 2021

Change https://golang.org/cl/283852 mentions this issue: embed: treat uninitialized FS as empty

Loading

@mvdan
Copy link
Member

@mvdan mvdan commented Jan 14, 2021

If this never makes sense, why not a build error?

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Jan 14, 2021

in a non-test source file.

Do we expect this restriction is necessary? Proposal restricts this to package level variables. The current draft of https://golang.org/cl/283852 does have an embed.FS at this level. Do we expect anyone else would have one intentionally?

Loading

@bcmills
Copy link
Member Author

@bcmills bcmills commented Jan 14, 2021

If this never makes sense, why not a build error?

I'd be ok with that too, but I suspect that a vet warning is an easier sell. 😅

Loading

@bcmills
Copy link
Member Author

@bcmills bcmills commented Jan 14, 2021

Do we expect this restriction is necessary?

I expect that it will catch a non-trivial number of real errors with few or no false-positives. I believe that meets the bar for a vet check.

Proposal restricts this to package level variables. The current draft of https://golang.org/cl/283852 does have an embed.FS at this level. Do we expect anyone else would have one intentionally?

Note that (a) the package-level variable in that CL is unnecessary (it could be a local variable instead), and (b) it would not trigger the proposed check anyway, because that variable is in a test source file.

Loading

@bcmills
Copy link
Member Author

@bcmills bcmills commented Jan 14, 2021

@timothy-king, Oh, I think I see what you're saying now. “Would it be ok to warn for package-level embed.FS variables in test source files?”

I don't actually know. I suspect that we could warn for those cases too..?

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Jan 14, 2021

I expect that it will catch a non-trivial number of real errors with few or no false-positives. I believe that meets the bar for a vet check.

Agreed. I also think the cost of a false-positive is small. I believe var global *embed.FS=&embed.FS{} would be a decent workaround if you absolutely had to have an initially empty package level embed.FS.

@timothy-king, Oh, I think I see what you're saying now. “Would it be ok to warn for package-level embed.FS variables in test source files?”

Yep that was my question.

I don't actually know. I suspect that we could warn for those cases too..?

I also suspect warning on tests would be okay. Maybe I am not being clever enough, but I can't think of a good use case for test files either.

Loading

gopherbot pushed a commit that referenced this issue Jan 19, 2021
As described in the FS documentation.

This prevents http.FS and other clients from panicking when the
go:embed directive is missing.

For #43682
Related #43698

Change-Id: Iecf26d229a099e55d24670c3119cd6c6d17ecc6e
Reviewed-on: https://go-review.googlesource.com/c/go/+/283852
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@rsc
Copy link
Contributor

@rsc rsc commented Feb 10, 2021

The spec in the adopted proposal was very clear that declaring an embed.FS with no //go:embed line is valid.
If we believe it is valid, then vet should not "warn" about it.
We don't warn about non-errors.
As implemented today, embed.FS are copyable, so that you can do things like

var fs embed.FS
if cond {
   fs = version1files
} else {
   fs = version2files
}

You don't want vet to complain about fs in that code.

Loading

@rsc rsc added this to Active in Proposals Feb 10, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Feb 10, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Feb 10, 2021

@rsc The proposal is only to warn on package level variables. I think fs would be function scope in your example. But you make a really good point about the proposal:

Similarly, an embed.Files variable can be a global or a local variable, depending on what is more convenient in context.

It is not an error to declare an embed.Files without a //go:embed directive. That variable simply contains no embedded files.

That is saying fairly definitively that a strict reading of this proposal that it will warn on non-errors.

It does seem like there should be something we can do to help in cases like #43682 though. What about instead just warning when there is no //go:embed and there is also a likely misspelling like "// go:embed" (with a space)?

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Feb 17, 2021

It seems fine to develop a vet checker that warns about "// go:" lines where "//go:" would have a meaning instead.
For example "// go:embed" right before an embed.FS or []byte or string declaration,
or "// go:noinline" right before a function.
We'd want to check for false positives in existing open-source Go code though.
Should we repurpose this issue for that, @bcmills?

Loading

@bcmills
Copy link
Member Author

@bcmills bcmills commented Feb 17, 2021

Yep, that seems ok by me. A // go: check would probably help with other directives too.

Loading

@rsc rsc changed the title proposal: cmd/vet: warn about embed.FS package variables that lack go:embed directives proposal: cmd/vet: warn about "// go:" comments (with space) Feb 24, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Feb 24, 2021

It might be worth looking at the buildtag analysis pass, which may already do this for //go:build.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Feb 24, 2021

Does anyone object to the retitled proposal (a vet check to warn about "// go:foo" where "//go:foo" has a meaning)?

Loading

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 10, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Mar 10, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Mar 24, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Loading

@rsc rsc changed the title proposal: cmd/vet: warn about "// go:" comments (with space) cmd/vet: warn about "// go:" comments (with space) Mar 24, 2021
@rsc rsc removed this from the Proposal milestone Mar 24, 2021
@rsc rsc added this to the Backlog milestone Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
@rsc @timothy-king @mvdan @bcmills @gopherbot and others