internal/gcimporter: improve error handling

This change:
- updates the error message reported when the importer
  recovers from a panic.
- updates the set of test input files to include examples
  of the formats used in go1.16-go1.20.
- adds a recover handler to UImportData, for symmetry with
  IImportData. This was exposed by the new test case.
- fixes an accidental shadowing bug that suppressed the
  bundle format version check.

Fixes golang/go#59179

Change-Id: Ib6c20fc15e2051481fccba593607a7df0e01bc74
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494676
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>

Author: adonovan

internal/gcimporter/gcimporter.go

@@ -230,9 +230,7 @@ func Import(packages map[string]*types.Package, path, srcDir string, lookup func
 		// Or, define a new standard go/types/gcexportdata package.
 		fset := token.NewFileSet()
 
-		// The indexed export format starts with an 'i'; the older
-		// binary export format starts with a 'c', 'd', or 'v'
-		// (from "version"). Select appropriate importer.
+		// Select appropriate importer.
 		if len(data) > 0 {
 			switch data[0] {
 			case 'v', 'c', 'd': // binary, till go1.10

internal/gcimporter/gcimporter_test.go

@@ -314,14 +314,6 @@ func TestVersionHandling(t *testing.T) {
 		// test that export data can be imported
 		_, err := Import(make(map[string]*types.Package), pkgpath, dir, nil)
 		if err != nil {
-			// ok to fail if it fails with a newer version error for select files
-			if strings.Contains(err.Error(), "newer version") {
-				switch name {
-				case "test_go1.11_999b.a", "test_go1.11_999i.a":
-					continue
-				}
-				// fall through
-			}
 			t.Errorf("import %q failed: %v", pkgpath, err)
 			continue
 		}
@@ -351,7 +343,7 @@ func TestVersionHandling(t *testing.T) {
 		_, err = Import(make(map[string]*types.Package), pkgpath, corruptdir, nil)
 		if err == nil {
 			t.Errorf("import corrupted %q succeeded", pkgpath)
-		} else if msg := err.Error(); !strings.Contains(msg, "version skew") {
+		} else if msg := err.Error(); !strings.Contains(msg, "internal error") {
 			t.Errorf("import %q error incorrect (%s)", pkgpath, msg)
 		}
 	}

internal/gcimporter/iimport.go

@@ -131,7 +131,7 @@ func iimportCommon(fset *token.FileSet, getPackage GetPackageFunc, data []byte,
 				} else if version > currentVersion {
 					err = fmt.Errorf("cannot import %q (%v), export data is newer version - update tool", path, e)
 				} else {
-					err = fmt.Errorf("cannot import %q (%v), possibly version skew - reinstall package", path, e)
+					err = fmt.Errorf("internal error while importing %q (%v); please report an issue", path, e)
 				}
 			}
 		}()
@@ -140,11 +140,8 @@ func iimportCommon(fset *token.FileSet, getPackage GetPackageFunc, data []byte,
 	r := &intReader{bytes.NewReader(data), path}
 
 	if bundle {
-		bundleVersion := r.uint64()
-		switch bundleVersion {
-		case bundleVersion:
-		default:
-			errorf("unknown bundle format version %d", bundleVersion)
+		if v := r.uint64(); v != bundleVersion {
+			errorf("unknown bundle format version %d", v)
 		}
 	}
 

internal/gcimporter/testdata/versions/test.go

@@ -13,10 +13,7 @@
 //
 // go build -o test_go1.$X_$Y.a test.go
 //
-// with $X = Go version and $Y = export format version
-// (add 'b' or 'i' to distinguish between binary and
-// indexed format starting with 1.11 as long as both
-// formats are supported).
+// with $X = Go version and $Y = export format version (e.g. 'i', 'u').
 //
 // Make sure this source is extended such that it exercises
 // whatever export format change has taken place.

internal/gcimporter/testdata/versions/test_go1.16_i.a

@@ -10,6 +10,7 @@
 package gcimporter
 
 import (
+	"fmt"
 	"go/token"
 	"go/types"
 	"sort"
@@ -63,6 +64,14 @@ type typeInfo struct {
 }
 
 func UImportData(fset *token.FileSet, imports map[string]*types.Package, data []byte, path string) (_ int, pkg *types.Package, err error) {
+	if !debug {
+		defer func() {
+			if x := recover(); x != nil {
+				err = fmt.Errorf("internal error in importing %q (%v); please report an issue", path, x)
+			}
+		}()
+	}
+
 	s := string(data)
 	s = s[:strings.LastIndex(s, "\n$$\n")]
 	input := pkgbits.NewPkgDecoder(path, s)