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

Comments

Projects
None yet
9 participants
@0xmohit
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2015

@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 comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

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

tmthrgd added a commit to tmthrgd/go-bindata that referenced this issue Apr 11, 2017

Workaround for go vet bug (closes #4)
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

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

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 modified the milestones: Unplanned, Go1.11 Mar 13, 2018

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

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

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

Only lexing (scanning) is needed, no parsing.

@robpike

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented May 4, 2018

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

@gopherbot gopherbot closed this in 07d384b May 29, 2018

gwatts added a commit to gwatts/rootcerts that referenced this issue Aug 10, 2018

Disable go vet during Travis test builds
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.