gopls/internal/cache: add assertions for export data inconsistency

This change pushes the assertions more deeply into the type
checking logic so that we might have a chance of understanding
the recurring inconsistency problems reported by telemetry.

Updates golang/go#63822

Change-Id: I1371f7aa7104df0c55e055a11e9e6bb15219d79a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560795
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/cache/check.go

@@ -22,10 +22,10 @@ import (
 	"golang.org/x/mod/module"
 	"golang.org/x/sync/errgroup"
 	"golang.org/x/tools/go/ast/astutil"
-	"golang.org/x/tools/gopls/internal/file"
-	"golang.org/x/tools/gopls/internal/filecache"
 	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/cache/typerefs"
+	"golang.org/x/tools/gopls/internal/file"
+	"golang.org/x/tools/gopls/internal/filecache"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
@@ -610,21 +610,39 @@ func (b *typeCheckBatch) importPackage(ctx context.Context, mp *metadata.Package
 			if item.Path == string(mp.PkgPath) {
 				id = mp.ID
 				pkg = thisPackage
+
+				// debugging issues #60904, #64235
+				if pkg.Name() != item.Name {
+					// This would mean that mp.Name != item.Name, so the
+					// manifest in the export data of mp.PkgPath is
+					// inconsistent with mp.Name. Or perhaps there
+					// are duplicate PkgPath items in the manifest?
+					return bug.Errorf("internal error: package name is %q, want %q (id=%q, path=%q) (see issue #60904)",
+						pkg.Name(), item.Name, id, item.Path)
+				}
 			} else {
 				id = impMap[item.Path]
 				var err error
 				pkg, err = b.getImportPackage(ctx, id)
 				if err != nil {
 					return err
 				}
+
+				// We intentionally duplicate the bug.Errorf calls because
+				// telemetry tells us only the program counter, not the message.
+
+				// debugging issues #60904, #64235
+				if pkg.Name() != item.Name {
+					// This means that, while reading the manifest of the
+					// export data of mp.PkgPath, one of its indirect
+					// dependencies had a name that differs from the
+					// Metadata.Name
+					return bug.Errorf("internal error: package name is %q, want %q (id=%q, path=%q) (see issue #60904)",
+						pkg.Name(), item.Name, id, item.Path)
+				}
 			}
 			items[i].Pkg = pkg
 
-			// debugging issue #60904
-			if pkg.Name() != item.Name {
-				return bug.Errorf("internal error: package name is %q, want %q (id=%q, path=%q) (see issue #60904)",
-					pkg.Name(), item.Name, id, item.Path)
-			}
 		}
 		return nil
 	}
@@ -724,7 +742,11 @@ func (b *typeCheckBatch) importMap(id PackageID) map[string]PackageID {
 	populateDeps = func(parent *metadata.Package) {
 		for _, id := range parent.DepsByPkgPath {
 			mp := b.handles[id].mp
-			if _, ok := impMap[string(mp.PkgPath)]; ok {
+			if prevID, ok := impMap[string(mp.PkgPath)]; ok {
+				// debugging #63822
+				if prevID != mp.ID {
+					bug.Reportf("inconsistent view of dependencies")
+				}
 				continue
 			}
 			impMap[string(mp.PkgPath)] = mp.ID

internal/gcimporter/iimport.go

@@ -224,6 +224,7 @@ func iimportCommon(fset *token.FileSet, getPackages GetPackagesFunc, data []byte
 
 	// Gather the relevant packages from the manifest.
 	items := make([]GetPackagesItem, r.uint64())
+	uniquePkgPaths := make(map[string]bool)
 	for i := range items {
 		pkgPathOff := r.uint64()
 		pkgPath := p.stringAt(pkgPathOff)
@@ -248,6 +249,12 @@ func iimportCommon(fset *token.FileSet, getPackages GetPackagesFunc, data []byte
 		}
 
 		items[i].nameIndex = nameIndex
+
+		uniquePkgPaths[pkgPath] = true
+	}
+	// Debugging #63822; hypothesis: there are duplicate PkgPaths.
+	if len(uniquePkgPaths) != len(items) {
+		reportf("found duplicate PkgPaths while reading export data manifest: %v", items)
 	}
 
 	// Request packages all at once from the client,