From fc67db3d9a53227ff9e3a1d2c9fe897b3227a0c6 Mon Sep 17 00:00:00 2001
From: Ian Cottrell <iancottrell@google.com>
Date: Mon, 5 Nov 2018 14:24:16 -0500
Subject: [PATCH] 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        | 11 +++++++++++
 go/packages/packages.go      |  5 +++++
 go/packages/packages_test.go | 25 +++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/go/packages/golist.go b/go/packages/golist.go
index 2675c882..2dcc85e2 100644
--- a/go/packages/golist.go
+++ b/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,
diff --git a/go/packages/packages.go b/go/packages/packages.go
index 4e78f341..f3e04f39 100644
--- a/go/packages/packages.go
+++ b/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.
 
diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go
index 3f800036..275e8899 100644
--- a/go/packages/packages_test.go
+++ b/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 {