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: PackageVetx in vet.cfg lists full transitive closure of package dependencies #30296

Closed
slon opened this Issue Feb 18, 2019 · 14 comments

Comments

Projects
None yet
6 participants
@slon
Copy link
Contributor

slon commented Feb 18, 2019

We are building custom analysis driver for golang.org/x/tools/go/analysis.

Seems like current behaviour of go vet command contradicts documentations.

This paragraph states, that each pass has access only to facts from directly imported packages.

Each pass (a, p) starts with the set of facts produced by the same analyzer a applied to the packages directly imported by p.

I would expect, that PackageVetx section of vetx.cfg should contain the same number of entries as the PackageFile section.

But go vet driver lists .vetx files from full transitive closure of package dependencies. That seems less efficient and might cause problems with analysis scalability on large projects or in distributed build setting.

Could you please clarify, what the intended behaviour should be?

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

$ go version
go version go1.11.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/prime/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/prime/Code/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build362922290=/tmp/go-build -gno-record-gcc-switches"

What did you do?

go vet -n bytes

What did you expect to see?

PackageVetx section of vet.cfg should list only directly imported packages.

What did you see instead?

PackageVetx contains full transitive closure of package dependencies.

	"PackageVetx": {
		"context": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"encoding/base64": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"encoding/binary": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"errors": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"flag": "/home/prime/.cache/go-build/48/48d167f02fb6c29c9a15d692ec56e1885d291680c2cc1cc44b7b6549655c1af8-d",
		"fmt": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/bytealg": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/cpu": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/poll": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/race": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/syscall/unix": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/testlog": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"io": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"math": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"math/bits": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"math/rand": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"os": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"path/filepath": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"reflect": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"runtime": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"runtime/debug": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"runtime/internal/atomic": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"runtime/internal/sys": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"runtime/trace": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"sort": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"strconv": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"strings": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"sync": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"sync/atomic": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"syscall": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"time": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"unicode": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"unicode/utf8": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d"
	},
@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Feb 18, 2019

@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Feb 18, 2019

But go vet driver lists .vetx files from full transitive closure of package dependencies.

As with type information, analysis facts also include information about methods and struct fields or imported types, even though they might come from indirectly imported packages. But it should still be much less than the complete transitive closure in most cases.

What command did you run, exactly?

@slon

This comment has been minimized.

Copy link
Contributor Author

slon commented Feb 18, 2019

What command did you run, exactly?

$ ls 
a.go
$ cat a.go
package vet

import "bytes"

func F() {
	_ = bytes.Buffer{}
}
$ go vet -x .
WORK=/tmp/go-build192890857
mkdir -p $WORK/b003/
mkdir -p $WORK/b009/
mkdir -p $WORK/b012/
mkdir -p $WORK/b014/
mkdir -p $WORK/b015/
mkdir -p $WORK/b005/
mkdir -p $WORK/b013/
mkdir -p $WORK/b004/
mkdir -p $WORK/b011/
mkdir -p $WORK/b010/
mkdir -p $WORK/b008/
mkdir -p $WORK/b007/
mkdir -p $WORK/b002/
mkdir -p $WORK/b001/
cat >$WORK/b001/vet.cfg << 'EOF' # internal
{
	"Compiler": "gc",
	"Dir": "/home/prime/Code/go/src/github.com/slon/vet",
	"ImportPath": "github.com/slon/vet",
	"GoFiles": [
		"/home/prime/Code/go/src/github.com/slon/vet/a.go"
	],
	"ImportMap": {
		"bytes": "bytes",
		"fmt": "fmt"
	},
	"PackageFile": {
		"bytes": "/usr/local/go/pkg/linux_amd64/bytes.a",
		"fmt": "/usr/local/go/pkg/linux_amd64/fmt.a"
	},
	"Standard": {
		"bytes": true,
		"fmt": true
	},
	"PackageVetx": {
		"bytes": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"errors": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/bytealg": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/cpu": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"internal/race": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"io": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"runtime": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"runtime/internal/atomic": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"runtime/internal/sys": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"sync": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"sync/atomic": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"unicode": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d",
		"unicode/utf8": "/home/prime/.cache/go-build/38/38d3730d3ea99899bdfe8bfbe3e1b6fb15ff67a8bc46ec86883faf956d4e9b55-d"
	},
	"VetxOnly": false,
	"VetxOutput": "$WORK/b001/vet.out",
	"SucceedOnTypecheckFailure": false
}
EOF
cd /home/prime/Code/go/src/github.com/slon/vet
TERM='dumb' /usr/local/go/pkg/tool/linux_amd64/vet $WORK/b001/vet.cfg

