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: wrong package version used to vet orphaned code (?) #45678

Closed
bobg opened this issue Apr 21, 2021 · 5 comments
Closed

cmd/vet: wrong package version used to vet orphaned code (?) #45678

bobg opened this issue Apr 21, 2021 · 5 comments

Comments

@bobg
Copy link

@bobg bobg commented Apr 21, 2021

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

$ go version
go version go1.16.3 darwin/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
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/bobglickstein/Library/Caches/go-build"
GOENV="/Users/bobglickstein/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/bobglickstein/go/pkg/mod"
GONOPROXY=""
GOOS="darwin"
GOPATH="/Users/bobglickstein/go"
GOPRIVATE=""
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.3"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/bobglickstein/src/bobg/repo/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/mp/8ycgyr3j64s42ym9lbl9cqdh0000gn/T/go-build1894730681=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have a project that depends on code in go.uber.org/cadence at version v0.10.5. I ran go vet on my code and got impossible-seeming output. See "Speculation," below.

What did you expect to see?

Nothing.

What did you see instead?

go.uber.org/cadence/internal/common
/root/go/pkg/mod/go.uber.org/cadence@v0.10.5/internal/common/thrift_util.go:29:38: not enough arguments in call to thrift.NewTSerializer().Write
	have (thrift.TStruct)
	want (context.Context, thrift.TStruct)
/root/go/pkg/mod/go.uber.org/cadence@v0.10.5/internal/common/thrift_util.go:51:27: not enough arguments in call to t.Protocol.Flush
	have ()
	want (context.Context)
/root/go/pkg/mod/go.uber.org/cadence@v0.10.5/internal/common/thrift_util.go:55:28: not enough arguments in call to t.Transport.Flush
	have ()
	want (context.Context)

Speculation

This output seems wrong because the version of thrift that cadence v0.10.5 depends on does not include those context.Context parameters.

Here is the go.mod line from cadence v0.10.5 specifying a version of thrift: link
Here is one call in cadence v0.10.5 that go vet erroneously claims is wrong: link
Here is the declaration in thrift, at the version requested by cadence, showing that the call is in fact OK: link

Later versions of thrift do add a context.Context parameter to those calls, e.g. link (but without assigning a new major version number, naughty naughty).

However, the code that go vet complains about is orphaned. Nothing in the cadence module calls it. And since it's in an internal tree, nothing outside cadence can call it.

It therefore seems as if go vet is using the version of thrift that my module would want (i.e., the latest version, if my module wanted thrift) and, perhaps because the code in question is orphaned, ignoring the version of thrift that cadence has asked for.

Note, I reproduced this with clean Go mod and build caches and with various settings for GOPROXY.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Apr 21, 2021

which version of thrift is actually used? just because cadence asks for v0.10.5 doesn't mean it will be the version selected if other dependencies or you declare the need for a higher version.

output of or find the line in your go.mod

go list -mod=readonly -m github.com/apache/thrift
@bobg
Copy link
Author

@bobg bobg commented Apr 21, 2021

$ go list -mod=readonly -m github.com/apache/thrift
github.com/apache/thrift v0.13.0

Also, in case it wasn't clear, my module has no direct dependency on thrift.

$ go mod why github.com/apache/thrift
# github.com/apache/thrift
(main module does not need package github.com/apache/thrift)
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Apr 21, 2021

right now everything reachable, including unused things, participates in version selection. #36460 seeks to solve that.

try go mod graph | grep github.com/apache/thrift to find which dependencies declare a need for it

@bobg
Copy link
Author

@bobg bobg commented Apr 21, 2021

Ah, interesting, thanks. Following #36460.

$ go mod graph | grep github.com/apache/thrift
contrib.go.opencensus.io/exporter/jaeger@v0.1.0 github.com/apache/thrift@v0.12.0
go.uber.org/cadence@v0.10.5 github.com/apache/thrift@v0.0.0-20161221203622-b2a4d4ae21c7
go.uber.org/yarpc@v1.49.1 github.com/apache/thrift@v0.13.0
go.opencensus.io@v0.19.2 github.com/apache/thrift@v0.12.0
go.opencensus.io@v0.20.1 github.com/apache/thrift@v0.12.0
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Apr 22, 2021

So it is correctly selecting the highest version (v0.13.0) of thrift (since a module can only exist at a single version).

Closing as working as intended.

@seankhliao seankhliao closed this Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants