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: build tags in raw string literals should be ignored #13533

Closed
0xmohit opened this issue Dec 8, 2015 · 11 comments
Closed

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

0xmohit opened this issue Dec 8, 2015 · 11 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@0xmohit
Copy link
Contributor

0xmohit commented Dec 8, 2015

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
Copy link
Contributor

cznic commented Dec 8, 2015

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
Copy link
Contributor

@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
Copy link
Contributor

tmthrgd commented Apr 10, 2017

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
Copy link
Contributor

@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
Copy link
Contributor

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

tmthrgd added a commit to tmthrgd/go-bindata that referenced this issue Apr 11, 2017
This is pretty firmly a bug in go vet (see golang/go#12269 and
golang/go#13533), but is considered not worth the effort to fix
by the Go authors. This is a workaround and I don't love it, but
it's not objectively hideous.
@josharian
Copy link
Contributor

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 josharian reopened this Mar 13, 2018
@josharian josharian added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 13, 2018
@josharian josharian modified the milestones: Unplanned, Go1.11 Mar 13, 2018
@josharian
Copy link
Contributor

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

@griesemer
Copy link
Contributor

Only lexing (scanning) is needed, no parsing.

@robpike
Copy link
Contributor

robpike commented Mar 13, 2018

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
Copy link
Member

mvdan commented May 4, 2018

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
Copy link

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

gwatts added a commit to gwatts/rootcerts that referenced this issue Aug 10, 2018
Go 1.10 runs go vet during go test automatically; unfortunately go vet
fires a false positive triggered by a build tag appearing in a template
string literal.  Will be fixed in Go 1.11

See golang/go#13533
@golang golang locked and limited conversation to collaborators May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

9 participants