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/go: build required dependencies for 'go vet' type information #16086

Closed
ixdy opened this issue Jun 16, 2016 · 24 comments
Closed

cmd/go: build required dependencies for 'go vet' type information #16086

ixdy opened this issue Jun 16, 2016 · 24 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ixdy
Copy link
Contributor

ixdy commented Jun 16, 2016

Quick summary: go vet currently fails on some code in the Kubernetes repo.
After running go install, however, go vet no longer fails - possibly because it's not building the code anymore?

  1. What version of Go are you using (go version)?
$ go version
go version go1.6.2 linux/amd64
  1. What operating system and processor architecture are you using (go env)?
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/tmp/go"
GORACE=""
GOROOT="/usr/local/google/home/jgrafton/.gvm/gos/go1.6.2"
GOTOOLDIR="/usr/local/google/home/jgrafton/.gvm/gos/go1.6.2/pkg/tool/linux_amd64"
GO15VENDOREXPERIMENT="1"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"
  1. What did you do?
$ mkdir -p /tmp/go/src/k8s.io
$ cd /tmp/go/src/k8s.io
$ git clone https://github.com/kubernetes/kubernetes.git
[output elided]
$ git checkout v1.4.0-alpha.0-248-g0856e1dd43834f
[output elided]
$ cd kubernetes
$ export GOPATH=/tmp/go
$ go vet ./pkg/kubelet/rkt
pkg/kubelet/rkt/rkt_test.go:84: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:130: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:144: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:851: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:982: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1040: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1103: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1815: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1827: appctypes.Annotations composite literal uses unkeyed fields
exit status 1
$ go install ./pkg/kubelet/rkt/...
$ go vet ./pkg/kubelet/rkt
[no output, exit success]
$ rm -r /tmp/go/pkg/linux_amd64/k8s.io/kubernetes/vendor/github.com/appc/spec/
$ go vet ./pkg/kubelet/rkt
pkg/kubelet/rkt/rkt_test.go:84: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:130: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:144: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:851: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:982: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1040: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1103: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1815: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1827: appctypes.Annotations composite literal uses unkeyed fields
exit status 1
  1. What did you expect to see?
    go vet should've still failed after go install.
  2. What did you see instead?
    go vet passed, until intermediate compilation artifacts were removed.

cc @mikedanese who first discovered this.

@ixdy
Copy link
Contributor Author

ixdy commented Jun 16, 2016

go1.7beta1 seems to be broken in the opposite way, which is even more worrying: go vet fails only after go install is run:

$ rm -rf /tmp/go
...
[same git setup steps as before]
...
$ go version
go version devel +3c6b668 Wed Jun 1 17:22:03 2016 linux/amd64
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/tmp/go"
GORACE=""
GOROOT="/usr/local/google/home/jgrafton/.gvm/gos/go1.7beta1"
GOTOOLDIR="/usr/local/google/home/jgrafton/.gvm/gos/go1.7beta1/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build228397039=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
$ pwd
/tmp/go/src/k8s.io/kubernetes
$ go vet ./pkg/kubelet/rkt
[ no output, passes]
$ go install ./pkg/kubelet/rkt/...
$ go vet ./pkg/kubelet/rkt
pkg/kubelet/rkt/rkt_test.go:855: k8s.io/kubernetes/vendor/github.com/appc/spec/schema/types.EnvironmentVariable composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:988: k8s.io/kubernetes/vendor/github.com/appc/spec/schema/types.EnvironmentVariable composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1046: k8s.io/kubernetes/vendor/github.com/appc/spec/schema/types.EnvironmentVariable composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1047: k8s.io/kubernetes/vendor/github.com/appc/spec/schema/types.EnvironmentVariable composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1109: k8s.io/kubernetes/vendor/github.com/appc/spec/schema/types.EnvironmentVariable composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1110: k8s.io/kubernetes/vendor/github.com/appc/spec/schema/types.EnvironmentVariable composite literal uses unkeyed fields
exit status 1

I guess there could be something funky with vendor going on here? I haven't tried to simplify this yet.

@ianlancetaylor
Copy link
Contributor

cmd/vet makes some decisions based on types. When the type is defined in some other package, cmd/vet tries to determine the type by looking at the installed packages. This has the fortunate consequences of better results, but the unfortunate consequence of results that are harder to reproduce.

In this case you can see that the types in question are in different packages. The vet warning composite literal uses unkeyed fields is only produced for struct literals, as that is the only case where it really matters. Presumably appctypes.Annotations and appctypes.Exec are not struct types, so the warning is useless anyhow.

Closing because this is expected behavior. Please feel free to comment if you disagree.

@ianlancetaylor
Copy link
Contributor

Ah, I see my understanding of this particular problem is out of date thanks to issue #9171. In 1.7 we are more careful about issuing this warning for unknown types.

I'm not sure why the 1.7 warnings are not issued for 1.6. Are the warnings correct?

@ixdy
Copy link
Contributor Author

