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
x/tools: nilness panics "no concrete method" #58296
Comments
Adding
|
Found a minimal test case.
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
)
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)
}
|
CC @golang/tools-team |
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
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 |
Thank you for the minimal reproducer. Here is an even more minimal reproducer:
with the go.mod
|
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? |
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. |
So if I change |
Sorry, I only skimmed this issue, and didn't see Tim's comment about go list. |
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.) |
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. |
Using Tim's reduced testcase, the import graph of interest is
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.) |
Even more minimal testcase: Target program:
SSA loader program: https://go.dev/play/p/CUqfPBiomIs The import graph is [Update: This analysis is one of many wrong hypotheses I have come up with today.] |
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 By contrast, the go1.19 reader would set 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. |
@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 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. |
@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.) |
Change https://go.dev/cl/465936 mentions this issue: |
Change https://go.dev/cl/467896 mentions this issue: |
This CL is a port of go.dev/cl/465936 from the x/tools importer, which changes the unified importer to (1) only call Package.SetImports on the main package being imported (not any transitively imported packages), and (2) to only populate it with any packages that were referenced by the exported API. With these changes, it should behave identically to how the indexed importer worked in Go 1.19. It will also allow eventually dropping the serialized import DAG from the export data format, which should help with export data file sizes somewhat. Updates #54096. Updates #58296. Change-Id: I70d252a19cada3333ed59b16d1df2abc5a4cff73 Reviewed-on: https://go-review.googlesource.com/c/go/+/467896 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
This CL is a port of go.dev/cl/465936 from the x/tools importer, which changes the unified importer to (1) only call Package.SetImports on the main package being imported (not any transitively imported packages), and (2) to only populate it with any packages that were referenced by the exported API. With these changes, it should behave identically to how the indexed importer worked in Go 1.19. It will also allow eventually dropping the serialized import DAG from the export data format, which should help with export data file sizes somewhat. Updates golang#54096. Updates golang#58296. Change-Id: I70d252a19cada3333ed59b16d1df2abc5a4cff73 Reviewed-on: https://go-review.googlesource.com/c/go/+/467896 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
The latest release of nilness panics when used with Go 1.20. See golang/go#58296. This was fixed, but hasn't been released yet, which is why we pull from master.
What version of Go are you using (
go version
)?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
: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
OutputWhat 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 ofgoogle.golang.org/protobuf
but the exact package and method varies. So far we've seen it on:A full panic
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 itsgo.mod
isgoogle.golang.org/protobuf v1.26.0 // indirect
.The text was updated successfully, but these errors were encountered: