From fe83b33fb4b93fe8bd4c142468b940323318ebea Mon Sep 17 00:00:00 2001 From: sam boyer Date: Mon, 13 Feb 2017 02:22:57 -0500 Subject: [PATCH] s/ListExternalImports/Flatten/ Much better name. Also adds the capability of filtering out stdlib from PackageTree imports, addressing #113. --- analysis.go | 107 ++++++++++++------------------------------ analysis_test.go | 24 +++++++--- rootdata.go | 2 +- solve_basic_test.go | 9 ---- solve_bimodal_test.go | 2 +- 5 files changed, 50 insertions(+), 94 deletions(-) diff --git a/analysis.go b/analysis.go index 4939a78..7abcd3d 100644 --- a/analysis.go +++ b/analysis.go @@ -706,103 +706,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) diff --git a/analysis_test.go b/analysis_test.go index 7058b19..3b8c565 100644 --- a/analysis_test.go +++ b/analysis_test.go @@ -1137,7 +1137,7 @@ func TestListPackages(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 { @@ -1147,11 +1147,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) } @@ -1191,12 +1191,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() @@ -1291,7 +1301,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) diff --git a/rootdata.go b/rootdata.go index 96965d8..ba0af34 100644 --- a/rootdata.go +++ b/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) { diff --git a/solve_basic_test.go b/solve_basic_test.go index 8a704eb..9566927 100644 --- a/solve_basic_test.go +++ b/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} diff --git a/solve_bimodal_test.go b/solve_bimodal_test.go index d969ccd..b786baa 100644 --- a/solve_bimodal_test.go +++ b/solve_bimodal_test.go @@ -1089,7 +1089,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