s/ListExternalImports/Flatten/

Much better name. Also adds the capability of filtering out stdlib from
PackageTree imports, addressing sdboyer/gps#113.

analysis.go

@@ -737,103 +737,58 @@ func wmToReach(workmap map[string]wm) ReachMap {
 	return rm
 }
 
-// ListExternalImports computes a sorted, deduplicated list of all the external
-// packages that are reachable through imports from all valid packages in a
-// ReachMap, as computed by PackageTree.ExternalReach().
+// FlattenAll flattens a reachmap into a sorted, deduplicated list of all the
+// external imports named by its contained packages.
 //
-// If an internal path is ignored, all of the external packages that it uniquely
-// imports are omitted. Note, however, that no internal transitivity checks are
-// made here - every non-ignored package in the tree is considered independently
-// (with one set of exceptions, noted below). That means, given a PackageTree
-// with root A and packages at A, A/foo, and A/bar, and the following import
-// chain:
-//
-//  A -> A/foo -> A/bar -> B/baz
-//
-// If you ignore A or A/foo, A/bar will still be visited, and B/baz will be
-// returned, because this method visits ALL packages in the tree, not only those
-// reachable from the root (or any other) packages. If your use case requires
-// interrogating external imports with respect to only specific package entry
-// points, you need ExternalReach() instead.
-//
-// It is safe to pass a nil map if there are no packages to ignore.
-//
-// If an internal package has an error (that is, PackageOrErr is Err), it is excluded from
-// consideration. Internal packages that transitively import the error package
-// are also excluded. So, if:
-//
-//    -> B/foo
-//   /
-//  A
-//   \
-//    -> A/bar -> B/baz
-//
-// And A/bar has some error in it, then both A and A/bar will be eliminated from
-// consideration; neither B/foo nor B/baz will be in the results. If A/bar, with
-// its errors, is ignored, however, then A will remain, and B/foo will be in the
-// results.
-//
-// Finally, note that if a directory is named "testdata", or has a leading dot
-// or underscore, it will not be directly analyzed as a source. This is in
-// keeping with Go tooling conventions that such directories should be ignored.
-// So, if:
-//
-//  A -> B/foo
-//  A/.bar -> B/baz
-//  A/_qux -> B/baz
-//  A/testdata -> B/baz
-//
-// Then B/foo will be returned, but B/baz will not, because all three of the
-// packages that import it are in directories with disallowed names.
-//
-// HOWEVER, in keeping with the Go compiler, if one of those packages in a
-// disallowed directory is imported by a package in an allowed directory, then
-// it *will* be used. That is, while tools like go list will ignore a directory
-// named .foo, you can still import from .foo. Thus, it must be included. So,
-// if:
-//
-//    -> B/foo
-//   /
-//  A
-//   \
-//    -> A/.bar -> B/baz
+// If stdlib is true, then stdlib imports are excluded from the result.
+func (rm ReachMap) FlattenAll(stdlib bool) []string {
+	return rm.flatten(func(pkg string) bool { return true }, stdlib)
+}
+
+// Flatten flattens a reachmap into a sorted, deduplicated list of all the
+// external imports named by its contained packages, but excludes imports coming
+// from packages with disallowed patterns in their names: any path element with
+// a leading dot, a leading underscore, with the name "testdata".
 //