As with type information, analysis facts also include information about methods and struct fields or imported types, even though they might come from indirectly imported packages.

I'm not an expert in go compilation process, so forgive me if i'm wrong. But it seems that in the case of type information all indirect typeinfo is "pulled-along" into .a files. So that only direct dependencies are needed for compilation.

Rob Pipe specifically mentions that fact in his talk https://youtu.be/rKnDgT73v8s?t=880

For example, see https://github.com/slon/vet/blob/master/inner/b.go . This package implicitly uses io.Writer interface. But go compiler is able to build this package without io.a file.

$ go build -x github.com/slon/vet/inner/
WORK=/tmp/go-build554605437
mkdir -p $WORK/b001/
cat >$WORK/b001/importcfg << 'EOF' # internal
# import config
packagefile github.com/slon/vet=/home/prime/.cache/go-build/30/30b1d9659c97fea230f63500e216e6dc22af6d2293eeeb91cad440c900a67a8f-d
EOF
cd /home/prime/Code/go/src/github.com/slon/vet/inner
/usr/local/go/pkg/tool/linux_amd64/compile -o $WORK/b001/_pkg_.a -trimpath $WORK/b001 -p github.com/slon/vet/inner -complete -buildid I0CqKT0XkQVU4cpqjWQG/I0CqKT0XkQVU4cpqjWQG -goversion go1.11.5 -D "" -importcfg $WORK/b001/importcfg -pack -c=4 ./b.go
/usr/local/go/pkg/tool/linux_amd64/buildid -w $WORK/b001/_pkg_.a # internal
cp $WORK/b001/_pkg_.a /home/prime/.cache/go-build/03/03c6bcb1778b1b4311a50e56ccbcc52eacc07aca7043deeb8db815274391fc1d-d # internal
@slon

This comment has been minimized.

Copy link
Contributor Author

slon commented Feb 20, 2019

@alandonovan what do you think?

@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Feb 20, 2019

Thanks. It does appear that the set of .vetx files equals the transitive closure:

% go list -deps bytes | sort
bytes
errors
internal/bytealg
internal/cpu
internal/race
io
runtime
runtime/internal/atomic
runtime/internal/math
runtime/internal/sys
sync
sync/atomic
unicode
unicode/utf8
unsafe

