golang/go

x/tools: nilness panics "no concrete method"

joeshaw opened this issue · 19 comments

joeshaw commented

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

$ go version
go version go1.20 linux/amd64

Does this issue reproduce with the latest release?

Yes, having installed with go install golang.org/x/tools/go/analysis/passes/nilness/cmd/nilness@latest:

go version -m $(which nilness)
/go/bin/nilness: go1.20
	path	golang.org/x/tools/go/analysis/passes/nilness/cmd/nilness
	mod	golang.org/x/tools	v0.5.0	h1:+bSpV5HIeWkuvgaMfI3UmKRThoTA5ODJTUd8T17NO+4=
	dep	golang.org/x/mod	v0.7.0	h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA=
	dep	golang.org/x/sys	v0.4.0	h1:Zr2JFtRQNX3BCZ8YtxRE9hNJYC8J6I1MVbMg6owUp18=
	build	-buildmode=exe
	build	-compiler=gc
	build	CGO_ENABLED=1
	build	CGO_CFLAGS=
	build	CGO_CPPFLAGS=
	build	CGO_CXXFLAGS=
	build	CGO_LDFLAGS=
	build	GOARCH=amd64
	build	GOOS=linux
	build	GOAMD64=v1

I've also tried with @master (golang.org/x/tools v0.5.1-0.20230202234227-811111804389)

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

I am on an Intel Mac, but I am running this in a container in a Linux VM. This also happens in a container running on Linux directly.

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/tmp/test/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2170370967=/tmp/go-build -gno-record-gcc-switches"

What did you do?

As part of a CI process, we run nilness ./... over a project's sources. For three different applications we hit this panic. In all three cases they are panicking on a method inside of google.golang.org/protobuf but the exact package and method varies. So far we've seen it on:

func (google.golang.org/protobuf/encoding/protowire.Number).IsValid() bool
func (google.golang.org/protobuf/reflect/protoreflect.Cardinality).GoString() string
A full panic 
nilness ./...
panic: no concrete method: func (google.golang.org/protobuf/reflect/protoreflect.Cardinality).GoString() string

