Fix bug in preparation of workmaps

The old logic was a holdover from before a proper depth-first search
algorithm was implemented in wmToReach(). It was trying to do a "bit" of
the search work before the real algorithm; however, the final else
statement was causing some internal imports to be dropped if the
referent had already been visited. The bug was intermittent, as it
depended on map iteration order.

This also solves the secondary problem of inaccurate
backpropagation/poisoning in wmToReach itself, as the internal linkage
data it's operating on is now reliable.

Author: sdboyer

analysis.go

@@ -462,35 +462,28 @@ func (t PackageTree) ToReachMaps(main, tests bool, ignore map[string]bool) (ex R
 		}
 
 		imps = imps[:0]
-		imps = p.Imports
 		if tests {
-			imps = dedupeStrings(imps, p.TestImports)
+			imps = dedupeStrings(p.Imports, p.TestImports)
+		} else {
+			imps = p.Imports
 		}
 
 		w := wm{
 			ex: make(map[string]bool),
 			in: make(map[string]bool),
 		}
 
+		// For each import, decide whether it should be ignored, or if it
+		// belongs in the external or internal imports list.
 		for _, imp := range imps {
-			// Skip ignored imports
 			if ignore[imp] {
 				continue
 			}
 
 			if !eqOrSlashedPrefix(imp, t.ImportRoot) {
 				w.ex[imp] = true
 			} else {
-				if w2, seen := workmap[imp]; seen {
-					for i := range w2.ex {
-						w.ex[i] = true
-					}
-					for i := range w2.in {
-						w.in[i] = true
-					}
-				} else {
-					w.in[imp] = true
-				}
+				w.in[imp] = true
 			}
 		}
 

analysis_test.go

@@ -243,6 +243,109 @@ func TestWorkmapToReach(t *testing.T) {
 				"A/quux": nil,
 			},
 		},
+		// The following tests are mostly about regressions and weeding out
+		// weird assumptions
+		"internal diamond": {
+			workmap: map[string]wm{
+				"A": {
+					ex: map[string]bool{
+						"B/foo": true,
+					},
+					in: map[string]bool{
+						"A/foo": true,
+						"A/bar": true,
+					},
+				},
+				"A/foo": {
+					ex: map[string]bool{
+						"C": true,
+					},
+					in: map[string]bool{
+						"A/quux": true,
+					},
+				},
+				"A/bar": {
+					ex: map[string]bool{
+						"D": true,
+					},
+					in: map[string]bool{
+						"A/quux": true,
+					},
+				},
+				"A/quux": {
+					ex: map[string]bool{
+						"B/baz": true,
+					},
+					in: empty(),
+				},
+			},
+			exrm: ReachMap{
+				"A": {
+					"B/baz",
+					"B/foo",
+					"C",
+					"D",
+				},
+				"A/foo": {
+					"B/baz",
+					"C",
+				},
+				"A/bar": {
+					"B/baz",
+					"D",
+				},
+				"A/quux": {
+					"B/baz",
+				},
+			},
+			inrm: ReachMap{
+				"A": {
+					"A/bar",
+					"A/foo",
+					"A/quux",
+				},
+				"A/foo": {
+					"A/quux",
+				},
+				"A/bar": {
+					"A/quux",
+				},
+				"A/quux": nil,
+			},
+		},
+		"rootmost gets imported": {
+			workmap: map[string]wm{
+				"A": {
+					ex: map[string]bool{
+						"B": true,
+					},
+					in: empty(),
+				},
+				"A/foo": {
+					ex: map[string]bool{
+						"C": true,
+					},
+					in: map[string]bool{
+						"A": true,
+					},
+				},
+			},
+			exrm: ReachMap{
+				"A": {
+					"B",
+				},
+				"A/foo": {
+					"B",
+					"C",
+				},
+			},
+			inrm: ReachMap{
+				"A": nil,
+				"A/foo": {
+					"A",
+				},
+			},
+		},
 	}
 
 	for name, fix := range table {