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

Jump to bottom

cmd/vet: more clearly document go tool vet on a single file #23916

Closed
ooesili opened this issue Feb 19, 2018 · 11 comments
Closed

cmd/vet: more clearly document go tool vet on a single file #23916

ooesili opened this issue Feb 19, 2018 · 11 comments
Labels
Documentation FrozenDueToAge
Milestone
Go1.11

Comments

@ooesili
Copy link

ooesili commented Feb 19, 2018

What did you do?

After upgrading from 1.9.4 to 1.10, I ran go vet against a single test file.

$ go vet vetissue_test.go

I have simplified the code that gave me the original issue and uploaded it
here.

What did you expect to see?

No output, and a successful exit code. This command succeeded on Go 1.9.4.

What did you see instead?

$ go vet vetissue_test.go
# command-line-arguments
./vetissue_test.go:6:6: undefined: value

What's happening here is that go vet is complaining about not being able to
find the definition of an identifier that was declared in a different file in
the same package.

It looks like this change in behaviour was introduced in an effort to run go vet in parallel with go test, specifically in https://golang.org/cl/74355
which causes go vet to fail when the type check fails.

I'm not really sure what the best course of action is here. My thought is that
when running go vet against a single file, you can never be guaranteed to have
all of the type information you need, so we could ignore type check failures in
vet against single files.

System details

go version go1.10 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/wmerkel/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/wmerkel/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.10/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.10/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
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"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/52/h8r09kdd5t17prlr9whr4cs4pm06q9/T/go-build420935568=/tmp/go-build -gno-record-gcc-switches -fno-common"
GOROOT/bin/go version: go version go1.10 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.10
uname -v: Darwin Kernel Version 17.4.0: Sun Dec 17 09:19:54 PST 2017; root:xnu-4570.41.2~1/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.13.3
BuildVersion:	17D47
lldb --version: lldb-900.0.64
  Swift-4.0
@ooesili ooesili changed the title cmd/vet: behavior of vetting a single file has change from 1.9.4 to 1.10 cmd/vet: behavior of vetting a single file has changed from 1.9.4 to 1.10 Feb 19, 2018
@ianlancetaylor
Copy link
Contributor

To run vet on a single file, use go tool vet.

@ooesili
Copy link
Author

ooesili commented Feb 19, 2018

Will go tool vet return the same results as running go vet on the package the file is in? I saw this #16086 (comment) and am curious about what checks might be skipped if the type-check information is not available.

@ooesili ooesili mentioned this issue Feb 19, 2018
Closed
@ianlancetaylor
Copy link
Contributor

It's true that some checks are only available if type checking information is available. That's not a change from 1.9, though. Offhand I'm not sure which vet tests are affected.

@carterjones
Copy link

