diff --git a/analysis.go b/analysis.go index 4350f8b..4939a78 100644 --- a/analysis.go +++ b/analysis.go @@ -325,13 +325,16 @@ type PackageOrErr struct { Err error } -// ReachMap maps a set of import paths (keys) to the set of external packages -// transitively reachable from the packages at those import paths. +// ReachMap maps a set of import paths (keys) to the sets of transitively +// reachable tree-internal packages, and all the tree-external reachable through +// those internal packages. // -// See PackageTree.ExternalReach() for more information. -type ReachMap map[string][]string +// See PackageTree.ToReachMap() for more information. +type ReachMap map[string]struct { + Internal, External []string +} -// ToReachMaps looks through a PackageTree and computes the list of external +// ToReachMap looks through a PackageTree and computes the list of external // import statements (that is, import statements pointing to packages that are // not logical children of PackageTree.ImportRoot) that are transitively // imported by the internal packages in the tree. @@ -403,7 +406,7 @@ type ReachMap map[string][]string // // When backprop is false, errors in internal packages are functionally // identical to ignoring that package. -func (t PackageTree) ToReachMaps(main, tests bool, ignore map[string]bool) (ex ReachMap, in ReachMap) { +func (t PackageTree) ToReachMap(main, tests bool, ignore map[string]bool) ReachMap { if ignore == nil { ignore = make(map[string]bool) } @@ -477,7 +480,7 @@ func (t PackageTree) ToReachMaps(main, tests bool, ignore map[string]bool) (ex R // // The basedir string, with a trailing slash ensured, will be stripped from the // keys of the returned map. -func wmToReach(workmap map[string]wm) (ex ReachMap, in ReachMap) { +func wmToReach(workmap map[string]wm) ReachMap { // Uses depth-first exploration to compute reachability into external // packages, dropping any internal packages on "poisoned paths" - a path // containing a package with an error, or with a dep on an internal package @@ -510,10 +513,6 @@ func wmToReach(workmap map[string]wm) (ex ReachMap, in ReachMap) { // stack of parent packages we've visited to get to pkg. The return value // indicates whether the level completed successfully (true) or if it was // poisoned (false). - // - // TODO(sdboyer) some deft improvements could probably be made by passing the list of - // parent reachsets, rather than a list of parent package string names. - // might be able to eliminate the use of exrsets map-of-maps entirely. dfe = func(pkg string, path []string) bool { // white is the zero value of uint8, which is what we want if the pkg // isn't in the colors map, so this works fine @@ -660,12 +659,16 @@ func wmToReach(workmap map[string]wm) (ex ReachMap, in ReachMap) { dfe(pkg, path) } - // Flatten exrsets into the final external reachmap - rm := make(map[string][]string) + type ie struct { + Internal, External []string + } + + // Flatten exrsets into reachmap + rm := make(ReachMap) for pkg, rs := range exrsets { rlen := len(rs) if rlen == 0 { - rm[pkg] = nil + rm[pkg] = ie{} continue } @@ -675,15 +678,16 @@ func wmToReach(workmap map[string]wm) (ex ReachMap, in ReachMap) { } sort.Strings(edeps) - rm[pkg] = edeps + + sets := rm[pkg] + sets.External = edeps + rm[pkg] = sets } - // Flatten inrsets into the final internal reachmap - irm := make(map[string][]string) + // Flatten inrsets into reachmap for pkg, rs := range inrsets { rlen := len(rs) if rlen == 0 { - irm[pkg] = nil continue } @@ -693,10 +697,13 @@ func wmToReach(workmap map[string]wm) (ex ReachMap, in ReachMap) { } sort.Strings(ideps) - irm[pkg] = ideps + + sets := rm[pkg] + sets.Internal = ideps + rm[pkg] = sets } - return rm, irm + return rm } // ListExternalImports computes a sorted, deduplicated list of all the external diff --git a/analysis_test.go b/analysis_test.go index 378ca61..146bb43 100644 --- a/analysis_test.go +++ b/analysis_test.go @@ -23,6 +23,9 @@ func TestWorkmapToReach(t *testing.T) { return make(map[string]bool) } + e := struct { + Internal, External []string + }{} table := map[string]struct { workmap map[string]wm exrm, inrm ReachMap @@ -35,10 +38,7 @@ func TestWorkmapToReach(t *testing.T) { }, }, exrm: ReachMap{ - "foo": nil, - }, - inrm: ReachMap{ - "foo": nil, + "foo": e, }, }, "no external": { @@ -53,12 +53,8 @@ func TestWorkmapToReach(t *testing.T) { }, }, exrm: ReachMap{ - "foo": nil, - "foo/bar": nil, - }, - inrm: ReachMap{ - "foo": nil, - "foo/bar": nil, + "foo": e, + "foo/bar": e, }, }, "no external with subpkg": { @@ -75,12 +71,10 @@ func TestWorkmapToReach(t *testing.T) { }, }, exrm: ReachMap{ - "foo": nil, - "foo/bar": nil, - }, - inrm: ReachMap{ - "foo": {"foo/bar"}, - "foo/bar": nil, + "foo": { + Internal: []string{"foo/bar"}, + }, + "foo/bar": e, }, }, "simple base transitive": { @@ -100,16 +94,13 @@ func TestWorkmapToReach(t *testing.T) { }, exrm: ReachMap{ "foo": { - "baz", + External: []string{"baz"}, + Internal: []string{"foo/bar"}, }, "foo/bar": { - "baz", + External: []string{"baz"}, }, }, - inrm: ReachMap{ - "foo": {"foo/bar"}, - "foo/bar": nil, - }, }, "missing package is poison": { workmap: map[string]wm{ @@ -131,12 +122,9 @@ func TestWorkmapToReach(t *testing.T) { }, exrm: ReachMap{ "A/bar": { - "B/baz", + External: []string{"B/baz"}, }, }, - inrm: ReachMap{ - "A/bar": nil, - }, }, "transitive missing package is poison": { workmap: map[string]wm{ @@ -166,12 +154,9 @@ func TestWorkmapToReach(t *testing.T) { }, exrm: ReachMap{ "A/quux": { - "B/baz", + External: []string{"B/baz"}, }, }, - inrm: ReachMap{ - "A/quux": nil, - }, }, "err'd package is poison": { workmap: map[string]wm{ @@ -196,12 +181,9 @@ func TestWorkmapToReach(t *testing.T) { }, exrm: ReachMap{ "A/bar": { - "B/baz", + External: []string{"B/baz"}, }, }, - inrm: ReachMap{ - "A/bar": nil, - }, }, "transitive err'd package is poison": { workmap: map[string]wm{ @@ -234,12 +216,9 @@ func TestWorkmapToReach(t *testing.T) { }, exrm: ReachMap{ "A/quux": { - "B/baz", + External: []string{"B/baz"}, }, }, - inrm: ReachMap{ - "A/quux": nil, - }, }, // The following tests are mostly about regressions and weeding out // weird assumptions @@ -279,36 +258,39 @@ func TestWorkmapToReach(t *testing.T) { }, exrm: ReachMap{ "A": { - "B/baz", - "B/foo", - "C", - "D", + External: []string{ + "B/baz", + "B/foo", + "C", + "D", + }, + Internal: []string{ + "A/bar", + "A/foo", + "A/quux", + }, }, "A/foo": { - "B/baz", - "C", + External: []string{ + "B/baz", + "C", + }, + Internal: []string{ + "A/quux", + }, }, "A/bar": { - "B/baz", - "D", + External: []string{ + "B/baz", + "D", + }, + Internal: []string{ + "A/quux", + }, }, "A/quux": { - "B/baz", - }, - }, - inrm: ReachMap{ - "A": { - "A/bar", - "A/foo", - "A/quux", - }, - "A/foo": { - "A/quux", + External: []string{"B/baz"}, }, - "A/bar": { - "A/quux", - }, - "A/quux": nil, }, }, "rootmost gets imported": { @@ -330,29 +312,25 @@ func TestWorkmapToReach(t *testing.T) { }, exrm: ReachMap{ "A": { - "B", + External: []string{"B"}, }, "A/foo": { - "B", - "C", - }, - }, - inrm: ReachMap{ - "A": nil, - "A/foo": { - "A", + External: []string{ + "B", + "C", + }, + Internal: []string{ + "A", + }, }, }, }, } for name, fix := range table { - exrm, inrm := wmToReach(fix.workmap) - if !reflect.DeepEqual(exrm, fix.exrm) { - t.Errorf("wmToReach(%q): Did not get expected external reach map:\n\t(GOT): %s\n\t(WNT): %s", name, exrm, fix.exrm) - } - if !reflect.DeepEqual(inrm, fix.inrm) { - t.Errorf("wmToReach(%q): Did not get expected internal reach map:\n\t(GOT): %s\n\t(WNT): %s", name, exrm, fix.exrm) + rm := wmToReach(fix.workmap) + if !reflect.DeepEqual(rm, fix.exrm) { + t.Errorf("wmToReach(%q): Did not get expected reach map:\n\t(GOT): %s\n\t(WNT): %s", name, rm, fix.exrm) } } } @@ -1172,8 +1150,8 @@ func TestListExternalImports(t *testing.T) { var main, tests bool validate := func() { - exmap, _ := vptree.ToReachMaps(main, tests, ignore) - result := exmap.ListExternalImports() + rm := vptree.ToReachMap(main, tests, ignore) + result := rm.ListExternalImports() if !reflect.DeepEqual(expect, result) { t.Errorf("Wrong imports in %q case:\n\t(GOT): %s\n\t(WNT): %s", name, result, expect) } @@ -1312,8 +1290,8 @@ func TestListExternalImports(t *testing.T) { t.Fatalf("listPackages failed on disallow test case: %s", err) } - exmap, _ := ptree.ToReachMaps(false, false, nil) - result := exmap.ListExternalImports() + rm := ptree.ToReachMap(false, false, nil) + result := rm.ListExternalImports() 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) @@ -1342,52 +1320,31 @@ func TestToReachMaps(t *testing.T) { } // Set up vars for validate closure - var wantex, wantin map[string][]string + var want ReachMap var name string var main, tests bool var ignore map[string]bool validate := func() { - gotex, gotin := vptree.ToReachMaps(main, tests, ignore) - if !reflect.DeepEqual(wantex, gotex) { + got := vptree.ToReachMap(main, tests, ignore) + if !reflect.DeepEqual(want, got) { seen := make(map[string]bool) - for ip, epkgs := range wantex { + for ip, wantie := range want { seen[ip] = true - if pkgs, exists := gotex[ip]; !exists { + if gotie, exists := got[ip]; !exists { t.Errorf("ver(%q): expected import path %s was not present in result", name, ip) } else { - if !reflect.DeepEqual(pkgs, epkgs) { - t.Errorf("ver(%q): did not get expected package set for import path %s:\n\t(GOT): %s\n\t(WNT): %s", name, ip, pkgs, epkgs) + if !reflect.DeepEqual(wantie, gotie) { + t.Errorf("ver(%q): did not get expected import set for pkg %s:\n\t(GOT): %#v\n\t(WNT): %#v", name, ip, gotie, wantie) } } } - for ip, pkgs := range gotex { + for ip, ie := range got { if seen[ip] { continue } - t.Errorf("ver(%q): Got packages for import path %s, but none were expected:\n\t%s", name, ip, pkgs) - } - } - - if !reflect.DeepEqual(wantin, gotin) { - seen := make(map[string]bool) - for ip, epkgs := range wantin { - seen[ip] = true - if pkgs, exists := gotin[ip]; !exists { - t.Errorf("ver(%q): expected internal import path %s was not present in result", name, ip) - } else { - if !reflect.DeepEqual(pkgs, epkgs) { - t.Errorf("ver(%q): did not get expected internal package set for import path %s:\n\t(GOT): %s\n\t(WNT): %s", name, ip, pkgs, epkgs) - } - } - } - - for ip, pkgs := range gotin { - if seen[ip] { - continue - } - t.Errorf("ver(%q): Got internal packages for import path %s, but none were expected:\n\t%s", name, ip, pkgs) + t.Errorf("ver(%q): Got packages for import path %s, but none were expected:\n\t%s", name, ip, ie) } } } @@ -1433,67 +1390,28 @@ func TestToReachMaps(t *testing.T) { validin[ip] = m } - // helper to compose wantex, excepting specific packages + // helper to compose want, excepting specific packages // // this makes it easier to see what we're taking out on each test except := func(pkgig ...string) { // reinit expect with everything from all - wantex = make(map[string][]string) + want = make(ReachMap) for ip, expkgs := range allex { - sl := make([]string, len(expkgs)) - copy(sl, expkgs) - wantex[ip] = sl - } + var ie struct{ Internal, External []string } - // now build the dropmap - drop := make(map[string]map[string]bool) - for _, igstr := range pkgig { - // split on space; first elem is import path to pkg, the rest are - // the imports to drop. - not := strings.Split(igstr, " ") - var ip string - ip, not = not[0], not[1:] - if _, exists := valid[ip]; !exists { - t.Fatalf("%s is not a package name we're working with, doofus", ip) + inpkgs := allin[ip] + lenex, lenin := len(expkgs), len(inpkgs) + if lenex > 0 { + ie.External = make([]string, len(expkgs)) + copy(ie.External, expkgs) } - // if only a single elem was passed, though, drop the whole thing - if len(not) == 0 { - delete(wantex, ip) - continue + if lenin > 0 { + ie.Internal = make([]string, len(inpkgs)) + copy(ie.Internal, inpkgs) } - m := make(map[string]bool) - for _, imp := range not { - if !valid[ip][imp] { - t.Fatalf("%s is not a reachable import of %s, even in the all case", imp, ip) - } - m[imp] = true - } - - drop[ip] = m - } - - for ip, pkgs := range wantex { - var npkgs []string - for _, imp := range pkgs { - if !drop[ip][imp] { - npkgs = append(npkgs, imp) - } - } - - wantex[ip] = npkgs - } - } - - // same as above, but for internal reachmap - exceptin := func(pkgig ...string) { - // reinit expect with everything from all - wantin = make(map[string][]string) - for ip, inpkgs := range allin { - sl := make([]string, len(inpkgs)) - copy(sl, inpkgs) - wantin[ip] = sl + want[ip] = ie } // now build the dropmap @@ -1504,20 +1422,26 @@ func TestToReachMaps(t *testing.T) { not := strings.Split(igstr, " ") var ip string ip, not = not[0], not[1:] - if _, exists := validin[ip]; !exists { + if _, exists := valid[ip]; !exists { t.Fatalf("%s is not a package name we're working with, doofus", ip) } // if only a single elem was passed, though, drop the whole thing if len(not) == 0 { - delete(wantin, ip) + delete(want, ip) continue } m := make(map[string]bool) for _, imp := range not { - if !validin[ip][imp] { - t.Fatalf("%s is not a reachable import of %s, even in the all case", imp, ip) + if strings.HasPrefix(imp, "github.com/example/varied") { + if !validin[ip][imp] { + t.Fatalf("%s is not a reachable import of %s, even in the all case", imp, ip) + } + } else { + if !valid[ip][imp] { + t.Fatalf("%s is not a reachable import of %s, even in the all case", imp, ip) + } } m[imp] = true } @@ -1525,15 +1449,21 @@ func TestToReachMaps(t *testing.T) { drop[ip] = m } - for ip, pkgs := range wantin { - var npkgs []string - for _, imp := range pkgs { + for ip, ie := range want { + var nie struct{ Internal, External []string } + for _, imp := range ie.Internal { + if !drop[ip][imp] { + nie.Internal = append(nie.Internal, imp) + } + } + + for _, imp := range ie.External { if !drop[ip][imp] { - npkgs = append(npkgs, imp) + nie.External = append(nie.External, imp) } } - wantin[ip] = npkgs + want[ip] = nie } } @@ -1543,14 +1473,12 @@ func TestToReachMaps(t *testing.T) { name = "all" main, tests = true, true except() - exceptin() validate() // turn off main pkgs, which necessarily doesn't affect anything else name = "no main" main = false except(b("")) - exceptin(b("")) validate() // ignoring the "varied" pkg has same effect as disabling main pkgs @@ -1581,11 +1509,7 @@ func TestToReachMaps(t *testing.T) { b(""), b("simple")+" encoding/binary", b("simple/another")+" encoding/binary", - b("otherpath")+" github.com/sdboyer/gps os sort", - ) - exceptin( - b(""), - bl("otherpath", "m1p"), + bl("otherpath", "m1p")+" github.com/sdboyer/gps os sort", ) validate() @@ -1600,14 +1524,10 @@ func TestToReachMaps(t *testing.T) { } except( // root pkg loses on everything in varied/simple/another - b("")+" hash encoding/binary go/parser", - b("simple"), - ) - exceptin( // FIXME this is a bit odd, but should probably exclude m1p as well, // because it actually shouldn't be valid to import a package that only // has tests. This whole model misses that nuance right now, though. - bl("", "simple", "simple/another"), + bl("", "simple", "simple/another")+" hash encoding/binary go/parser", b("simple"), ) validate() @@ -1620,12 +1540,7 @@ func TestToReachMaps(t *testing.T) { } except( // root pkg loses on everything in varied/simple/another and varied/m1p - b("")+" hash encoding/binary go/parser github.com/sdboyer/gps sort", - b("otherpath"), - b("simple"), - ) - exceptin( - bl("", "simple", "simple/another", "m1p", "otherpath"), + bl("", "simple", "simple/another", "m1p", "otherpath")+" hash encoding/binary go/parser github.com/sdboyer/gps sort", b("otherpath"), b("simple"), ) @@ -1636,13 +1551,7 @@ func TestToReachMaps(t *testing.T) { ignore[b("namemismatch")] = true except( // root pkg loses on everything in varied/simple/another and varied/m1p - b("")+" hash encoding/binary go/parser github.com/sdboyer/gps sort os github.com/Masterminds/semver", - b("otherpath"), - b("simple"), - b("namemismatch"), - ) - exceptin( - bl("", "simple", "simple/another", "m1p", "otherpath", "namemismatch"), + bl("", "simple", "simple/another", "m1p", "otherpath", "namemismatch")+" hash encoding/binary go/parser github.com/sdboyer/gps sort os github.com/Masterminds/semver", b("otherpath"), b("simple"), b("namemismatch"), @@ -1657,7 +1566,7 @@ func TestToReachMapsCycle(t *testing.T) { t.Fatalf("ListPackages failed on cycle test case: %s", err) } - rm, _ := ptree.ToReachMaps(true, true, nil) + rm := ptree.ToReachMap(true, true, nil) // FIXME TEMPORARILY COMMENTED UNTIL WE CREATE A BETTER LISTPACKAGES MODEL - //if len(rm) > 0 { diff --git a/rootdata.go b/rootdata.go index b718655..96965d8 100644 --- a/rootdata.go +++ b/rootdata.go @@ -45,8 +45,8 @@ type rootdata struct { // rootImportList returns a list of the unique imports from the root data. // Ignores and requires are taken into consideration, and stdlib is excluded. func (rd rootdata) externalImportList() []string { - ex, _ := rd.rpt.ToReachMaps(true, true, rd.ig) - all := ex.ListExternalImports() + rm := rd.rpt.ToReachMap(true, true, rd.ig) + all := rm.ListExternalImports() reach := make([]string, 0, len(all)) for _, r := range all { if !isStdLib(r) { diff --git a/solve_bimodal_test.go b/solve_bimodal_test.go index 8569569..d969ccd 100644 --- a/solve_bimodal_test.go +++ b/solve_bimodal_test.go @@ -1138,7 +1138,11 @@ func computeBimodalExternalMap(ds []depspec) map[pident]map[string][]string { workmap[pkg.path] = w } - drm, _ := wmToReach(workmap) + reachmap := wmToReach(workmap) + drm := make(map[string][]string) + for ip, ie := range reachmap { + drm[ip] = ie.External + } rm[pident{n: d.n, v: d.v}] = drm } diff --git a/solver.go b/solver.go index 60ea983..bd76969 100644 --- a/solver.go +++ b/solver.go @@ -504,13 +504,13 @@ func (s *solver) getImportsAndConstraintsOf(a atomWithPackages) ([]string, []com return nil, nil, err } - allex, allin := ptree.ToReachMaps(false, false, s.rd.ig) + rm := ptree.ToReachMap(false, false, s.rd.ig) // Use maps to dedupe the unique internal and external packages. exmap, inmap := make(map[string]struct{}), make(map[string]struct{}) for _, pkg := range a.pl { inmap[pkg] = struct{}{} - for _, ipkg := range allin[pkg] { + for _, ipkg := range rm[pkg].Internal { inmap[ipkg] = struct{}{} } } @@ -531,7 +531,7 @@ func (s *solver) getImportsAndConstraintsOf(a atomWithPackages) ([]string, []com // Add to the list those packages that are reached by the packages // explicitly listed in the atom for _, pkg := range a.pl { - expkgs, exists := allex[pkg] + ie, exists := rm[pkg] if !exists { // missing package here *should* only happen if the target pkg was // poisoned somehow - check the original ptree. @@ -545,7 +545,7 @@ func (s *solver) getImportsAndConstraintsOf(a atomWithPackages) ([]string, []com return nil, nil, fmt.Errorf("package %s does not exist within project %s", pkg, a.a.id.errString()) } - for _, ex := range expkgs { + for _, ex := range ie.External { exmap[ex] = struct{}{} } } diff --git a/trace.go b/trace.go index ebcf3d8..ec72065 100644 --- a/trace.go +++ b/trace.go @@ -107,7 +107,7 @@ func (s *solver) traceSelectRoot(ptree PackageTree, cdeps []completeDep) { // This duplicates work a bit, but we're in trace mode and it's only once, // so who cares - rm, _ := ptree.ToReachMaps(true, true, s.rd.ig) + rm := ptree.ToReachMap(true, true, s.rd.ig) s.tl.Printf("Root project is %q", s.rd.rpt.ImportRoot)