goroutine 2017 [running]:
golang.org/x/tools/go/ssa.(*Program).declaredFunc(0xc002032000, 0xc00158df80)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:149 +0xf9
golang.org/x/tools/go/ssa.(*Program).originFunc(0xc002549450?, 0xc004828868?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/source.go:193 +0x32
golang.org/x/tools/go/ssa.(*Program).addMethod(0xc002032000, 0xc0044248c0, 0xc002549450, 0x71b0e0?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:109 +0x18c
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8088?, 0xc0012fbf80?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:200 +0x705
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc00037b380?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8088?, 0xc0012fbf10?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:209 +0x1f7
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc00037be30?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8088?, 0xc0012fbea0?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:208 +0x1d0
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc000c595d8?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f80d8?, 0xc000fb3e40?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:209 +0x1f7
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc000c59728?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f80d8?, 0xc000fb3fb0?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:209 +0x1f7
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc000c59818?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f80d8?, 0xc000e28a10?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:209 +0x1f7
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc000c59e78?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f80d8?, 0xc000e28a90?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:209 +0x1f7
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc000c59ef0?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8088?, 0xc0012fb260?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:208 +0x1d0
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8010?, 0xc000c59f50?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:226 +0x30f
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc000c59f68?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8088?, 0xc0012fb1f0?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:208 +0x1d0
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8178?, 0xc001534ab0?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f80d8?, 0xc0041f40c0?}, 0x0, 0x9ad620?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:208 +0x1d0
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc002032000, {0x7f8088?, 0xc0019b4690?}, 0x0, 0xc0008178c0?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:242 +0x458
golang.org/x/tools/go/ssa.(*Program).needMethodsOf(0xc002032000, {0x7f8088?, 0xc0019b4690?}, 0x624c508726b89f64?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/methods.go:172 +0x7f
golang.org/x/tools/go/ssa.(*Package).build(0xc0041e4600)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/builder.go:2501 +0x13f
sync.(*Once).doSlow(0xc002032000?, 0xc0012d7810?)
	/opt/go/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
	/opt/go/src/sync/once.go:65
golang.org/x/tools/go/ssa.(*Package).Build(...)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/ssa/builder.go:2477
golang.org/x/tools/go/analysis/passes/buildssa.run(0xc002030000)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/analysis/passes/buildssa/buildssa.go:72 +0x1a8
golang.org/x/tools/go/analysis/internal/checker.(*action).execOnce(0xc0017268c0)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/analysis/internal/checker/checker.go:725 +0x9ec
sync.(*Once).doSlow(0x0?, 0x0?)
	/opt/go/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
	/opt/go/src/sync/once.go:65
golang.org/x/tools/go/analysis/internal/checker.(*action).exec(0x0?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/analysis/internal/checker/checker.go:641 +0x3d
golang.org/x/tools/go/analysis/internal/checker.execAll.func1(0x0?)
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/analysis/internal/checker/checker.go:629 +0x25
created by golang.org/x/tools/go/analysis/internal/checker.execAll
	/go/pkg/mod/golang.org/x/tools@v0.5.0/go/analysis/internal/checker/checker.go:635 +0x165

I haven't yet been able to reduce it to a sharable test case. A minimal test case is below. Simply using those types above doesn't trigger it.

In the application that panics with func (google.golang.org/protobuf/reflect/protoreflect.Cardinality).GoString() string, in its go.mod is google.golang.org/protobuf v1.26.0 // indirect.

joeshaw commented

Adding -debug fpstv doesn't yield much more info:

16:45:40.851633 load [./...]
16:45:50.913971 building graph of analysis passes
panic: no concrete method: func (google.golang.org/protobuf/reflect/protoreflect.Cardinality).GoString() string
[...]
joeshaw commented

Found a minimal test case.

go.mod:

module test

go 1.20

require github.com/prometheus/client_golang v1.12.2

require (
	github.com/beorn7/perks v1.0.1 // indirect
	github.com/cespare/xxhash/v2 v2.1.2 // indirect
	github.com/golang/protobuf v1.5.2 // indirect
	github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
	github.com/prometheus/client_model v0.3.0 // indirect
	github.com/prometheus/common v0.37.0 // indirect
	github.com/prometheus/procfs v0.8.0 // indirect
	golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect
	google.golang.org/protobuf v1.26.0 // indirect
)

main.go:

package main

import "github.com/prometheus/client_golang/prometheus/promhttp"
import "net/http/httptest"

func main() {
	w := httptest.NewRecorder()
	req := httptest.NewRequest("GET", "/", nil)

	h := promhttp.Handler()
	h.ServeHTTP(w, req)
}

nilness ./...:

panic: no concrete method: func (*crypto/x509/pkix.CertificateList).HasExpired(now time.Time) bool

goroutine 1146 [running]:
golang.org/x/tools/go/ssa.(*Program).declaredFunc(0xc000334540, 0xc0002ea780)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:149 +0xf9
golang.org/x/tools/go/ssa.(*Program).originFunc(0xc00081db80?, 0xc000290b70?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/source.go:193 +0x32
golang.org/x/tools/go/ssa.(*Program).addMethod(0xc000334540, 0xc00086ca40, 0xc00081db80, 0x7204e0?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:109 +0x18c
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe898?, 0xc00005ab80?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:200 +0x705
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe938?, 0xc0004978f0?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe898?, 0xc00005ac10?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:208 +0x1d0
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe8e8?, 0xc00005ac30?}, 0x0, 0x9b6768?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:223 +0x2d7
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe910?, 0xc0002e0570?}, 0x1, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:255 +0x617
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe848?, 0xc0000c4fc0?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:248 +0x487
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe938?, 0xc000497a58?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe8c0?, 0xc0003745c0?}, 0x0, 0x9b6768?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:236 +0x3bc
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe910?, 0xc0002e0630?}, 0x1, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:255 +0x617
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe848?, 0xc0000c42a0?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:248 +0x487
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe898?, 0xc00005ae80?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:220 +0x29b
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe938?, 0xc000497ae8?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe848?, 0xc0000c4150?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:208 +0x1d0
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe938?, 0xc0000ab1e8?}, 0x0, 0x9b6760?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:260 +0x5ae
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000334540, {0x7fe898?, 0xc000318450?}, 0x0, 0x1?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:209 +0x1f7
golang.org/x/tools/go/ssa.(*Program).needMethodsOf(0xc000334540, {0x7fe898?, 0xc000318450?}, 0xc0005240f0?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/methods.go:172 +0x7f
golang.org/x/tools/go/ssa.(*builder).needsRuntimeTypes(0xc000291a40)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/builder.go:2439 +0x36d
golang.org/x/tools/go/ssa.(*Package).build(0xc00039d080)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/builder.go:2607 +0xc93
sync.(*Once).doSlow(0xc000334540?, 0xc000871180?)
	/opt/go/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
	/opt/go/src/sync/once.go:65
golang.org/x/tools/go/ssa.(*Package).Build(...)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/ssa/builder.go:2477
golang.org/x/tools/go/analysis/passes/buildssa.run(0xc0003263c0)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/analysis/passes/buildssa/buildssa.go:72 +0x1a8
golang.org/x/tools/go/analysis/internal/checker.(*action).execOnce(0xc000436320)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/analysis/internal/checker/checker.go:763 +0x9ec
sync.(*Once).doSlow(0xc00063b778?, 0x6bc760?)
	/opt/go/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
	/opt/go/src/sync/once.go:65
golang.org/x/tools/go/analysis/internal/checker.(*action).exec(0x43f265?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/analysis/internal/checker/checker.go:679 +0x3d
golang.org/x/tools/go/analysis/internal/checker.execAll.func1(0x0?)
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/analysis/internal/checker/checker.go:667 +0x25
created by golang.org/x/tools/go/analysis/internal/checker.execAll
	/go/pkg/mod/golang.org/x/tools@v0.5.1-0.20230202234227-811111804389/go/analysis/internal/checker/checker.go:673 +0x165
mknyszek commented

CC @golang/tools-team

adonovan commented

Many thanks @joeshaw for taking the time to find a minimal testcase; I confirm that it reproduces the panic for me at tip today.

A brief investigation makes me suspect the bug is in this code in the buildssa analyzer. It tries to construct an SSA package for every package in the transitive closure of the target package:

func run(pass *analysis.Pass) (interface{}, error) {
...
	// Create SSA packages for all imports.
	// Order is not significant.
	created := make(map[*types.Package]bool)
	var createAll func(pkgs []*types.Package)
	createAll = func(pkgs []*types.Package) {
		for _, p := range pkgs {
			if !created[p] {
				created[p] = true
				prog.CreatePackage(p, nil, nil, true) // <--here
				createAll(p.Imports())
			}
		}
	}
	createAll(pass.Pkg.Imports())

but this traversal over the Imports graph is not guaranteed to reach all dependencies if some were loaded from export data. I notice that setting cfg.Mode |= NeedDeps in packages.Load is an effective workaround, since it causes all packages to be loaded from syntax (at great expense).

timothy-king commented

Thank you for the minimal reproducer. Here is an even more minimal reproducer:

package main

import "net/http"

func main() {
	var h any = http.Header{}
	_ = h
}

with the go.mod

module test

go 1.20
timothy-king commented

My investigation seems to agree with Alan's. For the types.Package for "crypto/tls", the Imports() is is empty, and no types.Package is importing "crypto/x509". So no types.Package for "crypto/x509/pkix". So no ssa.Package for "crypto/x509/pkix". Hence the panic for the missing method "(*crypto/x509/pkix.CertificateList).HasExpired".

Alan do you recall if something is being more conservative during package loading than it used to?

adonovan commented

Alan do you recall if something is being more conservative during package loading than it used to?

No, I suspect there has always been a latent problem here with packages loaded from export data.

I encountered a very similar problem in another context--gopls--this morning, which may explain why I noticed the problem so quickly. In that case the solution was to tabulate the mapping from PackagePath to *types.Package during type checking and to use that map instead of the types.Packages.Imports graph. But there's no way to do that in the analysis framework.

Perhaps the best solution would be to create the non-syntactic ssa.Packages on demand as they are referenced or needed (most aren't). This would avoid the need for the logic in buildssa (it would be harmless). By good fortune, the public API for Program would seem to allow packages to be created at any time (need a mutex of course). It would also reduce memory usage.

timothy-king commented

So if I change cmd := exec.Command("go", goArgs...) to cmd := exec.Command("go1.19", goArgs...) in x/tools/internal/gocommand , the imported packages seem to include "crypto/x509" and "crypto/x509/pkix" and the panic goes away. The constant "go1.20" also fails [edit: so it looks like the issue is starting in 1.20]. I am not seeing anything relevant in the release notes about go list changing in 1.20.

findleyr commented

I suspect that the difference is the unified export data format. CC @mdempsky

Sorry, I only skimmed this issue, and didn't see Tim's comment about go list.

adonovan commented

Just because the bug appeared at 1.19 doesn't mean go list is at fault: it could just be that the reference structure of the program evolved and now there is an indirect reference to a package that cannot be reached by a chain of 'import' edges that is preserved by the export data, revealing the latent bug. (I have no evidence either way, just want to make sure we don't point the finger at the wrong component.)

mdempsky commented

FWIW, Go 1.20 changed how go/types.Package.Imports works, due to the change to unified IR.

Suppose: You import package P from export data. P imports A and B. B imports C. P's exported API references types from A and C, but not B.

Old behavior: P.Imports() would return {A, C}. {A,B,C}.Imports() would all return nil. (That is, P.Imports misses B, but also includes transitive dependency C; and B's import list is empty, as an incomplete package.)

New behavior: P.Imports() returns {A, B, C}. B.Imports() returns {C}. (That is, Imports now returns the complete transitive closure of all of its imports, for both the directly imported package and any incomplete transitively imported packages.)

See also #54096.

adonovan commented

Using Tim's reduced testcase, the import graph of interest is

     main
 ⟶ net/http
 ⟶ crypto/tls
 ⟶ crypto/x509
 ⟶ crypto/x509/pkix

I observe that the Packages.Imports list for net/http includes crypto/tls in all cases, but in go1.19 it also includes crypto/x509 and crypto/x509/pkix, whereas in go1.20 these two edges are missing. So if the intent of the new behavior is to include the complete transitive closure, that suggests there is a bug in the go1.20 exporter.

(But is that really the intent? It would seem to scale poorly: the export data for a package that depends on a vast tree of dependencies would have to name them all, even if it consists of nothing more than an init function and no public declarations.)

adonovan commented

Even more minimal testcase:

Target program:

xtools$ head $(find minim2 -type f)
==> minim2/minim2.go <==
package minim2

import "golang.org/x/tools/minim2/a"

var h any = new(a.A)

==> minim2/a/a.go <==
package a

import "golang.org/x/tools/minim2/b"

type A struct {
	b b.B
}

==> minim2/b/b.go <==
package b

import "golang.org/x/tools/minim2/c"

type B struct {
	c c.C
}

==> minim2/c/c.go <==
package c

type C int

func (C) f() {
}

SSA loader program: https://go.dev/play/p/CUqfPBiomIs

The import graph is minim2 -> a -> b -> c. As @mdempsky said, the Packages.Import list with go1.20 export data is transitively closed, so a (loaded from export data) has Imports of b and c. Those packages weren't loaded from export data since they are indirect dependencies and we didn't request that go/packages give us type information for them. But it's surprising that it gave us a non-nil Package with nil Package.Scope and Imports. That seems like a possible bug in its redaction of unrequested information. The inconsistency between what types you can reach from a and what types you can get from c's Scope is what causes the SSA builder to panic.

[Update: This analysis is one of many wrong hypotheses I have come up with today.]

adonovan commented

I suspect a bug in flattenImports in the go1.20 unified export data reader: it assumes that Packages.Imports of each of its direct dependencies is already set. But consider minim2 -> a -> b -> c where minim2 is loaded from syntax and a is loaded from export data, and b and c are never loaded as complete packages. (They are just receptacles for symbols created on demand.) When loading a, the Imports of b and c have not been set: they are just partial packages, so the only element in a's Imports list will be b, even though a references types defined in c.

By contrast, the go1.19 reader would set a.Imports = {b,c}.

The invariant we need is of course that the transitive closure of the Imports graph of any Package includes at least the types referenced by that Package. Given that we can't rely on b.Imports or c.Imports, I think the fix must necessarily involve computing reachability over a's Package, just as it used to.

mdempsky commented

@adonovan The list of imported packages is constructed here: https://cs.opensource.google/go/x/tools/+/master:internal/gcimporter/ureader_yes.go;drc=3cba5a847ff8ab8c232faf4e4310cb2939a835b4;l=265

And each returned package should already have SetImports (and flattenImports) called by time r.pkg() returns.

In your scenario, are {a,b,c} all being created/loaded from export data, or have some of them been prepopulated into the import map?

adonovan commented

And each returned package should already have SetImports (and flattenImports) called by time r.pkg() returns.
In your scenario, are {a,b,c} all being created/loaded from export data, or have some of them been prepopulated into the import map?

The import map is prepopulated by go/packages for every package except the one of interest, and they may be incomplete. The export data reader is allowed to add symbols to the incomplete packages already in the map (the whole thing is a critical section), but it can't assume that it created them all.

mdempsky commented

@adonovan Hm, okay. My impression was the import map could be prepopulated by other invocations of the same importer, but not that users could prepopulate it themselves. That would certainly explain the issue then.

So currently the unified importer short circuits if it sees a package present in the import map, and assumes its import list has been setup. We could probably easily change it to only short circuit when the package is present and has a non-empty import list. (This would mean some extra overhead for leaf packages, but that should be negligible.)

gopherbot commented

Change https://go.dev/cl/465936 mentions this issue: internal/gcimporter: compute imports for unified IR

  • 👍1
gopherbot commented

Change https://go.dev/cl/467896 mentions this issue: go/internal/gcimporter: restore Go 1.19 Package.SetImports behavior