ixdy commented Jun 16, 2016

Now that I'm looking more closely, yes, the warnings in 1.6 are incorrect.
Annotations is a struct slice:

type Annotations []Annotation
type annotations Annotations
type Annotation struct {
        Name  ACIdentifier `json:"name"`
        Value string       `json:"value"`
}

and it's used like

Annotations: appctypes.Annotations{                                                                                                
        appctypes.Annotation{
                Name:  *appctypes.MustACIdentifier(k8sRktKubeletAnno),
                Value: k8sRktKubeletAnnoValue,
        },
        appctypes.Annotation{                                                                                                      
                Name:  *appctypes.MustACIdentifier(types.KubernetesPodUIDLabel),
                Value: podUID,
        },
[etc]

which seems valid.

Exec is just []string, so its uses are definitely OK:

Exec:              appctypes.Exec{"/bin/foo", "bar"},

The 1.7 warnings look legitimate, too.

type Environment []EnvironmentVariable
type environment Environment
type EnvironmentVariable struct {
        Name  string `json:"name"`
        Value string `json:"value"`
}

One of the failing lines looks like

Environment: []appctypes.EnvironmentVariable{
        {"env-foo", "bar"},                                                                                   
},

which is definitely using unkeyed fields.

I guess my only complaint then is that go vet behavior is more correct when dependencies have been built. Can it not build dependencies as necessary so it gives more accurate results?

@ianlancetaylor ianlancetaylor changed the title cmd/vet: failures not reported after code is built cmd/vet: optionally build required dependencies so that results are correct and consistent Jun 16, 2016
@ianlancetaylor
Copy link
Contributor

I will reopen the issue as a request that cmd/vet rebuild dependencies. That can not be a required behavior, as people use cmd/vet to check incomplete code and dependencies may not be available. But I think it would be an optional behavior.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jun 16, 2016
@ixdy
Copy link
Contributor Author

ixdy commented Jun 16, 2016

sounds great, thanks!

@josharian
Copy link
Contributor

IIUC, it's not a question of rebuilding dependencies but of re-typechecking them, at least when source code is available. This is generally desirable, since it avoids these kinds of problems, and go/types is fast. There's a similar issue and discussion around stringer: #10249. The permanent answer here is to get go/loader's API fixed up and have vet use it. I'm not sure where that project stands.

@griesemer
Copy link
Contributor

go vet should probably use a source-based importer for for go/types so that its dependencies are as up-to-date as possible.

@griesemer griesemer modified the milestones: Go1.8Maybe, Go1.8 Sep 6, 2016
@josharian
Copy link
Contributor

If/when go vet uses a source-based importer, the vet-the-core runner currently in CL 27811 should be updated not to do go install -a std cmd.

@nmiyake
Copy link
Contributor

nmiyake commented Oct 2, 2016

+1 to using source-based importer. We were running go vet in our CI environment and weren't running go install ./... first, so there were many issues which were not flagged in our CI environment but would be flagged locally if the packages were built, which was extremely confusing.

For reference, here's a self-contained test that demonstrates the issue (wrote it with the intent of including it in a bug before finding this issue):

package main_test

import (
    "io/ioutil"
    "os"
    "os/exec"
    "path"
    "testing"
)

const (
    testMain = `package main
import "github.com/nmiyake/govetbug/testprint"
func main() {
    testprint.Print("hello, %v!", "world")
}
`
    testPrint = `package testprint
import "fmt"
func Print(msgAndArgs ...interface{}) {
    if len(msgAndArgs) == 0 {
        return
    }
    fmt.Printf(msgAndArgs[0].(string), msgAndArgs[1:]...)
}
`
)

// (1) Create a file that calls a "Print" function in another package in a manner that should trigger a go vet error
// (2) Run "go vet ." on the project
//
// Expected: go vet issues are reported
// Actual: go vet reports no failures
//
// (4) Run "go install ./..." on the project
// (5) Run "go vet ." on the project
//
// Expected: output/behavior is the same as running after (2)
// Actual: go vet now reports the failures as originally expected
func TestGoVet(t *testing.T) {
    // create tmpDir to act as test GOPATH
    tmpDir, err := ioutil.TempDir("", "")
    if err != nil { panic(err) }
    defer os.RemoveAll(tmpDir)

    err = os.Setenv("GOPATH", tmpDir)
    if err != nil { panic(err) }

    // write test print file
    testPrintDir := path.Join(tmpDir, "src/github.com/nmiyake/govetbug/testprint")
    err = os.MkdirAll(testPrintDir, 0755)
    if err != nil { panic(err) }
    err = ioutil.WriteFile(path.Join(testPrintDir, "testprint.go"), []byte(testPrint), 0644)
    if err != nil { panic(err) }

    // write out test main file
    testMainDir := path.Join(tmpDir, "src/github.com/nmiyake/govetbug")
    err = ioutil.WriteFile(path.Join(testMainDir, "main.go"), []byte(testMain), 0644)
    if err != nil { panic(err) }

    cmd := exec.Command("go", "vet", ".")
    cmd.Dir = testMainDir
    output, err := cmd.CombinedOutput()
    // Expect "possible formatting directive in Print call" error, but does not occur
    if err != nil {
        t.Errorf("%v failed: %v", cmd.Args, string(output))
    }

    // install the package
    cmd = exec.Command("go", "install", "./...")
    cmd.Dir = testMainDir
    output, err = cmd.CombinedOutput()
    if err != nil {
        t.Errorf("%v failed: %v", cmd.Args, string(output))
    }

    cmd = exec.Command("go", "vet", ".")
    cmd.Dir = testMainDir
    output, err = cmd.CombinedOutput()
    // "possible formatting directive in Print call" is reported after "go install ./..." is run
    if err != nil {
        t.Errorf("%v failed: %v", cmd.Args, string(output))
    }
}

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
tbg added a commit to tbg/cockroach that referenced this issue Oct 14, 2016
All packages need to be installed before we can run (some) of the checks and
code generators reliably. More precisely, anything that using x/tools/go/loader
is fragile (this includes stringer, vet and others).

The blocking issue is golang/go#14120; see
golang/go#10249 for some more concrete discussion on
`stringer` and golang/go#16086 for `vet`.
@rsc
Copy link
Contributor

rsc commented Oct 20, 2016

We could make the go command just build the necessary pieces and pass the path to tool vet, like it does for, say, go test or go build. We probably should. But not for Go 1.8.

@rsc rsc modified the milestones: Go1.9, Go1.8Maybe Oct 20, 2016
@rsc rsc changed the title cmd/vet: optionally build required dependencies so that results are correct and consistent cmd/go: build required dependencies for 'go vet' type information Oct 20, 2016
@gopherbot
Copy link

CL https://golang.org/cl/36992 mentions this issue.

@josharian
Copy link
Contributor

We could make the go command just build the necessary pieces and pass the path to tool vet, like it does for, say, go test or go build. We probably should.

I think we should have vet typecheck from source. The compiler is much slower than go/types--it has to do lots more work.

@griesemer
Copy link
Contributor

It's a 1.9 item for me to compute package information from source for gotype, if it's not present. See #11415.

gopherbot pushed a commit to golang/tools that referenced this issue Feb 17, 2017
As noted by griesemer in golang/go#18799, this doesn't address the issues
raised in golang/go#11415 because when checking an xtest package the
corresponding package is assumed to have been installed. This
however is similar to the assumptions made by go vet (raised
as an issue in golang/go#16086). So whilst not perfect, it will probably
suffice until golang/go#11415 is resolved.

Fixes golang/go#18799

Change-Id: I1ea005c402e5d6f5abddda68fee6386b0531dfba
Reviewed-on: https://go-review.googlesource.com/36992
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer
Copy link
Contributor

#11415 is now fixed. This issue can go ahead now.

@rsc
Copy link
Contributor

rsc commented Mar 2, 2017

I disagree that vet should use source. In a large tree, you can cache the 'go build' output.

@josharian
Copy link
Contributor

@rsc can I at least add a -source flag to 'go tool vet', to force it into read-from-source mode?

@gopherbot
Copy link

CL https://golang.org/cl/37690 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 2, 2017
Add a -source flag to cmd/vet that instructs
it to typecheck purely from source code.

Updates #16086
Fixes #19332

Change-Id: Ic83d0f14d5bb837a329d539b2873aeccdf7bf669
Reviewed-on: https://go-review.googlesource.com/37690
Reviewed-by: Rob Pike <r@golang.org>
bboozzoo added a commit to bboozzoo/mender-go-lib-micro that referenced this issue Mar 15, 2017
go vet relies on some type information extracted from installed archives. See
golang/go#16086 for details.

Signed-off-by: Maciej Borzecki <maciej.borzecki@rndity.com>
@rsc
Copy link
Contributor

rsc commented Jun 22, 2017

Vet will need even more work along these lines with the new caching work planned for Go 1.10. But then things will be better, and this will be fixed. See also #17571.

@rsc rsc modified the milestones: Go1.10, Go1.9 Jun 22, 2017
@tamird
Copy link
Contributor

tamird commented Jun 22, 2017

@rsc I think you may have linked to the wrong issue?

@rsc
Copy link
Contributor

rsc commented Jun 23, 2017

Thanks, fixed link - #17571.

@corylanou
Copy link

Today using go 1.8.3 and go vet ./... I kept getting the same error on a copied mutex even though I corrected the code. I ended up finally removing my$GOPATH/pkg directory and then when I re-ran the go vet ./... command everything came up clean. Not sure if this is related or not.

@gopherbot
Copy link

Change https://golang.org/cl/74356 mentions this issue: cmd/go: run vet automatically during go test

@gopherbot
Copy link

Change https://golang.org/cl/74750 mentions this issue: cmd/go: pass package config to vet during "go vet"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

10 participants