Exclude root dirs from source list if bad path

Fixes golang#62. Also fixes a weird loop issue that caused erroneous poisoning
in wmToReach()'s depth-first traversal.

Author: sdboyer

_testdata/src/disallow/.m1p/a.go

@@ -0,0 +1,12 @@
+package m1p
+
+import (
+	"sort"
+
+	"github.com/sdboyer/gps"
+)
+
+var (
+	_ = sort.Strings
+	S = gps.Solve
+)

_testdata/src/disallow/.m1p/b.go

@@ -0,0 +1,11 @@
+package m1p
+
+import (
+	"os"
+	"sort"
+)
+
+var (
+	_ = sort.Strings
+	_ = os.PathSeparator
+)

_testdata/src/disallow/a.go

@@ -0,0 +1,14 @@
+package disallow
+
+import (
+	"sort"
+	"disallow/.m1p"
+
+	"github.com/sdboyer/gps"
+)
+
+var (
+	_ = sort.Strings
+	_ = gps.Solve
+	_ = m1p.S
+)

_testdata/src/disallow/testdata/another.go

@@ -0,0 +1,7 @@
+package testdata
+
+import "hash"
+
+var (
+	H = hash.Hash
+)

analysis.go

@@ -139,12 +139,18 @@ func listPackages(fileRoot, importRoot string) (PackageTree, error) {
 
 		// Skip dirs that are known to hold non-local/dependency code.
 		//
-		// We don't skip .*, _*, or testdata dirs because, while it may be poor
-		// form, it's not a compiler error to import them.
+		// We don't skip _*, or testdata dirs because, while it may be poor
+		// form, importing them is not a compilation error.
 		switch fi.Name() {
 		case "vendor", "Godeps":
 			return filepath.SkipDir
 		}
+		// We do skip dot-dirs, though, because it's such a ubiquitous standard
+		// that they not be visited by normal commands, and because things get
+		// really weird if we don't.
+		//
+		// TODO(sdboyer) does this entail that we should chuck dot-led import
+		// paths later on?
 		if strings.HasPrefix(fi.Name(), ".") {
 			return filepath.SkipDir
 		}
@@ -391,6 +397,14 @@ func wmToReach(workmap map[string]wm, basedir string) map[string][]string {
 			// path is poisoned.
 			var clean bool
 			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 reality,
+				// this becomes a no-op, so just skip it.
+				if in == pkg {
+					continue
+				}
+
 				clean = dfe(in, path)
 				if !clean {
 					// Path is poisoned. Our reachmap was already deleted by the
@@ -720,8 +734,8 @@ type PackageOrErr struct {
 // transitively imported by the internal packages in the tree.
 //
 // main indicates whether (true) or not (false) to include main packages in the
-// analysis. main packages should generally be excluded when analyzing the
-// non-root dependency, as they inherently can't be imported.
+// analysis. main packages are generally excluded when analyzing anything other
+// than the root project, as they inherently can't be imported.
 //
 // tests indicates whether (true) or not (false) to include imports from test
 // files in packages when computing the reach map.
@@ -826,9 +840,10 @@ func (t PackageTree) ExternalReach(main, tests bool, ignore map[string]bool) map
 //
 // 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. That means, given a PackageTree with root A and packages at A,
-// A/foo, and A/bar, and the following import chain:
+// 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
 //
@@ -854,50 +869,64 @@ func (t PackageTree) ExternalReach(main, tests bool, ignore map[string]bool) map
 // 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.
-func (t PackageTree) ListExternalImports(main, tests bool, ignore map[string]bool) ([]string, error) {
-	var someerrs bool
-	exm := make(map[string]struct{})
-
-	if ignore == nil {
-		ignore = make(map[string]bool)
-	}
-
-	var imps []string
-	for ip, perr := range t.Packages {
-		if perr.Err != nil {
-			someerrs = true
-			continue
-		}
-
-		p := perr.P
-		// Skip main packages, unless param says otherwise
-		if p.Name == "main" && !main {
-			continue
-		}
-		// Skip ignored packages
-		if ignore[ip] {
-			continue
-		}
+//
+// 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
+//
+// A is legal, and it imports A/.bar, so the results will include B/baz.
+func (t PackageTree) ListExternalImports(main, tests bool, ignore map[string]bool) []string {
+	// First, we need a reachmap
+	rm := t.ExternalReach(main, tests, ignore)
 
-		imps = imps[:0]
-		imps = p.Imports
-		if tests {
-			imps = dedupeStrings(imps, p.TestImports)
+	exm := make(map[string]struct{})
+	for pkg, reach := range rm {
+		// 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
+			}
 		}
 
-		for _, imp := range imps {
-			if !checkPrefixSlash(filepath.Clean(imp), t.ImportRoot) && !ignore[imp] {
-				exm[imp] = struct{}{}
+		if !skip {
+			for _, ex := range reach {
+				exm[ex] = struct{}{}
 			}
 		}
 	}
 
 	if len(exm) == 0 {
-		if someerrs {
-			// TODO(sdboyer) proper errs
-			return nil, fmt.Errorf("No packages without errors in %s", t.ImportRoot)
-		}
-		return nil, nil
+		return nil
 	}
 
 	ex := make([]string, len(exm))
@@ -908,7 +937,7 @@ func (t PackageTree) ListExternalImports(main, tests bool, ignore map[string]boo
 	}
 
 	sort.Strings(ex)
-	return ex, nil
+	return ex
 }
 
 // checkPrefixSlash checks to see if the prefix is a prefix of the string as-is,

analysis_test.go

@@ -698,6 +698,50 @@ func TestListPackages(t *testing.T) {
 				},
 			},
 		},
+		// has disallowed dir names
+		"disallowed dirs": {
+			fileRoot:   j("disallow"),
+			importRoot: "disallow",
+			out: PackageTree{
+				ImportRoot: "disallow",
+				Packages: map[string]PackageOrErr{
+					"disallow": {
+						P: Package{
+							ImportPath:  "disallow",
+							CommentPath: "",
+							Name:        "disallow",
+							Imports: []string{
+								"disallow/.m1p",
+								"github.com/sdboyer/gps",
+								"sort",
+							},
+						},
+					},
+					"disallow/.m1p": {
+						P: Package{
+							ImportPath:  "disallow/.m1p",
+							CommentPath: "",
+							Name:        "m1p",
+							Imports: []string{
+								"github.com/sdboyer/gps",
+								"os",
+								"sort",
+							},
+						},
+					},
+					"disallow/testdata": {
+						P: Package{
+							ImportPath:  "disallow/testdata",
+							CommentPath: "",
+							Name:        "testdata",
+							Imports: []string{
+								"hash",
+							},
+						},
+					},
+				},
+			},
+		},
 		// This case mostly exists for the PackageTree methods, but it does
 		// cover a bit of range
 		"varied": {
@@ -854,10 +898,7 @@ func TestListExternalImports(t *testing.T) {
 	var main, tests bool
 
 	validate := func() {
-		result, err := vptree.ListExternalImports(main, tests, ignore)
-		if err != nil {
-			t.Errorf("%q case returned err: %s", name, err)
-		}
+		result := vptree.ListExternalImports(main, tests, ignore)
 		if !reflect.DeepEqual(expect, result) {
 			t.Errorf("Wrong imports in %q case:\n\t(GOT): %s\n\t(WNT): %s", name, result, expect)
 		}
@@ -942,8 +983,7 @@ func TestListExternalImports(t *testing.T) {
 	ignore = map[string]bool{
 		"varied/simple": true,
 	}
-	// we get github.com/sdboyer/gps from m1p, too, so it should still be
-	// there
+	// we get github.com/sdboyer/gps from m1p, too, so it should still be there
 	except("go/parser")
 	validate()
 
@@ -990,6 +1030,18 @@ func TestListExternalImports(t *testing.T) {
 	}
 	except("sort", "github.com/sdboyer/gps", "go/parser")
 	validate()
+
+	// The only thing varied *doesn't* cover is disallowed path patterns
+	ptree, err := listPackages(filepath.Join(getwd(t), "_testdata", "src", "disallow"), "disallow")
+	if err != nil {
+		t.Fatalf("listPackages failed on disallow test case: %s", err)
+	}
+
+	result := ptree.ListExternalImports(false, false, nil)
+	expect = []string{"github.com/sdboyer/gps", "os", "sort"}
+	if !reflect.DeepEqual(expect, result) {
+		t.Errorf("Wrong imports in %q case:\n\t(GOT): %s\n\t(WNT): %s", name, result, expect)
+	}
 }
 
 func TestExternalReach(t *testing.T) {
@@ -1188,7 +1240,6 @@ func TestExternalReach(t *testing.T) {
 		"varied/namemismatch",
 	)
 	validate()
-
 }
 
 var _ = map[string][]string{

bridge.go

@@ -375,7 +375,7 @@ func (b *bridge) computeRootReach() ([]string, error) {
 		return nil, err
 	}
 
-	return ptree.ListExternalImports(true, true, b.s.ig)
+	return ptree.ListExternalImports(true, true, b.s.ig), nil
 }
 
 func (b *bridge) listRootPackages() (PackageTree, error) {

manifest.go

@@ -14,8 +14,7 @@ package gps
 // not themselves import. This is by design, but its implications are complex.
 // See the gps docs for more information: https://github.com/sdboyer/gps/wiki
 type Manifest interface {
-	// Returns a list of project constraints that will be  universally to
-	// the depgraph.
+	// Returns a list of project-level constraints.
 	DependencyConstraints() []ProjectConstraint
 	// Returns a list of constraints applicable to test imports. Note that this
 	// will only be consulted for root manifests.

solve_basic_test.go

@@ -1089,7 +1089,7 @@ func (b *depspecBridge) computeRootReach() ([]string, error) {
 		return nil, err
 	}
 
-	return ptree.ListExternalImports(true, true, dsm.ignore())
+	return ptree.ListExternalImports(true, true, dsm.ignore()), nil
 }
 
 // override verifyRoot() on bridge to prevent any filesystem interaction

solver.go

@@ -433,10 +433,10 @@ func (s *solver) selectRoot() error {
 	// If we're looking for root's deps, get it from opts and local root
 	// analysis, rather than having the sm do it
 	mdeps := append(s.rm.DependencyConstraints(), s.rm.TestDependencyConstraints()...)
-	reach, err := s.b.computeRootReach()
-	if err != nil {
-		return err
-	}
+
+	// Err is not possible at this point, as it could only come from
+	// listPackages(), which if we're here already succeeded for root
+	reach, _ := s.b.computeRootReach()
 
 	deps, err := s.intersectConstraintsWithImports(mdeps, reach)
 	if err != nil {