Go: cmd/vet: build tags in raw string literals should be ignored

[0xmohit]

main.go:

package main
const foo = `
//+build ignore
`

Running vet produces:

main.go:3: +build comment must appear before package clause and be followed by a blank line

This also contributes to noise in the vet output for core, e.g.

./src/crypto/x509/root_darwin_arm_gen.go:181: +build comment must appear before package clause and be followed by a blank line
./src/crypto/x509/root_darwin_arm_gen.go:182: +build comment must appear before package clause and be followed by a blank line
./src/crypto/x509/root_darwin_arm_gen.go:183: +build comment must appear before package clause and be followed by a blank line

[cznic]

Build tags detection by the go build system is by design line oriented to avoid unnecessary parsing. The vet tool does the same thing, which seems correct to me.

[ianlancetaylor]

@cznic That is a fair point, but in this case the other tools are not going to recognize the +build comment, which is what cmd/vet is saying anyhow. In a case like this it seems worth skipping raw string literals if not too complicated.

[tmthrgd]

This bug is actually a duplicate of #12269, I guess no one spotted that. That bug was closed by @robpike with the given reason "Not worth the trouble to fix."

I've hit this bug myself (here - tmthrgd/go-bindata#4), it's nothing all too major of course. Although it would be nice if go vet wasn't throwing up this false positive.

Is there any chance this bug might get fixed or is it still "not worth the trouble"?

[griesemer]

@ianlancetaylor Skipping raw string literals means recognizing them which requires full lexing (unless I'm missing some shortcut). That's probably more expensive than what we do now (to be verified). It seems a bit of overkill given that it's trivial to work around this issue (use a regular string and string concatenation as needed).

[ianlancetaylor]

Yes, it's probably not worth doing. Closing.

[josharian]

Now that vet is on by default, and we are aiming for 100% accuracy on such checks, and we’ve gotten a handful of dups of this recently, I think we should consider fixing this. Yes, parsing is more expensive than line-oriented searches, but compared to the rest of vet, which does full typechecking, I doubt it matters much, particularly since we only need parse up to the package clause.

[josharian]

Scratch the comment about only parsing to the package clause. Still shouldn’t be too expensive.

[griesemer]

Only lexing (scanning) is needed, no parsing.

[robpike]

I would think the right fix is a complete rewrite of the module that looks at the comment-decorated parse tree, which we have. The parse will honor valid build comments but disregard bad ones, which would still be present as comments in the tree.

[mvdan]

I'm going to give this a go - my current thinking is that since vet now always has the go/ast.File for all Go files, we can use that to cross-check that the lines that look like comments are actually comments, if we are on a Go file. That should get rid of the false positives in question without changing any other behavior.

I had initially re-written buildtag.go to simply iterate over ast.File.Comments, until I realised that the check must work for non-Go files such as assembly too :)

[gopherbot]

Change https://golang.org/cl/111415 mentions this issue: cmd/vet: avoid false positives with non-comments