proposal: cmd/vet: warn about // go:...

[odeke-em]

What version of Go are you using (go version)?

$ go version
go version devel go1.18-35b0db7607 Fri Jan 21 21:58:14 2022 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Irrelevant

What did you do?

From some internal code at Orijtech Inc that caused unexpected problems after runtime and deployment; it used an embed pragma statement to embed a file but unfortunately we had a space so // go:embed file.txt instead of //go:embed file.txt, the code compiled alright and was deployed but caused a bit of a scare until a colleague noticed the bug.

package main

import _ "embed"

// go:embed hello.txt
var file string

func main() { println(file) }

What did you expect to see?

I expected an error reported by a static analyzer or that it fails to compile this code. We have advanced tooling but unfortunately we are still letting such bugs go through. I really think that the compiler should do more to help out users flag such problems. I worked on an initiative such as in CL https://go-review.googlesource.com/c/go/+/34562/ which @randall77 alternated and nailed in https://go-review.googlesource.com/c/go/+/45010 to reject unknown pragmas in the standard library. I expected either a static analyzer report something such as

/home/emmanuel/Desktop/openSrc/bugs/golang/embed/main.go:5:1: pragmas should not have leading whitespace
Got:   "// go:embed hello.txt"
Want: "//go:embed hello.txt"

We should try as much to make our users very successful

What did you see instead?

$ go run main.go 

To solve this problem I wrote a static analyzer internally that I'd be delighted to open source or even write a pass for, for x/go/analysis or to contribute a compiler patch.

[seankhliao]

dup of #43698 ?

[gopherbot]

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

[odeke-em]

I've mailed out a new static analyzer "pragmyzer" in CL https://go-review.googlesource.com/c/tools/+/381914 which fixes the problem and is simple.

Thank you for pointing that out @seankhliao, indeed in a limited case that that issue only requests for cmd/vet! However, I still think perhaps we should consider having the compiler actually fail to compile hence I shall keep this one open for proposal review.

[mvdan]

My take is that a vet check is enough, as long as it runs as part of go test. Historically that combination has worked well, as developers run their tests regularly, and especially for error conditions that don't really fit a compiler's role.

[zpavlinovic]

That being said, perhaps the most straightforward way is indeed to take the logic from "pragmyzer" and push it into buildtag analyzer?

[ianlancetaylor]

I changed this to be a cmd/vet proposal.

[rsc]

There is already a buildtag vet check. That should be extended if needed.
It's a little tricky, because what if that's in a doc comment?
But we should probably do it.
The check should probably look for "// go:".
The buildtag vet check is still fine though; we do not need a separate analyzer.

[rsc]

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

[seankhliao]

This was already accepted for cmd/vet in #43698

[mvdan]

Also worth noting that I already suggested using a compiler/build error in #43698 (comment), and it seems like the idea was already discarded in favor of vet. So I agree with @seankhliao that this seems like a dupe; it's far too easy to forget that a proposal has already been filed, even when you've participated in the previous one! :)

[rsc]

Thanks for digging that up!

[rsc]

This proposal is a duplicate of a previously discussed proposal, as noted above,
and there is no significant new information to justify reopening the discussion.
The issue has therefore been declined as a duplicate.
— rsc for the proposal review group