x/tools/gopls: evaluation of build tags used in imported packages must be optional

Zyl9393 commented 1 month ago

Go version

go1.22.3 windows/amd64 gopls v0.16.1

Output of `go env` in your module/workspace:

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Zyl\AppData\Local\go-build
set GOENV=C:\Users\Zyl\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=D:\Projects\Code\Go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=D:\Projects\Code\Go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.22.3
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=D:\Projects\MyProject\src\client\go.mod
set GOWORK=D:\Projects\MyProject\src\go.work
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\Zyl\AppData\Local\Temp\go-build383807685=/tmp/go-build -gno-record-gcc-switches

What did you do?

While dealing with endianness, I introduced build tags to a package which has its own go.mod. After the change, one file would contain

//go:build 386 || amd64 || amd64p32 || alpha || arm || arm64 || loong64 || mipsle || mips64le || mips64p32le || nios2 || ppc64le || riscv || riscv64 || sh || wasm

and another would contain

//go:build armbe || arm64be || m68k || mips || mips64 || mips64p32 || ppc || ppc64 || s390 || s390x || shbe || sparc || sparc64

As a consequence, all code importing this package is now evaluated for GOOS=aix with GOARCH=ppc64, causing a large amount of problems to be marked, making me lose of track of actual problems.

Additionally, type redeclarations are detected in spite of the build tags being disjoint. (?)

To fix it, I specified "go.buildTags": "amd64" in .vscode/settings.json.

What did you see happen?

Compilation errors for architectures other than amd64 are still being reported.

What did you expect to see?

That gopls would no longer report compilation errors for architectures other than amd64.

gabyhelp commented 1 month ago

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

findleyr commented 1 month ago

Additionally, type redeclarations are detected in spite of the build tags being disjoint. (?)

This should not happen. Perhaps for the particular set of tags selected, there really are redeclarations?

And sorry for the poor experience here. This should be improved.

Generally, I think we should consider (1) restricting diagnostics for "dynamic" build tags only to open packages, and (2) implementing #65757 to specify a fixed set of builds (which would implicitly disable dynamic build selection.

Zyl9393 commented 1 month ago

Perhaps for the particular set of tags selected, there really are redeclarations?

No. I took direct inspiration from encoding/binary/native_endian_big.go and encoding/binary/native_endian_little.go, which have the same build tags respectively. There are only two declarations of the offending type; one in each file: After removing all the build tags in both files and all code in one of the two files, saving, putting it all back, and saving again, this particular issue was gone. Does gopls manage some sort of on-disk cache? Restarting it did not fix the issue, but this comment/uncomment maneuver did.

only to open packages

I should probably clarify that the package causing the issue is indeed opened within the troubled workspace. I have plans to move it elsewhere following more progress on it; I reckon the order of doing these things should not be dictated by assistive tooling.

Can you help me understand why setting go.buildTags would be expected to be ineffective in this scenario?

hyangah commented 1 month ago

Does gopls manage some sort of on-disk cache? Restarting it did not fix the issue, but this comment/uncomment maneuver did.

Yes, gopls maintains on-disk cache.

why setting go.buildTags would be expected to be ineffective in this scenario?

I can guess two reasons: 1) go.buildTags is passed to go command's -tags flag value. Even though platform-specific source selection is controlled with the //go:build statement like other tags, os and arch are special and expected to be controlled with GOOS and GOARCH env vars. Passing os/arch values to -tags may look working but that's a coincidence. It can eventually cause issues. See #45488. 2) Since v0.15.0, gopls tries to determine GOOS/GOARCH heuristically to process open source files. See https://github.com/golang/tools/releases/tag/gopls%2Fv0.15.0 Specifying os/arch using go.buildTags can potentially confuse gopls.

@findleyr Why/how gopls happened to pick up GOOS=aix and GOARCH=ppc64, given those build tags?

golang / go