I didn't write this code (see https://tip.golang.org/src/cmd/go/internal/work/exec.go#L1009); it's possible that it was necessary in the original version of vetx but I would expect the set of keys of PackageFile is all that is needed. (Even the "fmt' entry, which is artificially inserted, need no longer be treated specially for vet.)
@rsc @bcmills

@dmitshur dmitshur added this to the Go1.13 milestone Feb 22, 2019

@slon

This comment has been minimized.

Copy link
Contributor Author

slon commented Feb 25, 2019

@dmitshur is there any way I can help with investigation?

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Feb 25, 2019

@slon Sure. Take a look at https://golang.org/wiki/HandlingIssues to become familiar with what's needed to transition to the next state of NeedsDecision or NeedsFix.

Alan has started investigating this and pinged two other people who are familiar with cmd/vet. You should see if there's some information or understanding they need that you can help provide. You might have to wait for them to have a chance to look into this first.

@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Feb 25, 2019

I am not actively investigating this. My advice would be to try to change the logic in cmd/go so that it provides the exact same set of .vetx files to vet as it provides .a files to the compiler, then run all the vet tests.

@slon

This comment has been minimized.

Copy link
Contributor Author

slon commented Feb 26, 2019

The fix turned out very small.

diff --git a/src/cmd/go/internal/work/action.go b/src/cmd/go/internal/work/action.go
index 1f91046eb1..d3cefa35e6 100644
--- a/src/cmd/go/internal/work/action.go
+++ b/src/cmd/go/internal/work/action.go
@@ -417,7 +417,7 @@ func (b *Builder) vetAction(mode, depMode BuildMode, p *load.Package) *Action {
                } else {
                        deps = []*Action{a1, aFmt}
                }
-               for _, p1 := range load.PackageList(p.Internal.Imports) {
+               for _, p1 := range p.Internal.Imports {
                        deps = append(deps, b.vetAction(mode, depMode, p1))
                }

After rebuilding toolchain with make.bash, all tests in ../bin/go test cmd/vet are still passing.

./all.bash is passing.

All tests in master branch of golang.org/x/tools/go/analysis repository are also passing.

Tried running go vet -x and manually checking vet.cfg. PackageFile and PackageVetx match, except for artificially inserted fmt.

{
	"ID": "bytes",
	"Compiler": "gc",
	"Dir": "/home/prime/Code/golang/src/bytes",
	"ImportPath": "bytes",
	"GoFiles": [
		"/home/prime/Code/golang/src/bytes/buffer.go",
		"/home/prime/Code/golang/src/bytes/bytes.go",
		"/home/prime/Code/golang/src/bytes/reader.go",
		"/home/prime/Code/golang/src/bytes/export_test.go"
	],
	"NonGoFiles": [],
	"ImportMap": {
		"errors": "errors",
		"fmt": "fmt",
		"internal/bytealg": "internal/bytealg",
		"io": "io",
		"unicode": "unicode",
		"unicode/utf8": "unicode/utf8"
	},
	"PackageFile": {
		"errors": "/home/prime/Code/golang/pkg/linux_amd64/errors.a",
		"fmt": "/home/prime/Code/golang/pkg/linux_amd64/fmt.a",
		"internal/bytealg": "/home/prime/Code/golang/pkg/linux_amd64/internal/bytealg.a",
		"io": "/home/prime/Code/golang/pkg/linux_amd64/io.a",
		"unicode": "/home/prime/Code/golang/pkg/linux_amd64/unicode.a",
		"unicode/utf8": "/home/prime/Code/golang/pkg/linux_amd64/unicode/utf8.a"
	},
	"Standard": {
		"errors": true,
		"fmt": true,
		"internal/bytealg": true,
		"io": true,
		"unicode": true,
		"unicode/utf8": true
	},
	"PackageVetx": {
		"errors": "/home/prime/.cache/go-build/e3/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855-d",
		"internal/bytealg": "/home/prime/.cache/go-build/e3/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855-d",
		"io": "/home/prime/.cache/go-build/e3/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855-d",
		"unicode": "/home/prime/.cache/go-build/e3/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855-d",
		"unicode/utf8": "/home/prime/.cache/go-build/e3/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855-d"
	},
	"VetxOnly": false,
	"VetxOutput": "$WORK/b001/vet.out",
	"SucceedOnTypecheckFailure": false
}
@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Feb 26, 2019

Thanks. The fix may be small, but I couldn't find it in 10 minutes. :)

Please send a CL to @bcmills or @rsc as they know this code.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Feb 28, 2019

Change https://golang.org/cl/164459 mentions this issue: cmd/go: PackageVetx in vet.cfg should list only immediate dependencies.

@slon

This comment has been minimized.

Copy link
Contributor Author

slon commented Apr 4, 2019

@alandonovan would you please assist me with getting this change merged?

Looks like my CL was not done properly, since it is still open without reply :(

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Apr 16, 2019

@slon, thanks for the fix. Is there anything remaining to do here, or should we close this issue?

gopherbot pushed a commit that referenced this issue Apr 16, 2019

cmd/go: PackageVetx in vet.cfg should list only immediate dependencies.
Updates #30296

Change-Id: Ifea1a4c82c1c5b31fdc2e96fdbb1274748c8f50e
Reviewed-on: https://go-review.googlesource.com/c/go/+/164459
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@slon

This comment has been minimized.

Copy link
Contributor Author

slon commented Apr 17, 2019

We're done.

Thank you.

@slon slon closed this Apr 17, 2019

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.