gopls/internal/cache/metadata: assert graph is acyclic

This change causes the metadata graph to assert that it is
acyclic before applying the updates. This should be an
invariant, but the attached issues make us skeptical.

Updates golang/go#64227
Updates golang/vscode-go#3126

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

Author: adonovan

gopls/internal/cache/metadata/cycle_test.go

@@ -8,58 +8,25 @@ import (
 	"sort"
 	"strings"
 	"testing"
+
+	"golang.org/x/tools/gopls/internal/util/bug"
 )
 
+func init() {
+	bug.PanicOnBugs = true
+}
+
 // This is an internal test of the breakImportCycles logic.
 func TestBreakImportCycles(t *testing.T) {
 
-	type Graph = map[PackageID]*Package
-
-	// cyclic returns a description of a cycle,
-	// if the graph is cyclic, otherwise "".
-	cyclic := func(graph Graph) string {
-		const (
-			unvisited = 0
-			visited   = 1
-			onstack   = 2
-		)
-		color := make(map[PackageID]int)
-		var visit func(id PackageID) string
-		visit = func(id PackageID) string {
-			switch color[id] {
-			case unvisited:
-				color[id] = onstack
-			case onstack:
-				return string(id) // cycle!
-			case visited:
-				return ""
-			}
-			if mp := graph[id]; mp != nil {
-				for _, depID := range mp.DepsByPkgPath {
-					if cycle := visit(depID); cycle != "" {
-						return string(id) + "->" + cycle
-					}
-				}
-			}
-			color[id] = visited
-			return ""
-		}
-		for id := range graph {
-			if cycle := visit(id); cycle != "" {
-				return cycle
-			}
-		}
-		return ""
-	}
-
 	// parse parses an import dependency graph.
 	// The input is a semicolon-separated list of node descriptions.
 	// Each node description is a package ID, optionally followed by
 	// "->" and a comma-separated list of successor IDs.
 	// Thus "a->b;b->c,d;e" represents the set of nodes {a,b,e}
 	// and the set of edges {a->b, b->c, b->d}.
-	parse := func(s string) Graph {
-		m := make(Graph)
+	parse := func(s string) map[PackageID]*Package {
+		m := make(map[PackageID]*Package)
 		makeNode := func(name string) *Package {
 			id := PackageID(name)
 			n, ok := m[id]
@@ -98,7 +65,7 @@ func TestBreakImportCycles(t *testing.T) {
 	// format formats an import graph, in lexicographic order,
 	// in the notation of parse, but with a "!" after the name
 	// of each node that has errors.
-	format := func(graph Graph) string {
+	format := func(graph map[PackageID]*Package) string {
 		var items []string
 		for _, mp := range graph {
 			item := string(mp.ID)

gopls/internal/cache/metadata/graph.go

@@ -38,6 +38,22 @@ func (g *Graph) Update(updates map[PackageID]*Package) *Graph {
 		return g
 	}
 
+	// Debugging golang/go#64227, golang/vscode-go#3126:
+	// Assert that the existing metadata graph is acyclic.
+	if cycle := cyclic(g.Packages); cycle != "" {
+		bug.Reportf("metadata is cyclic even before updates: %s", cycle)
+	}
+	// Assert that the updates contain no self-cycles.
+	for id, mp := range updates {
+		if mp != nil {
+			for _, depID := range mp.DepsByPkgPath {
+				if depID == id {
+					bug.Reportf("self-cycle in metadata update: %s", id)
+				}
+			}
+		}
+	}
+
 	// Copy pkgs map then apply updates.
 	pkgs := make(map[PackageID]*Package, len(g.Packages))
 	for id, mp := range g.Packages {
@@ -223,6 +239,43 @@ func breakImportCycles(metadata, updates map[PackageID]*Package) {
 	}
 }
 
+// cyclic returns a description of a cycle,
+// if the graph is cyclic, otherwise "".
+func cyclic(graph map[PackageID]*Package) string {
+	const (
+		unvisited = 0
+		visited   = 1
+		onstack   = 2
+	)
+	color := make(map[PackageID]int)
+	var visit func(id PackageID) string
+	visit = func(id PackageID) string {
+		switch color[id] {
+		case unvisited:
+			color[id] = onstack
+		case onstack:
+			return string(id) // cycle!
+		case visited:
+			return ""
+		}
+		if mp := graph[id]; mp != nil {
+			for _, depID := range mp.DepsByPkgPath {
+				if cycle := visit(depID); cycle != "" {
+					return string(id) + "->" + cycle
+				}
+			}
+		}
+		color[id] = visited
+		return ""
+	}
+	for id := range graph {
+		if cycle := visit(id); cycle != "" {
+			return cycle
+		}
+	}
+	return ""
+}
+
 // detectImportCycles reports cycles in the metadata graph. It returns a new
 // unordered array of all cycles (nontrivial strong components) in the
 // metadata graph reachable from a non-nil 'updates' value.

gopls/internal/util/bug/bug.go

@@ -25,6 +25,9 @@ import (
 // PanicOnBugs controls whether to panic when bugs are reported.
 //
 // It may be set to true during testing.
+//
+// TODO(adonovan): should we make the default true, and
+// suppress it only in the product (gopls/main.go)?
 var PanicOnBugs = false
 
 var (