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: incorrect detection of +build tag even if it appears as a string in a file #26627

Closed
andybons opened this issue Jul 26, 2018 · 14 comments

Comments

Projects
None yet
7 participants
@andybons
Copy link
Member

commented Jul 26, 2018

Sometimes you may want to write a test that contains code with a build tag.

Example: https://go-review.googlesource.com/c/tools/+/125040

But it will fail to build due to the build tag in the test file.

@bcmills @rsc

@andybons andybons added the NeedsFix label Jul 26, 2018

@andybons andybons added this to the Unplanned milestone Jul 26, 2018

mostynb added a commit to mostynb/tools that referenced this issue Jul 26, 2018

Reland "godoc: skip build tag annotations when displaying examples"
After moving the filepath.Walk example to a standalone example file
in CL 122237 (so it could use a standalone function), godoc includes
the build tag annotation ("// +build !windows,!plan9" in this case)
in the runnable example.  The example runs correctly, but the
annotation might be confusing for new users.

I think godoc should skip these annotations when displaying examples.

Fixes golang/go#26490.

Skipped the testcase for now, since it triggers a false positive:
golang/go#26627

Change-Id: I1da4b3b7e1e5a85a76773e25d9355b3f92479c19
@gopherbot

This comment has been minimized.

Copy link

commented Jul 26, 2018

Change https://golang.org/cl/126256 mentions this issue: Reland "godoc: skip build tag annotations when displaying examples"

@mostynb

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

@andybons: what are the reproduction steps for this?

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

I don’t understand the issue. Is the problem that vet is complaining about it? Or something else? cc @mvdan just in case it is vet.

@dominikh

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

Vet complains about misplaced build tags. However, it doesn't use the Go parser to look at actual comments, it does a simple text search on the file. Thus, a multi-line string containing what looks like a build tag can trigger a vet warning. Since go test nowadays runs vet, it causes hard failure.

@mostynb

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

OK, so go vet godoc/*.go with the test in https://go-review.googlesource.com/c/tools/+/125040 shows the issue. I can try to take a look later today.

@agnivade

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

Re-titling it as a vet issue.

EDIT: Also, I'm surprised we don't have a single test which has build tags as normal text ? How are we even testing build tags ?

@agnivade agnivade changed the title cmd/go: false positive +build comment must appear before package clause and be followed by a blank line cmd/vet: incorrect detection of +build tag even if it appears as a string in a file Jul 27, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

Unless my memory fails me, I improved the vet check so that it would have zero false positives in Go source files, even in the presence of multiline strings. So this sounds like a bug - perhaps an edge case that I missed.

@mvdan mvdan self-assigned this Jul 27, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

I am clearly missing something here - this only happens with vet from Go 1.10 and not tip, as one would expect after my vet improvement in https://go-review.googlesource.com/c/go/+/111415:

$ git rev-parse HEAD # right before the revert
faa8a71ab532725a7fd9d901a97f74c5038c409b
$ go version
go version devel +5f5402b723 Tue Jul 24 23:54:08 2018 +0000 linux/amd64
$ go vet
$ go1 version
go version go1.10.3 linux/amd64
$ go1 vet
# golang.org/x/tools/godoc
./godoc_test.go:327: +build comment must appear before package clause and be followed by a blank line
./godoc_test.go:328: +build comment must appear before package clause and be followed by a blank line

So as far as I can tell, this is a duplicate of #13533 and has been fixed for 1.11 since May. Do we need 1.10's vet to also be fixed for some reason? The change in question is quite large, so it's likely too invasive to backport into 1.10.x.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

How are we even testing build tags?

See src/cmd/vet/testdata/buildtag/.

@agnivade

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

this only happens with vet from Go 1.10 and not tip

That seems correct. Although it is very hard to say which version of Go is being tested against for the tools repo here https://build.golang.org/?repo=golang.org%2fx%2ftools which caused the test to fail.

Do we need 1.10's vet to also be fixed for some reason?

Assuming we are testing using 1.10, then we cannot add the test done in CL 125040 because the build dashboard becomes red. I am not sure what is to be done here. Maybe we can put the test behind a build tag or something ? (Funny that we are using a build tag to prevent detection of a build tag)

See src/cmd/vet/testdata/buildtag/.

Thanks. Makes sense now since it is fixed at tip. :)

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

Although it is very hard to say which version of Go is being tested against for the tools repo here

I see three Go versions per each tools repo commit, presumably Go master, 1.10.x, and 1.9.x. So I guess there's our answer.

I am not sure what is to be done here.

One temporary solution, until all 1.12 is out and 1.10 can be dropped, is to "hide" the build tag in the multiline string, like:

const str = `
`+`// +build foo
etc etc
`
@mostynb

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2018

@mvdan: updated the reland with a similar workaround: https://go-review.googlesource.com/c/tools/+/126256

@agnivade

This comment has been minimized.

Copy link
Member

commented Aug 12, 2018

@mvdan - Perhaps this should be closed since this is fixed at tip ?

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 12, 2018

A workaround was used in the CL above, so yes, I agree that this can be closed. Thanks for the ping!

@mvdan mvdan closed this Aug 12, 2018

nmiyake added a commit to nmiyake/errcheck that referenced this issue Dec 30, 2018

Fix test for Go 1.10
Fails in Go 1.10 due to golang/go#26627.
Use workaround described at golang/go#26627 (comment)
to fix.

nmiyake added a commit to nmiyake/errcheck that referenced this issue Dec 30, 2018

Fix test for Go 1.10
Fails in Go 1.10 due to golang/go#26627.
Use workaround described at golang/go#26627 (comment)
to fix.
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.