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: support "vet a.go a_test.go b.go" when a regular package and a test package #22530

Closed
stevenh opened this issue Nov 1, 2017 · 15 comments

Comments

Projects
None yet
4 participants
@stevenh
Copy link
Contributor

commented Nov 1, 2017

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

go version go1.9.2 freebsd/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN="/data/go/bin"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="freebsd"
GOOS="freebsd"
GOPATH="/data/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/freebsd_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build969955960=/tmp/go-build -gno-record-gcc-switches"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

go tool vet example_test.go ticker.go 

What did you expect to see?

No errors

What did you see instead?

example_test.go:10: ExampleTicker refers to unknown identifier: Ticker

The issue is order dependent so if name the files in the opposite order then no error occurs.

The problem is that the pkg used by the checks is the first package from the listed files.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2017

What do the files example_test.go and ticker.go look like?

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Nov 1, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Nov 1, 2017

Change https://golang.org/cl/75190 mentions this issue: cmd/vet: prevent invalid errors due to package ordering

@stevenh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2017

Example is included in the test I just pushed @ianlancetaylor

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2017

Thanks. I'm not convinced that your fix is correct. The docs for cmd/vet say that when you use vet with a list of files on the command line, all of the files must be in the same package. Your example does not do that. I think a better fix would be to detect this case and report an error. I think your CL is trying to patch a specific case of a general problem, but the general problem will just crop up in different ways.

@stevenh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2017

As far as I can see it only does that in the case where you run vet on a directory, not on an explicit list of files. Am I missing something?

@stevenh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

extendScope was the precedence I found when looking into this issue.

You also have the definition of basePkg and arg processing

@stevenh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

An example of something that breaks due to this is gometalinter which calls go tool vet *.go this is how I came up against the issue and there is quite a few reports about it, but no fixes hence investigating it.

If we where trying to make it work when pointing it to files that are not in the same package directory then yes it would trying to fix something that its not designed to do, however given the information I have I believe this is an expected use case.

The fact that the example in the vet description page suffers from this issue depending on the order that the files are returned from glob further supports this, even though it does state they must be in the same package.

By files:
go tool vet source/directory/*.go
vets the files named, all of which must be in the same package.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

It sounds like you are arguing that we should change the way that cmd/vet is currently documented to work: when invoked with a list of file names, the files must all be in either the same package or in a test package. If we implement that, then we should make that case work, much as it does when we run vet on a single directory. I don't see any reason to expect that simply sorting the files will make all cases work, even though it fixes your case. We should change vet to handle it correctly by building whatever data structures it builds when running vet on a directory.

But before you put any work into that we should decide that that is what we want to have happen. I'll repurpose this issue.

@ianlancetaylor ianlancetaylor changed the title cmd/vet: invalid "refers to unknown identifier" cmd/vet: support "vet a.go a_test.go b.go" when a regular package and a test package Nov 3, 2017

@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.11 Nov 3, 2017

@stevenh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2017

Having a quick look at how the dir case handles this, it does so by running a check on all files which match the main package, then it runs a separate pass on the files which match _test the the base package passed in.

It looks like this could be replicated to file set case, but I from what I've seen I believe that the sort already achieves most of, if not all of this as the key thing is identifying and using the correct "main" package.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

If we're going to do it, let's do it right.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2017

Please try using "go vet" instead of "go tool vet". The latter is now essentially only for developers of vet to help debugging, like "go tool compile".

@stevenh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2017

I can confirm that go vet example_test.go ticker.go passes where go tool vet example_test.go ticker.go fails.

Should we just update the examples to use go vet instead of go tool vet?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2017

@stevenh Good point, I sent https://golang.org/cl/76390 .

@gopherbot

This comment has been minimized.

Copy link

commented Nov 7, 2017

Change https://golang.org/cl/76390 mentions this issue: cmd/vet: change docs to prefer "go vet" over "go tool vet"

gopherbot pushed a commit that referenced this issue Nov 7, 2017

cmd/vet: change docs to prefer "go vet" over "go tool vet"
Updates #22530

Change-Id: I161b5e706483744321e6089f747bd761310774eb
Reviewed-on: https://go-review.googlesource.com/76390
Reviewed-by: Rob Pike <r@golang.org>

@rsc rsc closed this Nov 13, 2017

@golang golang locked and limited conversation to collaborators Nov 13, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.