go/packages: add a workaround for golang/go#36188

This is a hack that checks for the extra "error" package that go list
generates when there's are some invalid imports, and attaches the error
to the importing package. This should eventually be taken out once
go list can return multiple errors on a package, and reports this error
on the importing package.

Fixes golang/go#36188

Change-Id: If355552d7d9bd2ed74a27ddb55133f8d60973d36
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216721
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

go/packages/golist.go

@@ -420,6 +420,8 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse
 		return nil, err
 	}
 	seen := make(map[string]*jsonPackage)
+	pkgs := make(map[string]*Package)
+	additionalErrors := make(map[string][]Error)
 	// Decode the JSON and convert it to Package form.
 	var response driverResponse
 	for dec := json.NewDecoder(buf); dec.More(); {
@@ -460,12 +462,63 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse
 		}
 
 		if old, found := seen[p.ImportPath]; found {
+			// If one version of the package has an error, and the other doesn't, assume
+			// that this is a case where go list is reporting a fake dependency variant
+			// of the imported package: When a package tries to invalidly import another
+			// package, go list emits a variant of the imported package (with the same
+			// import path, but with an error on it, and the package will have a
+			// DepError set on it). An example of when this can happen is for imports of
+			// main packages: main packages can not be imported, but they may be
+			// separately matched and listed by another pattern.
+			// See golang.org/issue/36188 for more details.
+
+			// The plan is that eventually, hopefully in Go 1.15, the error will be
+			// reported on the importing package rather than the duplicate "fake"
+			// version of the imported package. Once all supported versions of Go
+			// have the new behavior this logic can be deleted.
+			// TODO(matloob): delete the workaround logic once all supported versions of
+			// Go return the errors on the proper package.
+
+			// There should be exactly one version of a package that doesn't have an
+			// error.
+			if old.Error == nil && p.Error == nil {
 				if !reflect.DeepEqual(p, old) {
 					return nil, fmt.Errorf("internal error: go list gives conflicting information for package %v", p.ImportPath)
 				}
-			// skip the duplicate
 				continue
 			}
+
+			// Determine if this package's error needs to be bubbled up.
+			// This is a hack, and we expect for go list to eventually set the error
+			// on the package.
+			if old.Error != nil {
+				var errkind string
+				if strings.Contains(old.Error.Err, "not an importable package") {
+					errkind = "not an importable package"
+				} else if strings.Contains(old.Error.Err, "use of internal package") && strings.Contains(old.Error.Err, "not allowed") {
+					errkind = "use of internal package not allowed"
+				}
+				if errkind != "" {
+					if len(old.Error.ImportStack) < 2 {
+						return nil, fmt.Errorf(`internal error: go list gave a %q error with an import stack with fewer than two elements`, errkind)
+					}
+					importingPkg := old.Error.ImportStack[len(old.Error.ImportStack)-2]
+					additionalErrors[importingPkg] = append(additionalErrors[importingPkg], Error{
+						Pos:  old.Error.Pos,
+						Msg:  old.Error.Err,
+						Kind: ListError,
+					})
+				}
+			}
+
+			// Make sure that if there's a version of the package without an error,
+			// that's the one reported to the user.
+			if old.Error == nil {
+				continue
+			}
+
+			// This package will replace the old one at the end of the loop.
+		}
 		seen[p.ImportPath] = p
 
 		pkg := &Package{
@@ -563,6 +616,15 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse
 			})
 		}
 
+		pkgs[pkg.ID] = pkg
+	}
+
+	for id, errs := range additionalErrors {
+		if p, ok := pkgs[id]; ok {
+			p.Errors = append(p.Errors, errs...)
+		}
+	}
+	for _, pkg := range pkgs {
 		response.Packages = append(response.Packages, pkg)
 	}
 

go/packages/packages_test.go

@@ -1105,7 +1105,9 @@ func testNewPackagesInOverlay(t *testing.T, exporter packagestest.Exporter) {
 }
 
 // Test that we can create a package and its test package in an overlay.
-func TestOverlayNewPackageAndTest(t *testing.T) { packagestest.TestAll(t, testOverlayNewPackageAndTest) }
+func TestOverlayNewPackageAndTest(t *testing.T) {
+	packagestest.TestAll(t, testOverlayNewPackageAndTest)
+}
 func testOverlayNewPackageAndTest(t *testing.T, exporter packagestest.Exporter) {
 	exported := packagestest.Export(t, exporter, []packagestest.Module{
 		{
@@ -2409,6 +2411,38 @@ func testIssue35331(t *testing.T, exporter packagestest.Exporter) {
 	}, nil)
 }
 
+func TestMultiplePackageVersionsIssue36188(t *testing.T) {
+	packagestest.TestAll(t, testMultiplePackageVersionsIssue36188)
+}
+
+func testMultiplePackageVersionsIssue36188(t *testing.T, exporter packagestest.Exporter) {
+	exported := packagestest.Export(t, exporter, []packagestest.Module{{
+		Name: "golang.org/fake",
+		Files: map[string]interface{}{
+			"a/a.go": `package a; import _ "golang.org/fake/b"`,
+			"b/b.go": `package main`,
+		}}})
+	pkgs, err := packages.Load(exported.Config, "golang.org/fake/a", "golang.org/fake/b")
+	if err != nil {
+		t.Fatal(err)
+	}
+	sort.Slice(pkgs, func(i, j int) bool { return pkgs[i].ID < pkgs[j].ID })
+	if len(pkgs) != 2 {
+		t.Fatalf("expected two packages, got %v", pkgs)
+	}
+	if pkgs[0].ID != "golang.org/fake/a" && pkgs[1].ID != "golang.org/fake/b" {
+		t.Fatalf(`expected (sorted) IDs "golang.org/fake/a" and "golang.org/fake/b", got %q and %q`,
+			pkgs[0].ID, pkgs[1].ID)
+	}
+	if pkgs[0].Errors == nil {
+		t.Errorf(`expected error on package "golang.org/fake/a", got none`)
+	}
+	if pkgs[1].Errors != nil {
+		t.Errorf(`expected no errors on package "golang.org/fake/b", got %v`, pkgs[1].Errors)
+	}
+	defer exported.Cleanup()
+}
+
 func TestLoadModeStrings(t *testing.T) {
 	testcases := []struct {
 		mode     packages.LoadMode