-// A is legal, and it imports A/.bar, so the results will include B/baz.
-func (rm ReachMap) ListExternalImports() []string {
-	exm := make(map[string]struct{})
-	for pkg, reach := range rm {
+// If stdlib is true, then stdlib imports are excluded from the result.
+func (rm ReachMap) Flatten(stdlib bool) []string {
+	f := func(pkg string) bool {
 		// Eliminate import paths with any elements having leading dots, leading
 		// underscores, or testdata. If these are internally reachable (which is
 		// a no-no, but possible), any external imports will have already been
 		// pulled up through ExternalReach. The key here is that we don't want
 		// to treat such packages as themselves being sources.
-		//
-		// TODO(sdboyer) strings.Split will always heap alloc, which isn't great to do
-		// in a loop like this. We could also just parse it ourselves...
-		var skip bool
 		for _, elem := range strings.Split(pkg, "/") {
 			if strings.HasPrefix(elem, ".") || strings.HasPrefix(elem, "_") || elem == "testdata" {
-				skip = true
-				break
+				return false
 			}
 		}
+		return true
+	}
 
-		if !skip {
-			for _, ex := range reach {
+	return rm.flatten(f, stdlib)
+}
+
+func (rm ReachMap) flatten(filter func(string) bool, stdlib bool) []string {
+	exm := make(map[string]struct{})
+	for pkg, ie := range rm {
+		if filter(pkg) {
+			for _, ex := range ie.External {
+				if !stdlib && isStdLib(ex) {
+					continue
+				}
 				exm[ex] = struct{}{}
 			}
 		}
 	}
 
 	if len(exm) == 0 {
-		return nil
+		return []string{}
 	}
 
-	ex := make([]string, len(exm))
-	k := 0
+	ex := make([]string, 0, len(exm))
 	for p := range exm {
-		ex[k] = p
-		k++
+		ex = append(ex, p)
 	}
 
 	sort.Strings(ex)

analysis_test.go

@@ -1269,7 +1269,7 @@ func TestListPackagesNoPerms(t *testing.T) {
 	}
 }
 
-func TestListExternalImports(t *testing.T) {
+func TestFlattenReachMap(t *testing.T) {
 	// There's enough in the 'varied' test case to test most of what matters
 	vptree, err := ListPackages(filepath.Join(getwd(t), "_testdata", "src", "github.com", "example", "varied"), "github.com/example/varied")
 	if err != nil {
@@ -1279,11 +1279,11 @@ func TestListExternalImports(t *testing.T) {
 	var expect []string
 	var name string
 	var ignore map[string]bool
-	var main, tests bool
+	var stdlib, main, tests bool
 
 	validate := func() {
 		rm := vptree.ToReachMap(main, tests, ignore)
-		result := rm.ListExternalImports()
+		result := rm.Flatten(stdlib)
 		if !reflect.DeepEqual(expect, result) {
 			t.Errorf("Wrong imports in %q case:\n\t(GOT): %s\n\t(WNT): %s", name, result, expect)
 		}
@@ -1323,12 +1323,22 @@ func TestListExternalImports(t *testing.T) {
 	// everything on
 	name = "simple"
 	except()
-	main, tests = true, true
+	stdlib, main, tests = true, true, true
 	validate()
 
-	// Now without tests, which should just cut one
+	// turning off stdlib should cut most things, but we need to override the
+	// function
+	isStdLib = doIsStdLib
+	name = "no stdlib"
+	stdlib = false
+	except("encoding/binary", "go/parser", "hash", "net/http", "os", "sort")
+	validate()
+	// Restore stdlib func override
+	overrideIsStdLib()
+
+	// stdlib back in; now exclude tests, which should just cut one
 	name = "no tests"
-	tests = false
+	stdlib, tests = true, false
 	except("encoding/binary")
 	validate()
 
@@ -1423,7 +1433,7 @@ func TestListExternalImports(t *testing.T) {
 	}
 
 	rm := ptree.ToReachMap(false, false, nil)
-	result := rm.Flatten()
+	result := rm.Flatten(true)
 	expect = []string{"github.com/sdboyer/gps", "hash", "sort"}
 	if !reflect.DeepEqual(expect, result) {
 		t.Errorf("Wrong imports in %q case:\n\t(GOT): %s\n\t(WNT): %s", name, result, expect)

rootdata.go

@@ -46,7 +46,7 @@ type rootdata struct {
 // Ignores and requires are taken into consideration, and stdlib is excluded.
 func (rd rootdata) externalImportList() []string {
 	rm := rd.rpt.ToReachMap(true, true, rd.ig)
-	all := rm.ListExternalImports()
+	all := rm.Flatten(false)
 	reach := make([]string, 0, len(all))
 	for _, r := range all {
 		if !isStdLib(r) {

solve_basic_test.go

@@ -1416,15 +1416,6 @@ func (sm *depspecSourceManager) ExternalReach(id ProjectIdentifier, v Version) (
 	return nil, fmt.Errorf("No reach data for %s at version %s", id.errString(), v)
 }
 
-func (sm *depspecSourceManager) ListExternal(id ProjectIdentifier, v Version) ([]string, error) {
-	// This should only be called for the root
-	pid := pident{n: ProjectRoot(id.normalizedSource()), v: v}
-	if r, exists := sm.rm[pid]; exists {
-		return r[string(id.ProjectRoot)], nil
-	}
-	return nil, fmt.Errorf("No reach data for %s at version %s", id.errString(), v)
-}
-
 func (sm *depspecSourceManager) ListPackages(id ProjectIdentifier, v Version) (PackageTree, error) {
 	pid := pident{n: ProjectRoot(id.normalizedSource()), v: v}
 

solve_bimodal_test.go

@@ -1121,7 +1121,7 @@ func (sm *bmSourceManager) GetManifestAndLock(id ProjectIdentifier, v Version) (
 
 // computeBimodalExternalMap takes a set of depspecs and computes an
 // internally-versioned external reach map that is useful for quickly answering
-// ListExternal()-type calls.
+// ReachMap.Flatten()-type calls.
 //
 // Note that it does not do things like stripping out stdlib packages - these
 // maps are intended for use in SM fixtures, and that's a higher-level