go/packages: fix crash

This is based on 147340 and has the same test, but with a different fix.
We now produce a nice error if there is an empty root, instead of
crashing, and the cause of empty roots in go list has been fixed.
The underlying call to go list is returning the same package more than
once, and we only fix the first entry in the root list, so the second
one got left blank.
The fix was to not add the second duplicate copy to the output of the
driver at all.

Change-Id: I9f1b2f0fd63635ba101cdd3c8a5108530e968ba9
Reviewed-on: https://go-review.googlesource.com/c/147440
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>

go/packages/golist.go

@@ -13,6 +13,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"reflect"
 	"regexp"
 	"strings"
 	"sync"
@@ -488,6 +489,7 @@ func golistDriverCurrent(cfg *Config, words ...string) (*driverResponse, error)
 	if err != nil {
 		return nil, err
 	}
+	seen := make(map[string]*jsonPackage)
 	// Decode the JSON and convert it to Package form.
 	var response driverResponse
 	for dec := json.NewDecoder(buf); dec.More(); {
@@ -510,6 +512,15 @@ func golistDriverCurrent(cfg *Config, words ...string) (*driverResponse, error)
 			return nil, fmt.Errorf("package missing import path: %+v", p)
 		}
 
+		if old, found := seen[p.ImportPath]; found {
+			if !reflect.DeepEqual(p, old) {
+				return nil, fmt.Errorf("go list repeated package %v with different values", p.ImportPath)
+			}
+			// skip the duplicate
+			continue
+		}
+		seen[p.ImportPath] = p
+
 		pkg := &Package{
 			Name:            p.Name,
 			ID:              p.ImportPath,

go/packages/packages.go

@@ -437,6 +437,11 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 			lpkg.initial = true
 		}
 	}
+	for i, root := range roots {
+		if initial[i] == nil {
+			return nil, fmt.Errorf("root package %v is missing", root)
+		}
+	}
 
 	// Materialize the import graph.
 

go/packages/packages_test.go

@@ -1488,6 +1488,31 @@ EOF
 	}
 }
 
+// This test that a simple x test package layout loads correctly.
+// There was a bug in go list where it returned multiple copies of the same
+// package (specifically in this case of golang.org/fake/a), and this triggered
+// a bug in go/packages where it would leave an empty entry in the root package
+// list. This would then cause a nil pointer crash.
+// This bug was triggered by the simple package layout below, and thus this
+// test will make sure the bug remains fixed.
+func TestBasicXTest(t *testing.T) { packagestest.TestAll(t, testBasicXTest) }
+func testBasicXTest(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;`,
+			"a/a_test.go": `package a_test;`,
+		}}})
+	defer exported.Cleanup()
+
+	exported.Config.Mode = packages.LoadFiles
+	exported.Config.Tests = true
+	_, err := packages.Load(exported.Config, "golang.org/fake/a")
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
 func errorMessages(errors []packages.Error) []string {
 	var msgs []string
 	for _, err := range errors {