From 19e2aca3fdf9724feacb8701307db6fb35055030 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 18 Sep 2018 15:06:34 -0400 Subject: [PATCH] internal/gopathwalk: create Extract goimports' logic for walking Go source directories into a separate package, suitable for use in go/packages. No functional changes. Added a convenience feature to fastwalk, allowing the user to say that they're done with a directory and stop receiving callbacks for it. Testing is a little light; I expect goimports' tests to cover most everything we care about. Change-Id: If047ada4414f5f282637d11fd07e8342fadc9c33 Reviewed-on: https://go-review.googlesource.com/c/138877 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Michael Matloob Reviewed-by: Brad Fitzpatrick --- imports/fix.go | 190 ++-------------------- imports/fix_test.go | 73 --------- internal/fastwalk/fastwalk.go | 5 + internal/fastwalk/fastwalk_portable.go | 8 + internal/fastwalk/fastwalk_test.go | 32 ++++ internal/fastwalk/fastwalk_unix.go | 8 + internal/gopathwalk/walk.go | 209 +++++++++++++++++++++++++ internal/gopathwalk/walk_test.go | 135 ++++++++++++++++ 8 files changed, 407 insertions(+), 253 deletions(-) create mode 100644 internal/gopathwalk/walk.go create mode 100644 internal/gopathwalk/walk_test.go diff --git a/imports/fix.go b/imports/fix.go index d9cbe656ec..75d37f894e 100644 --- a/imports/fix.go +++ b/imports/fix.go @@ -5,8 +5,6 @@ package imports import ( - "bufio" - "bytes" "context" "fmt" "go/ast" @@ -24,7 +22,7 @@ import ( "sync" "golang.org/x/tools/go/ast/astutil" - "golang.org/x/tools/internal/fastwalk" + "golang.org/x/tools/internal/gopathwalk" ) // Debug controls verbose logging. @@ -523,195 +521,27 @@ func distance(basepath, targetpath string) int { return strings.Count(p, string(filepath.Separator)) + 1 } -// getIgnoredDirs reads an optional config file at /.goimportsignore -// of relative directories to ignore when scanning for go files. -// The provided path is one of the $GOPATH entries with "src" appended. -func getIgnoredDirs(path string) []os.FileInfo { - file := filepath.Join(path, ".goimportsignore") - slurp, err := ioutil.ReadFile(file) - if Debug { - if err != nil { - log.Print(err) - } else { - log.Printf("Read %s", file) - } - } - if err != nil { - return nil - } - - var ignoredDirs []os.FileInfo - bs := bufio.NewScanner(bytes.NewReader(slurp)) - for bs.Scan() { - line := strings.TrimSpace(bs.Text()) - if line == "" || strings.HasPrefix(line, "#") { - continue - } - full := filepath.Join(path, line) - if fi, err := os.Stat(full); err == nil { - ignoredDirs = append(ignoredDirs, fi) - if Debug { - log.Printf("Directory added to ignore list: %s", full) - } - } else if Debug { - log.Printf("Error statting entry in .goimportsignore: %v", err) - } - } - return ignoredDirs -} - -func shouldSkipDir(fi os.FileInfo, ignoredDirs []os.FileInfo) bool { - for _, ignoredDir := range ignoredDirs { - if os.SameFile(fi, ignoredDir) { - return true - } - } - return false -} - -// shouldTraverse reports whether the symlink fi, found in dir, -// should be followed. It makes sure symlinks were never visited -// before to avoid symlink loops. -func shouldTraverse(dir string, fi os.FileInfo, ignoredDirs []os.FileInfo) bool { - path := filepath.Join(dir, fi.Name()) - target, err := filepath.EvalSymlinks(path) - if err != nil { - return false - } - ts, err := os.Stat(target) - if err != nil { - fmt.Fprintln(os.Stderr, err) - return false - } - if !ts.IsDir() { - return false - } - if shouldSkipDir(ts, ignoredDirs) { - return false - } - // Check for symlink loops by statting each directory component - // and seeing if any are the same file as ts. - for { - parent := filepath.Dir(path) - if parent == path { - // Made it to the root without seeing a cycle. - // Use this symlink. - return true - } - parentInfo, err := os.Stat(parent) - if err != nil { - return false - } - if os.SameFile(ts, parentInfo) { - // Cycle. Don't traverse. - return false - } - path = parent - } - -} - -var testHookScanDir = func(dir string) {} - // scanGoDirs populates the dirScan map for GOPATH and GOROOT. func scanGoDirs() map[string]*pkg { result := make(map[string]*pkg) var mu sync.Mutex - for _, srcDir := range build.Default.SrcDirs() { - if Debug { - log.Printf("scanning %s", srcDir) - } + add := func(srcDir, dir string) { + mu.Lock() + defer mu.Unlock() - testHookScanDir(srcDir) - w := &walker{ - srcDir: srcDir, - srcV: filepath.Join(srcDir, "v"), - srcMod: filepath.Join(srcDir, "mod"), - ignoredDirs: getIgnoredDirs(srcDir), - dirScanMu: &mu, - dirScan: result, + if _, dup := result[dir]; dup { + return } - if err := fastwalk.Walk(srcDir, w.walk); err != nil { - log.Printf("goimports: scanning directory %v: %v", srcDir, err) - } - - if Debug { - defer log.Printf("scanned %s", srcDir) - } - } - return result -} - -// walker is the callback for fastwalk.Walk. -type walker struct { - srcDir string // The source directory to scan. - srcV, srcMod string // vgo-style module cache dirs. Optional. - ignoredDirs []os.FileInfo // The ignored directories, loaded from .goimportsignore files. - - dirScanMu *sync.Mutex // The shared mutex guarding dirScan. - dirScan map[string]*pkg // The results of the scan, sharable across multiple Walk calls. -} - -func (w *walker) walk(path string, typ os.FileMode) error { - if path == w.srcV || path == w.srcMod { - return filepath.SkipDir - } - dir := filepath.Dir(path) - if typ.IsRegular() { - if dir == w.srcDir { - // Doesn't make sense to have regular files - // directly in your $GOPATH/src or $GOROOT/src. - return nil - } - if !strings.HasSuffix(path, ".go") { - return nil - } - - w.dirScanMu.Lock() - defer w.dirScanMu.Unlock() - if _, dup := w.dirScan[dir]; dup { - return nil - } - importpath := filepath.ToSlash(dir[len(w.srcDir)+len("/"):]) - w.dirScan[dir] = &pkg{ + importpath := filepath.ToSlash(dir[len(srcDir)+len("/"):]) + result[dir] = &pkg{ importPath: importpath, importPathShort: VendorlessPath(importpath), dir: dir, } - return nil } - if typ == os.ModeDir { - base := filepath.Base(path) - if base == "" || base[0] == '.' || base[0] == '_' || - base == "testdata" || base == "node_modules" { - return filepath.SkipDir - } - fi, err := os.Lstat(path) - if err == nil && shouldSkipDir(fi, w.ignoredDirs) { - if Debug { - log.Printf("skipping directory %q under %s", fi.Name(), dir) - } - return filepath.SkipDir - } - return nil - } - if typ == os.ModeSymlink { - base := filepath.Base(path) - if strings.HasPrefix(base, ".#") { - // Emacs noise. - return nil - } - fi, err := os.Lstat(path) - if err != nil { - // Just ignore it. - return nil - } - if shouldTraverse(dir, fi, w.ignoredDirs) { - return fastwalk.TraverseLink - } - } - return nil + gopathwalk.Walk(add, gopathwalk.Options{Debug: Debug, ModulesEnabled: false}) + return result } // VendorlessPath returns the devendorized version of the import path ipath. diff --git a/imports/fix_test.go b/imports/fix_test.go index 4a4315050a..f78fb5dffc 100644 --- a/imports/fix_test.go +++ b/imports/fix_test.go @@ -1559,10 +1559,8 @@ func withEmptyGoPath(fn func()) { oldCompiler := build.Default.Compiler build.Default.GOPATH = "" build.Default.Compiler = "gc" - testHookScanDir = func(string) {} defer func() { - testHookScanDir = func(string) {} build.Default.GOPATH = oldGOPATH build.Default.GOROOT = oldGOROOT build.Default.Compiler = oldCompiler @@ -2268,77 +2266,6 @@ func TestPkgIsCandidate(t *testing.T) { } } -func TestShouldTraverse(t *testing.T) { - switch runtime.GOOS { - case "windows", "plan9": - t.Skipf("skipping symlink-requiring test on %s", runtime.GOOS) - } - - dir, err := ioutil.TempDir("", "goimports-") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(dir) - - // Note: mapToDir prepends "src" to each element, since - // mapToDir was made for creating GOPATHs. - if err := mapToDir(dir, map[string]string{ - "foo/foo2/file.txt": "", - "foo/foo2/link-to-src": "LINK:" + dir + "/src", - "foo/foo2/link-to-src-foo": "LINK:" + dir + "/src/foo", - "foo/foo2/link-to-dot": "LINK:.", - "bar/bar2/file.txt": "", - "bar/bar2/link-to-src-foo": "LINK:" + dir + "/src/foo", - - "a/b/c": "LINK:" + dir + "/src/a/d", - "a/d/e": "LINK:" + dir + "/src/a/b", - }); err != nil { - t.Fatal(err) - } - tests := []struct { - dir string - file string - want bool - }{ - { - dir: dir + "/src/foo/foo2", - file: "link-to-src-foo", - want: false, // loop - }, - { - dir: dir + "/src/foo/foo2", - file: "link-to-src", - want: false, // loop - }, - { - dir: dir + "/src/foo/foo2", - file: "link-to-dot", - want: false, // loop - }, - { - dir: dir + "/src/bar/bar2", - file: "link-to-src-foo", - want: true, // not a loop - }, - { - dir: dir + "/src/a/b/c", - file: "e", - want: false, // loop: "e" is the same as "b". - }, - } - for i, tt := range tests { - fi, err := os.Stat(filepath.Join(tt.dir, tt.file)) - if err != nil { - t.Errorf("%d. Stat = %v", i, err) - continue - } - got := shouldTraverse(tt.dir, fi, nil) - if got != tt.want { - t.Errorf("%d. shouldTraverse(%q, %q) = %v; want %v", i, tt.dir, tt.file, got, tt.want) - } - } -} - // Issue 20941: this used to panic on Windows. func TestProcessStdin(t *testing.T) { got, err := Process("", []byte("package main\nfunc main() {\n\tfmt.Println(123)\n}\n"), nil) diff --git a/internal/fastwalk/fastwalk.go b/internal/fastwalk/fastwalk.go index 5cc7df53ff..7219c8e9ff 100644 --- a/internal/fastwalk/fastwalk.go +++ b/internal/fastwalk/fastwalk.go @@ -18,6 +18,11 @@ import ( // symlink named in the call may be traversed. var TraverseLink = errors.New("fastwalk: traverse symlink, assuming target is a directory") +// SkipFiles is a used as a return value from WalkFuncs to indicate that the +// callback should not be called for any other files in the current directory. +// Child directories will still be traversed. +var SkipFiles = errors.New("fastwalk: skip remaining files in directory") + // Walk is a faster implementation of filepath.Walk. // // filepath.Walk's design necessarily calls os.Lstat on each file, diff --git a/internal/fastwalk/fastwalk_portable.go b/internal/fastwalk/fastwalk_portable.go index e8ea50d65a..a906b87595 100644 --- a/internal/fastwalk/fastwalk_portable.go +++ b/internal/fastwalk/fastwalk_portable.go @@ -20,8 +20,16 @@ func readDir(dirName string, fn func(dirName, entName string, typ os.FileMode) e if err != nil { return err } + skipFiles := false for _, fi := range fis { + if fi.Mode().IsRegular() && skipFiles { + continue + } if err := fn(dirName, fi.Name(), fi.Mode()&os.ModeType); err != nil { + if err == SkipFiles { + skipFiles = true + continue + } return err } } diff --git a/internal/fastwalk/fastwalk_test.go b/internal/fastwalk/fastwalk_test.go index e9ab0d59f7..efb813c67a 100644 --- a/internal/fastwalk/fastwalk_test.go +++ b/internal/fastwalk/fastwalk_test.go @@ -145,6 +145,38 @@ func TestFastWalk_SkipDir(t *testing.T) { }) } +func TestFastWalk_SkipFiles(t *testing.T) { + // Directory iteration order is undefined, so there's no way to know + // which file to expect until the walk happens. Rather than mess + // with the test infrastructure, just mutate want. + var mu sync.Mutex + want := map[string]os.FileMode{ + "": os.ModeDir, + "/src": os.ModeDir, + "/src/zzz": os.ModeDir, + "/src/zzz/c.go": 0, + } + + testFastWalk(t, map[string]string{ + "a_skipfiles.go": "a", + "b_skipfiles.go": "b", + "zzz/c.go": "c", + }, + func(path string, typ os.FileMode) error { + if strings.HasSuffix(path, "_skipfiles.go") { + mu.Lock() + defer mu.Unlock() + want["/src/"+filepath.Base(path)] = 0 + return fastwalk.SkipFiles + } + return nil + }, + want) + if len(want) != 5 { + t.Errorf("saw too many files: wanted 5, got %v (%v)", len(want), want) + } +} + func TestFastWalk_TraverseSymlink(t *testing.T) { switch runtime.GOOS { case "windows", "plan9": diff --git a/internal/fastwalk/fastwalk_unix.go b/internal/fastwalk/fastwalk_unix.go index 67db6caf96..e541c57506 100644 --- a/internal/fastwalk/fastwalk_unix.go +++ b/internal/fastwalk/fastwalk_unix.go @@ -32,6 +32,7 @@ func readDir(dirName string, fn func(dirName, entName string, typ os.FileMode) e buf := make([]byte, blockSize) // stack-allocated; doesn't escape bufp := 0 // starting read position in buf nbuf := 0 // end valid data in buf + skipFiles := false for { if bufp >= nbuf { bufp = 0 @@ -62,7 +63,14 @@ func readDir(dirName string, fn func(dirName, entName string, typ os.FileMode) e } typ = fi.Mode() & os.ModeType } + if skipFiles && typ.IsRegular() { + continue + } if err := fn(dirName, name, typ); err != nil { + if err == SkipFiles { + skipFiles = true + continue + } return err } } diff --git a/internal/gopathwalk/walk.go b/internal/gopathwalk/walk.go new file mode 100644 index 0000000000..cbca5b0bbf --- /dev/null +++ b/internal/gopathwalk/walk.go @@ -0,0 +1,209 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package gopathwalk is like filepath.Walk but specialized for finding Go +// packages, particularly in $GOPATH and $GOROOT. +package gopathwalk + +import ( + "bufio" + "bytes" + "fmt" + "go/build" + "golang.org/x/tools/internal/fastwalk" + "io/ioutil" + "log" + "os" + "path/filepath" + "strings" +) + +// Options controls the behavior of a Walk call. +type Options struct { + Debug bool // Enable debug logging + ModulesEnabled bool // Search module caches. Also disables legacy goimports ignore rules. +} + +// Walk walks Go source directories ($GOROOT, $GOPATH, etc) to find packages. +// For each package found, add will be called (concurrently) with the absolute +// paths of the containing source directory and the package directory. +func Walk(add func(srcDir string, dir string), opts Options) { + for _, srcDir := range build.Default.SrcDirs() { + walkDir(srcDir, add, opts) + } +} + +func walkDir(srcDir string, add func(string, string), opts Options) { + if opts.Debug { + log.Printf("scanning %s", srcDir) + } + w := &walker{ + srcDir: srcDir, + srcV: filepath.Join(srcDir, "v"), + srcMod: filepath.Join(srcDir, "mod"), + add: add, + opts: opts, + } + w.init() + if err := fastwalk.Walk(srcDir, w.walk); err != nil { + log.Printf("goimports: scanning directory %v: %v", srcDir, err) + } + + if opts.Debug { + defer log.Printf("scanned %s", srcDir) + } +} + +// walker is the callback for fastwalk.Walk. +type walker struct { + srcDir string // The source directory to scan. + srcV, srcMod string // vgo-style module cache dirs. Optional. + add func(string, string) // The callback that will be invoked for every possible Go package dir. + opts Options // Options passed to Walk by the user. + + ignoredDirs []os.FileInfo // The ignored directories, loaded from .goimportsignore files. +} + +// init initializes the walker based on its Options. +func (w *walker) init() { + if !w.opts.ModulesEnabled { + w.ignoredDirs = w.getIgnoredDirs(w.srcDir) + } +} + +// getIgnoredDirs reads an optional config file at /.goimportsignore +// of relative directories to ignore when scanning for go files. +// The provided path is one of the $GOPATH entries with "src" appended. +func (w *walker) getIgnoredDirs(path string) []os.FileInfo { + file := filepath.Join(path, ".goimportsignore") + slurp, err := ioutil.ReadFile(file) + if w.opts.Debug { + if err != nil { + log.Print(err) + } else { + log.Printf("Read %s", file) + } + } + if err != nil { + return nil + } + + var ignoredDirs []os.FileInfo + bs := bufio.NewScanner(bytes.NewReader(slurp)) + for bs.Scan() { + line := strings.TrimSpace(bs.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + full := filepath.Join(path, line) + if fi, err := os.Stat(full); err == nil { + ignoredDirs = append(ignoredDirs, fi) + if w.opts.Debug { + log.Printf("Directory added to ignore list: %s", full) + } + } else if w.opts.Debug { + log.Printf("Error statting entry in .goimportsignore: %v", err) + } + } + return ignoredDirs +} + +func (w *walker) shouldSkipDir(fi os.FileInfo) bool { + for _, ignoredDir := range w.ignoredDirs { + if os.SameFile(fi, ignoredDir) { + return true + } + } + return false +} + +func (w *walker) walk(path string, typ os.FileMode) error { + if !w.opts.ModulesEnabled && (path == w.srcV || path == w.srcMod) { + return filepath.SkipDir + } + dir := filepath.Dir(path) + if typ.IsRegular() { + if dir == w.srcDir { + // Doesn't make sense to have regular files + // directly in your $GOPATH/src or $GOROOT/src. + return fastwalk.SkipFiles + } + if !strings.HasSuffix(path, ".go") { + return nil + } + + w.add(w.srcDir, dir) + return fastwalk.SkipFiles + } + if typ == os.ModeDir { + base := filepath.Base(path) + if base == "" || base[0] == '.' || base[0] == '_' || + base == "testdata" || (!w.opts.ModulesEnabled && base == "node_modules") { + return filepath.SkipDir + } + fi, err := os.Lstat(path) + if err == nil && w.shouldSkipDir(fi) { + return filepath.SkipDir + } + return nil + } + if typ == os.ModeSymlink { + base := filepath.Base(path) + if strings.HasPrefix(base, ".#") { + // Emacs noise. + return nil + } + fi, err := os.Lstat(path) + if err != nil { + // Just ignore it. + return nil + } + if w.shouldTraverse(dir, fi) { + return fastwalk.TraverseLink + } + } + return nil +} + +// shouldTraverse reports whether the symlink fi, found in dir, +// should be followed. It makes sure symlinks were never visited +// before to avoid symlink loops. +func (w *walker) shouldTraverse(dir string, fi os.FileInfo) bool { + path := filepath.Join(dir, fi.Name()) + target, err := filepath.EvalSymlinks(path) + if err != nil { + return false + } + ts, err := os.Stat(target) + if err != nil { + fmt.Fprintln(os.Stderr, err) + return false + } + if !ts.IsDir() { + return false + } + if w.shouldSkipDir(ts) { + return false + } + // Check for symlink loops by statting each directory component + // and seeing if any are the same file as ts. + for { + parent := filepath.Dir(path) + if parent == path { + // Made it to the root without seeing a cycle. + // Use this symlink. + return true + } + parentInfo, err := os.Stat(parent) + if err != nil { + return false + } + if os.SameFile(ts, parentInfo) { + // Cycle. Don't traverse. + return false + } + path = parent + } + +} diff --git a/internal/gopathwalk/walk_test.go b/internal/gopathwalk/walk_test.go new file mode 100644 index 0000000000..8e310c0fa9 --- /dev/null +++ b/internal/gopathwalk/walk_test.go @@ -0,0 +1,135 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package gopathwalk + +import ( + "io/ioutil" + "os" + "path/filepath" + "reflect" + "runtime" + "strings" + "testing" +) + +func TestShouldTraverse(t *testing.T) { + switch runtime.GOOS { + case "windows", "plan9": + t.Skipf("skipping symlink-requiring test on %s", runtime.GOOS) + } + + dir, err := ioutil.TempDir("", "goimports-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + // Note: mapToDir prepends "src" to each element, since + // mapToDir was made for creating GOPATHs. + if err := mapToDir(dir, map[string]string{ + "foo/foo2/file.txt": "", + "foo/foo2/link-to-src": "LINK:../../", + "foo/foo2/link-to-src-foo": "LINK:../../foo", + "foo/foo2/link-to-dot": "LINK:.", + "bar/bar2/file.txt": "", + "bar/bar2/link-to-src-foo": "LINK:../../foo", + + "a/b/c": "LINK:../../a/d", + "a/d/e": "LINK:../../a/b", + }); err != nil { + t.Fatal(err) + } + tests := []struct { + dir string + file string + want bool + }{ + { + dir: "src/foo/foo2", + file: "link-to-src-foo", + want: false, // loop + }, + { + dir: "src/foo/foo2", + file: "link-to-src", + want: false, // loop + }, + { + dir: "src/foo/foo2", + file: "link-to-dot", + want: false, // loop + }, + { + dir: "src/bar/bar2", + file: "link-to-src-foo", + want: true, // not a loop + }, + { + dir: "src/a/b/c", + file: "e", + want: false, // loop: "e" is the same as "b". + }, + } + for i, tt := range tests { + fi, err := os.Stat(filepath.Join(dir, tt.dir, tt.file)) + if err != nil { + t.Errorf("%d. Stat = %v", i, err) + continue + } + var w walker + got := w.shouldTraverse(filepath.Join(dir, tt.dir), fi) + if got != tt.want { + t.Errorf("%d. shouldTraverse(%q, %q) = %v; want %v", i, tt.dir, tt.file, got, tt.want) + } + } +} + +// TestSkip tests that various goimports rules are followed in non-modules mode. +func TestSkip(t *testing.T) { + dir, err := ioutil.TempDir("", "goimports-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + if err := mapToDir(dir, map[string]string{ + "ignoreme/f.go": "package ignoreme", // ignored by .goimportsignore + "node_modules/f.go": "package nodemodules;", // ignored by hardcoded node_modules filter + "v/f.go": "package v;", // ignored by hardcoded vgo cache rule + "mod/f.go": "package mod;", // ignored by hardcoded vgo cache rule + "shouldfind/f.go": "package shouldfind;", // not ignored + + ".goimportsignore": "ignoreme\n", + }); err != nil { + t.Fatal(err) + } + + var found []string + walkDir(filepath.Join(dir, "src"), func(srcDir string, dir string) { + found = append(found, dir[len(srcDir)+1:]) + }, Options{ModulesEnabled: false}) + if want := []string{"shouldfind"}; !reflect.DeepEqual(found, want) { + t.Errorf("expected to find only %v, got %v", want, found) + } +} + +func mapToDir(destDir string, files map[string]string) error { + for path, contents := range files { + file := filepath.Join(destDir, "src", path) + if err := os.MkdirAll(filepath.Dir(file), 0755); err != nil { + return err + } + var err error + if strings.HasPrefix(contents, "LINK:") { + err = os.Symlink(strings.TrimPrefix(contents, "LINK:"), file) + } else { + err = ioutil.WriteFile(file, []byte(contents), 0644) + } + if err != nil { + return err + } + } + return nil +}