diff --git a/internal/gopathwalk/walk.go b/internal/gopathwalk/walk.go index 9c07e4c2d..60eb67b69 100644 --- a/internal/gopathwalk/walk.go +++ b/internal/gopathwalk/walk.go @@ -59,12 +59,24 @@ func SrcDirsRoots(ctx *build.Context) []Root { // paths of the containing source directory and the package directory. // add will be called concurrently. func Walk(roots []Root, add func(root Root, dir string), opts Options) { + WalkSkip(roots, add, func(Root, string) bool { return false }, opts) +} + +// WalkSkip 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. +// For each directory that will be scanned, skip will be called (concurrently) +// with the absolute paths of the containing source directory and the directory. +// If skip returns false on a directory it will be processed. +// add will be called concurrently. +// skip will be called concurrently. +func WalkSkip(roots []Root, add func(root Root, dir string), skip func(root Root, dir string) bool, opts Options) { for _, root := range roots { - walkDir(root, add, opts) + walkDir(root, add, skip, opts) } } -func walkDir(root Root, add func(Root, string), opts Options) { +func walkDir(root Root, add func(Root, string), skip func(root Root, dir string) bool, opts Options) { if _, err := os.Stat(root.Path); os.IsNotExist(err) { if opts.Debug { log.Printf("skipping nonexistent directory: %v", root.Path) @@ -77,6 +89,7 @@ func walkDir(root Root, add func(Root, string), opts Options) { w := &walker{ root: root, add: add, + skip: skip, opts: opts, } w.init() @@ -91,9 +104,10 @@ func walkDir(root Root, add func(Root, string), opts Options) { // walker is the callback for fastwalk.Walk. type walker struct { - root Root // The source directory to scan. - add func(Root, string) // The callback that will be invoked for every possible Go package dir. - opts Options // Options passed to Walk by the user. + root Root // The source directory to scan. + add func(Root, string) // The callback that will be invoked for every possible Go package dir. + skip func(Root, string) bool // The callback that will be invoked for every dir. dir is skipped if it returns true. + opts Options // Options passed to Walk by the user. ignoredDirs []os.FileInfo // The ignored directories, loaded from .goimportsignore files. } @@ -151,12 +165,16 @@ func (w *walker) getIgnoredDirs(path string) []string { return ignoredDirs } -func (w *walker) shouldSkipDir(fi os.FileInfo) bool { +func (w *walker) shouldSkipDir(fi os.FileInfo, dir string) bool { for _, ignoredDir := range w.ignoredDirs { if os.SameFile(fi, ignoredDir) { return true } } + if w.skip != nil { + // Check with the user specified callback. + return w.skip(w.root, dir) + } return false } @@ -184,7 +202,7 @@ func (w *walker) walk(path string, typ os.FileMode) error { return filepath.SkipDir } fi, err := os.Lstat(path) - if err == nil && w.shouldSkipDir(fi) { + if err == nil && w.shouldSkipDir(fi, path) { return filepath.SkipDir } return nil @@ -224,7 +242,7 @@ func (w *walker) shouldTraverse(dir string, fi os.FileInfo) bool { if !ts.IsDir() { return false } - if w.shouldSkipDir(ts) { + if w.shouldSkipDir(ts, dir) { return false } // Check for symlink loops by statting each directory component diff --git a/internal/gopathwalk/walk_test.go b/internal/gopathwalk/walk_test.go index 0a1652d61..7d48e5e13 100644 --- a/internal/gopathwalk/walk_test.go +++ b/internal/gopathwalk/walk_test.go @@ -11,6 +11,7 @@ import ( "reflect" "runtime" "strings" + "sync" "testing" ) @@ -107,9 +108,47 @@ func TestSkip(t *testing.T) { } var found []string - walkDir(Root{filepath.Join(dir, "src"), RootGOPATH}, func(root Root, dir string) { - found = append(found, dir[len(root.Path)+1:]) - }, Options{ModulesEnabled: false, Debug: true}) + var mu sync.Mutex + walkDir(Root{filepath.Join(dir, "src"), RootGOPATH}, + func(root Root, dir string) { + mu.Lock() + defer mu.Unlock() + found = append(found, dir[len(root.Path)+1:]) + }, func(root Root, dir string) bool { + return false + }, Options{ModulesEnabled: false, Debug: true}) + if want := []string{"shouldfind"}; !reflect.DeepEqual(found, want) { + t.Errorf("expected to find only %v, got %v", want, found) + } +} + +// TestSkipFunction tests that scan successfully skips directories from user callback. +func TestSkipFunction(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 skip + "ignoreme/subignore/f.go": "package subignore", // also ignored by skip + "shouldfind/f.go": "package shouldfind;", // not ignored + }); err != nil { + t.Fatal(err) + } + + var found []string + var mu sync.Mutex + walkDir(Root{filepath.Join(dir, "src"), RootGOPATH}, + func(root Root, dir string) { + mu.Lock() + defer mu.Unlock() + found = append(found, dir[len(root.Path)+1:]) + }, func(root Root, dir string) bool { + return strings.HasSuffix(dir, "ignoreme") + }, + Options{ModulesEnabled: false}) if want := []string{"shouldfind"}; !reflect.DeepEqual(found, want) { t.Errorf("expected to find only %v, got %v", want, found) } diff --git a/internal/imports/mod.go b/internal/imports/mod.go index 98d5ff348..dc74e3985 100644 --- a/internal/imports/mod.go +++ b/internal/imports/mod.go @@ -31,10 +31,6 @@ type ModuleResolver struct { ModsByModPath []*ModuleJSON // All modules, ordered by # of path components in module Path... ModsByDir []*ModuleJSON // ...or Dir. - // ModCachePkgs contains the canonicalized importPath and directory of packages - // in the module cache. Keyed by absolute directory. - ModCachePkgs map[string]*pkg - // moduleCacheInfo stores information about the module cache. moduleCacheInfo *moduleCacheInfo } @@ -97,7 +93,6 @@ func (r *ModuleResolver) init() error { return count(j) < count(i) // descending order }) - r.ModCachePkgs = make(map[string]*pkg) if r.moduleCacheInfo == nil { r.moduleCacheInfo = &moduleCacheInfo{ modCacheDirInfo: make(map[string]*directoryPackageInfo), @@ -268,85 +263,114 @@ func (r *ModuleResolver) scan(_ references) ([]*pkg, error) { dupCheck := make(map[string]bool) var mu sync.Mutex - gopathwalk.Walk(roots, func(root gopathwalk.Root, dir string) { + // Packages in the module cache are immutable. If we have + // already seen this package on a previous scan of the module + // cache, return that result. + skip := func(root gopathwalk.Root, dir string) bool { mu.Lock() defer mu.Unlock() + // If we have already processed this directory on this walk, skip it. + if _, dup := dupCheck[dir]; dup { + return true + } + // If we have saved this directory information, skip it. + info, ok := r.moduleCacheInfo.Load(dir) + if !ok { + return false + } + // This directory can be skipped as long as we have already scanned it. + // Packages with errors will continue to have errors, so there is no need + // to rescan them. + packageScanned, _ := info.reachedStatus(directoryScanned) + return packageScanned + } + + add := func(root gopathwalk.Root, dir string) { + mu.Lock() + defer mu.Unlock() if _, dup := dupCheck[dir]; dup { return } - dupCheck[dir] = true - - absDir := dir - // Packages in the module cache are immutable. If we have - // already seen this package on a previous scan of the module - // cache, return that result. - if p, ok := r.ModCachePkgs[absDir]; ok { - result = append(result, p) + info, err := r.scanDirForPackage(root, dir) + if err != nil { + return + } + if root.Type == gopathwalk.RootModuleCache { + // Save this package information in the cache and return. + // Packages from the module cache are added after Walk. + r.moduleCacheInfo.Store(dir, info) return } - info, ok := r.moduleCacheInfo.Load(dir) - if !ok { - var err error - info, err = r.scanDirForPackage(root, dir) - if err != nil { - return - } - if root.Type == gopathwalk.RootModuleCache { - r.moduleCacheInfo.Store(dir, info) - } - } - - if info.status < directoryScanned || - (info.status == directoryScanned && info.err != nil) { + // Skip this package if there was an error loading package info. + if info.err != nil { return } // The rest of this function canonicalizes the packages using the results // of initializing the resolver from 'go list -m'. - importPath := info.nonCanonicalImportPath - - // Check if the directory is underneath a module that's in scope. - if mod := r.findModuleByDir(dir); mod != nil { - // It is. If dir is the target of a replace directive, - // our guessed import path is wrong. Use the real one. - if mod.Dir == dir { - importPath = mod.Path - } else { - dirInMod := dir[len(mod.Dir)+len("/"):] - importPath = path.Join(mod.Path, filepath.ToSlash(dirInMod)) - } - } else if info.needsReplace { - // This package needed a replace target we don't have. + res, err := r.canonicalize(info.nonCanonicalImportPath, info.dir, info.needsReplace) + if err != nil { return } - // We may have discovered a package that has a different version - // in scope already. Canonicalize to that one if possible. - if _, canonicalDir := r.findPackage(importPath); canonicalDir != "" { - dir = canonicalDir - } - - res := &pkg{ - importPathShort: VendorlessPath(importPath), - dir: dir, - } - - if root.Type == gopathwalk.RootModuleCache { - // Save the results of processing this directory. - // This needs to be invalidated when the results of - // 'go list -m' would change, as the directory and - // importPath in this map depend on those results. - r.ModCachePkgs[absDir] = res - } - result = append(result, res) - }, gopathwalk.Options{Debug: r.env.Debug, ModulesEnabled: true}) + } + + gopathwalk.WalkSkip(roots, add, skip, gopathwalk.Options{Debug: r.env.Debug, ModulesEnabled: true}) + + // Add the packages from the modules in the mod cache that were skipped. + for _, dir := range r.moduleCacheInfo.Keys() { + info, ok := r.moduleCacheInfo.Load(dir) + if !ok { + continue + } + + // Skip this directory if we were not able to get the package information successfully. + if scanned, err := info.reachedStatus(directoryScanned); !scanned || err != nil { + continue + } + + res, err := r.canonicalize(info.nonCanonicalImportPath, info.dir, info.needsReplace) + if err != nil { + continue + } + result = append(result, res) + } + return result, nil } +// canonicalize gets the result of canonicalizing the packages using the results +// of initializing the resolver from 'go list -m'. +func (r *ModuleResolver) canonicalize(importPath, dir string, needsReplace bool) (res *pkg, err error) { + // Check if the directory is underneath a module that's in scope. + if mod := r.findModuleByDir(dir); mod != nil { + // It is. If dir is the target of a replace directive, + // our guessed import path is wrong. Use the real one. + if mod.Dir == dir { + importPath = mod.Path + } else { + dirInMod := dir[len(mod.Dir)+len("/"):] + importPath = path.Join(mod.Path, filepath.ToSlash(dirInMod)) + } + } else if needsReplace { + return nil, fmt.Errorf("needed this package to be in scope: %s", dir) + } + + // We may have discovered a package that has a different version + // in scope already. Canonicalize to that one if possible. + if _, canonicalDir := r.findPackage(importPath); canonicalDir != "" { + dir = canonicalDir + } + return &pkg{ + importPathShort: VendorlessPath(importPath), + dir: dir, + }, nil +} + func (r *ModuleResolver) loadExports(ctx context.Context, expectPackage string, pkg *pkg) (map[string]bool, error) { if err := r.init(); err != nil { return nil, err diff --git a/internal/imports/mod_cache.go b/internal/imports/mod_cache.go index ad28483df..f96b92d00 100644 --- a/internal/imports/mod_cache.go +++ b/internal/imports/mod_cache.go @@ -109,3 +109,13 @@ func (d *moduleCacheInfo) Load(dir string) (directoryPackageInfo, bool) { } return *info, true } + +// Keys returns the keys currently present in d. +func (d *moduleCacheInfo) Keys() (keys []string) { + d.mu.Lock() + defer d.mu.Unlock() + for key := range d.modCacheDirInfo { + keys = append(keys, key) + } + return keys +} diff --git a/internal/imports/mod_cache_test.go b/internal/imports/mod_cache_test.go index 7d04560f2..0eeaf17fb 100644 --- a/internal/imports/mod_cache_test.go +++ b/internal/imports/mod_cache_test.go @@ -2,6 +2,7 @@ package imports import ( "fmt" + "sort" "testing" ) @@ -50,3 +51,73 @@ func TestDirectoryPackageInfoReachedStatus(t *testing.T) { } } } + +func TestModCacheInfo(t *testing.T) { + m := &moduleCacheInfo{ + modCacheDirInfo: make(map[string]*directoryPackageInfo), + } + + dirInfo := []struct { + dir string + info directoryPackageInfo + }{ + { + dir: "mypackage", + info: directoryPackageInfo{ + status: directoryScanned, + dir: "mypackage", + nonCanonicalImportPath: "example.com/mypackage", + needsReplace: false, + }, + }, + { + dir: "bad package", + info: directoryPackageInfo{ + status: directoryScanned, + err: fmt.Errorf("bad package"), + }, + }, + { + dir: "mypackage/other", + info: directoryPackageInfo{ + dir: "mypackage/other", + nonCanonicalImportPath: "example.com/mypackage/other", + needsReplace: false, + }, + }, + } + + for _, d := range dirInfo { + m.Store(d.dir, d.info) + } + + for _, d := range dirInfo { + val, ok := m.Load(d.dir) + if !ok { + t.Errorf("directory not loaded: %s", d.dir) + } + + if val != d.info { + t.Errorf("expected: %v, got: %v", d.info, val) + } + } + + var wantKeys []string + for _, d := range dirInfo { + wantKeys = append(wantKeys, d.dir) + } + sort.Strings(wantKeys) + + gotKeys := m.Keys() + sort.Strings(gotKeys) + + if len(gotKeys) != len(wantKeys) { + t.Errorf("different length of keys. expected: %d, got: %d", len(wantKeys), len(gotKeys)) + } + + for i, want := range wantKeys { + if want != gotKeys[i] { + t.Errorf("%d: expected %s, got %s", i, want, gotKeys[i]) + } + } +} diff --git a/internal/imports/mod_test.go b/internal/imports/mod_test.go index d2059c5a7..da220e9a5 100644 --- a/internal/imports/mod_test.go +++ b/internal/imports/mod_test.go @@ -137,6 +137,25 @@ import _ "example.com" } +// Tests that scanning the module cache > 1 time is able to find the same module +// in the module cache. +func TestModMultipleScansWithSubdirs(t *testing.T) { + mt := setup(t, ` +-- go.mod -- +module x + +require rsc.io/quote v1.5.2 + +-- x.go -- +package x +import _ "rsc.io/quote" +`, "") + defer mt.cleanup() + + mt.assertScanFinds("rsc.io/quote/buggy", "buggy") + mt.assertScanFinds("rsc.io/quote/buggy", "buggy") +} + // Tests that scanning the module cache > 1 after changing a package in module cache to make it unimportable // is able to find the same module. func TestModCacheEditModFile(t *testing.T) { @@ -151,6 +170,9 @@ import _ "rsc.io/quote" `, "") defer mt.cleanup() found := mt.assertScanFinds("rsc.io/quote", "quote") + if found == nil { + t.Fatal("rsc.io/quote not found in initial scan.") + } // Update the go.mod file of example.com so that it changes its module path (not allowed). if err := os.Chmod(filepath.Join(found.dir, "go.mod"), 0644); err != nil { @@ -176,7 +198,6 @@ import _ "rsc.io/quote" mt.resolver.Main = nil mt.resolver.ModsByModPath = nil mt.resolver.ModsByDir = nil - mt.resolver.ModCachePkgs = nil mt.assertScanFinds("rsc.io/quote", "quote") }