From efce0f5963afa087a1ac9af44255fce99c00ea33 Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Fri, 2 Feb 2024 14:14:25 -0500
Subject: [PATCH] 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 <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
---
 gopls/internal/cache/metadata/cycle_test.go | 51 ++++----------------
 gopls/internal/cache/metadata/graph.go      | 53 +++++++++++++++++++++
 gopls/internal/util/bug/bug.go              |  3 ++
 3 files changed, 65 insertions(+), 42 deletions(-)

diff --git a/gopls/internal/cache/metadata/cycle_test.go b/gopls/internal/cache/metadata/cycle_test.go
index 3597be73b84..09628d881e9 100644
--- a/gopls/internal/cache/metadata/cycle_test.go
+++ b/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)
diff --git a/gopls/internal/cache/metadata/graph.go b/gopls/internal/cache/metadata/graph.go
index ca550d7ce6a..f09822d3575 100644
--- a/gopls/internal/cache/metadata/graph.go
+++ b/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.
diff --git a/gopls/internal/util/bug/bug.go b/gopls/internal/util/bug/bug.go
index 6e9828f1e56..e04b753e315 100644
--- a/gopls/internal/util/bug/bug.go
+++ b/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 (