If you remove the blank line, it is not a build tag anymore, but just a comment (package comment).

Smells like some go/types issue.

@alandonovan, @griesemer ?

That's very weird. I don't see why go/types even cares about _test.go files in this case. Not a release blocker though.

Thanks for tracking this down; your analysis so far is correct: go/types does indeed see two versions of the same named type (crypto/cipher.Block). In go/types, named types that are identical must be represented with identical objects (i.e., identical pointers). In this case, this type is imported (via different paths), and the respective type objects are not identical (hence the failure). The reason why they are not identical is that the containing package (crypto/cipher) appears in two (structurally identical, but different) data structures.

Thus, the issue is only indirectly related to go/types; the real issue is that the importer provided to go/types, in this case Walker.Import defined in cmd/api/goapi.go provides two different (yet structurally identical) *types.Package objects to go/types for the same package crypto/cipher.

This is a caching bug and it disappears when usePkgCache (in goapi.go) is set to false. In fact this appears to be the very same bug as #8425, which was closed 4 years ago as "unfortunate".

Adding the foo_test.go file (with the respective build tag) to crypto/cipher extends the set of tags for this package which are considered, and now we see two different package keys ("/Users/gri/go/src/crypto/cipher" vs "/Users/gri/go/src/crypto/cipher,amd64" in my case) leading two two different entries in the package cache pkgCache) which then are used together.

Why they are used together I don't know yet. Will investigate.

It doesn't matter if the added file is a test file or not; we can also add a file foo.go belonging to the package with the respective build tag. All that matters is the existence of the build tag.

Simpler reproducer, after adding the foo_test.go file as described in the issue:

1. cd cmd/api
2. go build
3. ./api -contexts darwin-386,windows-amd64 crypto/x509

This only tests the API for 2 configurations: darwin-386 and windows-amd64. The problem is that we see two different versions of crypto/cipher. API checking a package requires that the package is imported (to get its API). The specifics are as follows.

First, in darwin-386 context:

1. (crypto/)x509 is imported
2. x509 imports (crypto/)des which imports cipher, and which indirectly exports cipher.Block (via des.NewCipher).
3. x509 also imports cipher directly

Second, in windows-amd64 context:

1. (crypto/)x509 is imported
2. x509 imports des, but the package cache is hit and we get the cached version of crypto/des, which includes the re-exported crypto/cipher.Block
3. x509 also imports cipher directly, but this time the package cache is not hit but instead a new version of the cipher package is created.

In x509, the struct rfc1423Algo contains the field cipherFunc func(key []byte) (cipher.Block, error) which refers to the directly imported cipher.Block. But on line 49, the value des.NewCipher imported via cipher/des is used. The two types, while identical, are from different packages objects (for the same package), and thus are not unified. As a result we get a type error during assignment.

If crypto/des is excluded from package caching, the issue disappears.

There are two package caches involved: The import cache that is used by importers (and cleared for each context), and the global package cache, keyed by package directory and tags. The import cache must ensure that each package (identified by package path) exists exactly once. When an object (in this case cipher.Block) is imported indirectly (via the import of crypto/des), the type cipher.Block must be unified with an already existing version of the type which is under the previously imported cipher package. Vice versa, package cipher is imported and an indirectly imported version of cipher.Block (and corresponding partial cipher package) already exists, that existing package structure must be used and extended.

Because there's a second, external cache involved here things fall apart: When x509 imports des, the package cache hits and the des package is used (and added to the import package cache). But the cipher/des package contains types such as the signature of des.NewCipher which refers to cipher.Block which refers to the cipher package. That package is not added to the import package cache (step 3 above for the windows-amd64 context). Thus, when x509 imports cipher directly, it is added a 2nd time. Hence the bug.

Possible solutions:

1. disable the global package cache (simple, but costs significant execution time); perhaps do this only if we run into a problem
2. when the global package cache hits, traverse all imported objects and types and unify types as needed (rather difficult to get right)
3. explicitly exclude packages that cause problems from the cache (via some blacklist in the code) (may or may not work as code changes)
4. don't do anything and hope this doesn't happen for the existing repo (trial and error)

Probably not for 1.11.

PS: Another option might be to make go/types not depend on the invariant that defined (*types.Named) types are represented uniquely. The implementation could choose structural comparison where pointer comparison fails. This would come at the cost of larger (duplicated) data structures and (minimally) slower type-checking, but only if a program makes use of it.

(The structural comparison would kick in whenever pointer equivalence for such defined types fails, but in correct programs that would not happen.)

cc: @alandonovan for comments.

Another option might be to make go/types not depend on the invariant that defined (*types.Named) types are represented uniquely. The implementation could choose structural comparison where pointer comparison fails. This would come at the cost of larger (duplicated) data structures and (minimally) slower type-checking, but only if a program makes use of it.

This would be a breaking change to the go/types API, since tons of client code depends on the canonicality of types.Named variables.

The only correct solution on your list is the first one, to disable the global cache. Another correct (though more complicated) solution would be to extend the tagKey mechanism transitively so that you only get cache hits for packages that are transitively compatible. However, most packages depend on syscall or something that varies by build tags, so this would probably reduce the hit rate significantly.

https://go-review.googlesource.com/c/go/+/125316 also triggers this

Any update on this bug?

@mengzhuo No. Does this affect you?

@griesemer, it's causing TryBot failures for bizarre reasons on seemingly-unrelated CLs: https://go-review.googlesource.com/c/go/+/125316

Change https://golang.org/cl/138315 mentions this issue: cmd/api: explicit tagKey with GOOS and GOARCH