cmd/compile: go1.27.0 unified export data loses method order, breaking objectpath, crashing staticcheck

[bradfitz]

We just started converting Tailscale's internal codebase to use Go 1.27's generic methods and a few rounds of conversion went well, and then we hit CI failures with staticcheck crashing on us:

panic: runtime error: index out of range [1] with length 1

goroutine 2938 [running]:
honnef.co/go/tools/analysis/facts/nilness.(*Result).Nilness(0x13ab31d515b0, 0x13ab2daf6480, 0x1)
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/analysis/facts/nilness/nilness.go:77 +0x10f
honnef.co/go/tools/staticcheck/sa4023.run(0x13ab352e2e00)
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/staticcheck/sa4023/sa4023.go:181 +0x6bf
honnef.co/go/tools/lintcmd/runner.(*analyzerRunner).do(0x13ab3b1122d0, {0xee2f70?, 0x13ab3b108be0})
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/lintcmd/runner/runner.go:989 +0x722
honnef.co/go/tools/lintcmd/runner.genericHandle({0xee2f70, 0x13ab3b108be0}, {0xee2f70?, 0x13ab27e12be0?}, 0x13ab382ed6c0, 0x0, 0x13ab3b5594d0)
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/lintcmd/runner/runner.go:814 +0x111
honnef.co/go/tools/lintcmd/runner.(*subrunner).runAnalyzers(0x13ab270e68a0, 0x13ab27262b40, 0x13ab36be0280)
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/lintcmd/runner/runner.go:1062 +0x73d
honnef.co/go/tools/lintcmd/runner.(*subrunner).doUncached(0x13ab270e68a0, 0x13ab27262b40)
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/lintcmd/runner/runner.go:717 +0x22d
honnef.co/go/tools/lintcmd/runner.(*subrunner).do(0x13ab270e68a0, {0xee2fb8?, 0x13ab27262b40})
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/lintcmd/runner/runner.go:574 +0xa72
honnef.co/go/tools/lintcmd/runner.(*Runner).Run.func2({0xee2fb8?, 0x13ab27262b40?})
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/lintcmd/runner/runner.go:1224 +0x2a
honnef.co/go/tools/lintcmd/runner.genericHandle({0xee2fb8, 0x13ab27262b40}, {0xee2fb8?, 0x13ab28b54120?}, 0x13ab26faa230, 0x13ab272ba158, 0x13ab26fa4700)
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/lintcmd/runner/runner.go:814 +0x111
created by honnef.co/go/tools/lintcmd/runner.(*Runner).Run in goroutine 1
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.0-rc.1/lintcmd/runner/runner.go:1223 +0x41f

I noticed we'd been using an old version (v0.8.0-rc.1) so I figured it might already be fixed in a newer one. I had to put the kids to bed so I told my trusty agent to bump staticcheck and see if that fixes it.

Instead, I came back to my computer and found it made this diagnosis & minimal repro: https://github.com/bradfitz/bug-go127-staticcheck-crash

$ git clone https://github.com/bradfitz/bug-go127-staticcheck-crash.git
$ cd bug-go127-staticcheck-crash
$ go tool staticcheck ./...
panic: runtime error: index out of range [1] with length 1

goroutine 169 [running]:
honnef.co/go/tools/analysis/facts/nilness.(*Result).Nilness(0x3f1e0842a220, 0x3f1e07ea1620, 0x1)
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.1/analysis/facts/nilness/nilness.go:77 +0x10f
honnef.co/go/tools/staticcheck/sa4023.run(0x3f1e0873d5e0)
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.1/staticcheck/sa4023/sa4023.go:181 +0x6bf
honnef.co/go/tools/lintcmd/runner.(*analyzerRunner).do(0x3f1e08017d70, {0xedfd38?, 0x3f1e0849bc20})
	/home/ubuntu/go/pkg/mod/honnef.co/go/tools@v0.8.1/lintcmd/runner/runner.go:989 +0x722
...

Something seems unhappy in the export data or go/types or their interaction with the new generic methods.

But for now we have to choose between using staticcheck and using generic methods.

/cc @golang/compiler @golang/tools-team @dominikh

[mrkfrmn]

Related to #61443 and cc @adonovan.

Generic methods are placed after ordinary methods in the export data. It seems this broke the invariant that export data must preserve method order. Thus, the same objectpath might refer to different methods depending on whether a type with generic methods came from source or export data.

The obvious solution is to bump the export data version to preserve ordering, but we'd need to either gate the new version on a minor release or wait to do that in Go 1.28, which seems untenable.

[adonovan]

Thanks for investigating. This is a nice illustration of one of the benefits of no longer using compiler export data in x/tools (#79427). It occurs to me that unitchecker could (indeed should) export type-export data too (not just facts), much like cmd/export does, then the build tool (e.g. go vet) wouldn't need to execute the compiler (or cmd/export in future) at all: it's just a redundant step. Let's discuss when you're back in the office. That's orthogonal to this issue though.

I think this bug may need a backported patch to cmd/compile in 1.27 since it breaks x/tools/go/packages.

[adonovan]

https://go.dev/cl/824004 is the unitchecker change mentioned above (though it only benefits the next Go release).

[gopherbot]

Change https://go.dev/cl/824004 mentions this issue: go/analysis/unitchecker: export type information

[adonovan]

The obvious solution is to bump the export data version to preserve ordering, but we'd need to either gate the new version on a minor release or wait to do that in Go 1.28, which seems untenable.

I agree that it can't be fixed without a protocol change as the necessary ordering information is lost. (Tempting though it is, we can't reliably reconstruct the ordering from each method's (filename, line, column) triple, because files are only guaranteed to be presented to the compiler in name order when using go build; other build systems such as Bazel compile files in the order they are enumerated in the BUILD file.)

So, we'll need to:

• bump the protocol version so that method index is explicit
• update the "u" decoder in x/tools
• release x/tools
• update the "u" encoder in cmd/compile
• backport the compiler change and re-release.

Staticcheck will need to update its x/tools requirement and be re-released so that it will understand the files produced by the fixed compiler (instead of giving a "ureader too old" error).

Sigh. #79427 cannot come soon enough.

cc: @griesemer @mrkfrmn @dominikh

[gopherbot]

Change https://go.dev/cl/826064 mentions this issue: internal/gcimporter: support V5 unified encoding (method indices)