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

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

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?

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

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.

@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..?

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.

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.

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

@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)?

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?

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

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

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

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

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

Change https://golang.org/cl/381914 mentions this issue: x/tools/go/analysis/passes: add pragmyzer to flag invalid pragmas

I've mailed out CL https://go-review.googlesource.com/c/tools/+/381914 which fixes the problem and is simple.