I confirmed the behavior differs between go vet and go tool vet (see fork here: https://github.com/carterjones/vetissue):

$ go vet other_file.go             
# command-line-arguments
./other_file.go:6:5: undefined: value
$ go tool vet other_file.go 
$ go version               
go version go1.10 darwin/amd64

@ooesili
Copy link
Author

ooesili commented Feb 19, 2018

So I guess the breaking change already happened, and there is a workaround (go tool vet instead of go vet). My only asks would be that this be documented somewhere, and we maintain a position on what's allowed to break in the future. It makes sense that over time go vet will get smarter and fail when it previously didn't but I don't want this to happen again.

Can we commit to saying go vet should only be run against packages, and that go tool vet will always work against single files?

carterjones pushed a commit to carterjones/pre-commit-golang that referenced this issue Feb 19, 2018
@carterjones
fix go vet check
03b07e4 
See golang/go#23916 for bug details.
@mvdan
Copy link
Member

mvdan commented Feb 19, 2018

Can we commit to saying go vet should only be run against packages, and that go tool vet will always work against single files?

$ go help vet
usage: go vet [-n] [-x] [build flags] [vet flags] [packages]

Vet runs the Go vet command on the packages named by the import paths.
[...]
$ go tool vet -h
Usage of vet:
        vet [flags] directory...
        vet [flags] files... # Must be a single package
[...]

I understand that this change of behavior of go vet in 1.10 can be confusing, but I belive it's already mentioned in the changelog clearly enough: https://golang.org/doc/go1.10#vet

@mvdan
Copy link
Member

mvdan commented Feb 19, 2018

The fact that go tool vet can work without type information (i.e. without full packages) isn't terribly obvious from reading its usage text, though. A change there might be useful.

@ooesili
Copy link
Author

ooesili commented Feb 19, 2018

usage: go vet [-n] [-x] [build flags] [vet flags] [packages]

Considering this, wouldn't it make sense for go vet to explicitly not accept individual files as arguments? Since it doesn't work anyways, we could give users a better error message.

@mvdan
Copy link
Member

mvdan commented Feb 19, 2018

From go help packages:

As a special case, if the package list is a list of .go files from a single directory, the command is applied to a single synthesized package made up of exactly those files, ignoring any build constraints in those files and ignoring any other files in the directory.

Both accept files. The question is whether it's a requirement that they form a complete package or not.

This was referenced Feb 20, 2018
Merged
Closed
Merged
oschwald added a commit to maxmind/Code-TidyAll-Plugin-Go that referenced this issue Mar 10, 2018
@oschwald
Use go tool vet
2804f84 
As of 1.10, `go vet` no longer works correctly on individual files. As
such, we use `go tool vet` instead. See
golang/go#23916 for more information.
@oschwald oschwald mentioned this issue Mar 10, 2018
Merged
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 28, 2018
@ianlancetaylor ianlancetaylor changed the title cmd/vet: behavior of vetting a single file has changed from 1.9.4 to 1.10 cmd/vet: more clearly document go tool vet on a single file Mar 28, 2018
@ianlancetaylor
Copy link
Contributor

It's not clear to me what to do here. I feel like there is documentation for how the tool works at https://golang.org/cmd/vet. It's true that vet changed from 1.9 to 1.10, and that was documented in the release notes. The question now is: what, if anything, needs to change in the cmd/vet and/or cmd/go docs? It would be very helpful if people who think that there should be a change could send in a CL. Thanks.

@mmaxim mmaxim mentioned this issue May 9, 2018
Merged
@ianlancetaylor
Copy link
Contributor

No response, so I'm going to close this issue. Please comment if you disagree.

@stefantalpalaru stefantalpalaru mentioned this issue Jun 15, 2018
Open
@derekargueta derekargueta mentioned this issue Mar 7, 2019
Open
@andresmgot andresmgot mentioned this issue Mar 29, 2019
Closed
3 tasks
@SataQiu SataQiu mentioned this issue Apr 8, 2019
Merged
3 tasks
rargulati added a commit to keep-network/pre-commit-golang that referenced this issue Apr 18, 2019
@rargulati
Ensure we vet the entire project at the same time
47493cd 
Rather than running go vet on a single file at a time, we are required
to run it on the entire project. This is because `go tool vet` and `go
vet` differ in behavior. `go tool vet` is able to analyze and infer
types in a single file. `go vet` is unable to do so (and requires the
context of the entire project`. We change the behavior to run `go vet`
on the entire project.

More captured in this issue:
golang/go#23916
@rargulati rargulati mentioned this issue Apr 18, 2019
Merged
rargulati added a commit to keep-network/pre-commit-golang that referenced this issue Apr 18, 2019
@rargulati
Merge pull request #2 from keep-network/fix-incorrect-go-vet
a3834ea 
Ensure we vet the entirety of the project

Rather than running go vet on a single file at a time, we are required
to run it on the entire project. This is because `go tool vet` and 
`go vet` differ in behavior. `go tool vet` is able to analyze and infer
types in a single file. `go vet` is unable to do so (and requires the
context of the entire project`. We change the behavior to run `go vet`
on the entire project.

More captured in this issue:
golang/go#23916
@golang golang locked and limited conversation to collaborators Jun 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
5 participants
@carterjones @ianlancetaylor @mvdan @ooesili @gopherbot