pkgtree: fix handling of cyclic import graphs

gps/pkgtree/pkgtree.go

@@ -901,14 +901,6 @@ func wmToReach(workmap map[string]wm, backprop bool) (ReachMap, map[string]*Prob
 			// Now, recurse until done, or a false bubbles up, indicating the
 			// path is poisoned.
 			for in := range w.in {
-				// It's possible, albeit weird, for a package to import itself.
-				// If we try to visit self, though, then it erroneously poisons
-				// the path, as it would be interpreted as grey. In practice,
-				// self-imports are a no-op, so we can just skip it.
-				if in == pkg {
-					continue
-				}
-
 				clean := dfe(in, path)
 				if !clean && backprop {
 					// Path is poisoned. If we're backpropagating errors, then
@@ -931,17 +923,35 @@ func wmToReach(workmap map[string]wm, backprop bool) (ReachMap, map[string]*Prob
 			return true
 
 		case grey:
-			// Import cycles can arise in healthy situations through xtests, so
-			// allow them for now.
+			// Grey means an import cycle. These can arise in healthy situations
+			// through xtest. They can also arise in less healthy but valid
+			// situations where an edge in the import graph is reversed based on
+			// the presence of a build tag. For example, if A depends on B on
+			// Linux, but B depends on A on Darwin, the import graph is not
+			// cyclic on either Linux or Darwin but dep will see what appears to
+			// be a dependency cycle because it considers all tags at once.
 			//
-			// FIXME(sdboyer) we need an improved model that allows us to
-			// accurately reject real import cycles.
+			// Handling import cycles for the purposes of reachablity is
+			// straightforward: we treat all packages in the cycle as
+			// equivalent. Any package imported by one package in the cycle is
+			// necessarily reachable by all other packages in the cycle.
+
+			// Merge the reachsets in the cycle by sharing the same external
+			// reachset and internal reachset amongst all packages in the
+			// cycle.
+			var cycleStarted bool
+			for _, ppkg := range path {
+				if cycleStarted {
+					exrsets[ppkg] = exrsets[pkg]
+					inrsets[ppkg] = inrsets[pkg]
+				} else if ppkg == pkg {
+					cycleStarted = true
+				}
+			}
+			if !cycleStarted {
+				panic(fmt.Sprintf("path to grey package %s did not include cycle: %s", pkg, path))
+			}
 			return true
-			// grey means an import cycle; guaranteed badness right here. You'd
-			// hope we never encounter it in a dependency (really? you published
-			// that code?), but we have to defend against it.
-			//colors[pkg] = black
-			//poison(append(path, pkg)) // poison self and parents
 
 		case black:
 			// black means we're revisiting a package that was already

gps/pkgtree/pkgtree_test.go

@@ -435,6 +435,103 @@ func TestWorkmapToReach(t *testing.T) {
 				},
 			},
 		},
+		"self cycle": {
+			workmap: map[string]wm{
+				"A": {in: map[string]bool{"A": true}},
+			},
+			rm: ReachMap{
+				"A": {Internal: []string{"A"}},
+			},
+		},
+		"simple cycle": {
+			workmap: map[string]wm{
+				"A": {in: map[string]bool{"B": true}},
+				"B": {in: map[string]bool{"A": true}},
+			},
+			rm: ReachMap{
+				"A": {Internal: []string{"A", "B"}},
+				"B": {Internal: []string{"A", "B"}},
+			},
+		},
+		"cycle with external dependency": {
+			workmap: map[string]wm{
+				"A": {
+					in: map[string]bool{"B": true},
+				},
+				"B": {
+					ex: map[string]bool{"C": true},
+					in: map[string]bool{"A": true},
+				},
+			},
+			rm: ReachMap{
+				"A": {
+					External: []string{"C"},
+					Internal: []string{"A", "B"},
+				},
+				"B": {
+					External: []string{"C"},
+					Internal: []string{"A", "B"},
+				},
+			},
+		},
+		"cycle with transitive external dependency": {
+			workmap: map[string]wm{
+				"A": {
+					in: map[string]bool{"B": true},
+				},
+				"B": {
+					in: map[string]bool{"A": true, "C": true},
+				},
+				"C": {
+					ex: map[string]bool{"D": true},
+				},
+			},
+			rm: ReachMap{
+				"A": {
+					External: []string{"D"},
+					Internal: []string{"A", "B", "C"},
+				},
+				"B": {
+					External: []string{"D"},
+					Internal: []string{"A", "B", "C"},
+				},
+				"C": {
+					External: []string{"D"},
+				},
+			},
+		},
+		"internal cycle": {
+			workmap: map[string]wm{
+				"A": {
+					ex: map[string]bool{"B": true},
+					in: map[string]bool{"C": true},
+				},
+				"C": {
+					in: map[string]bool{"D": true},
+				},
+				"D": {
+					in: map[string]bool{"E": true},
+				},
+				"E": {
+					in: map[string]bool{"C": true},
+				},
+			},
+			rm: ReachMap{
+				"A": {
+					External: []string{"B"},
+					Internal: []string{"C", "D", "E"},
+				},
+				"C": {
+					Internal: []string{"C", "D", "E"},
+				},
+				"D": {
+					Internal: []string{"C", "D", "E"},
+				},
+				"E": {
+					Internal: []string{"C", "D", "E"},
+				},
+			},
+		},
 	}
 
 	for name, fix := range table {