internal/lsp: use directoryFilters in import scanning

Based on dle8's CL 414454, wire in directoryFilters into the goimports
ModuleResolver scan. A few changes/fixes/additions from that CL:

- Fix a bug in filter validation that was causing -**/a not to validate.
- Fix a bug in regex building where -a was not treated as an excluded
  prefix (it was matching 'a' anywhere).
- Use absolute paths in the SkipPathInScan, so that we can evaluate
  directory filters relative to the workspace folder.
- Add several regression tests.
- Consolidate directoryFilters regression tests under a new
  directoryfilters_test.go file.
- Add several TODOs.

Fixes golang/go#46438

Change-Id: I55cd3d6410905216cc8cfbdf528f301d67c2bbac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420959
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dylan Le <dungtuanle@google.com>

gopls/internal/regtest/workspace/directoryfilters_test.go

@@ -0,0 +1,252 @@
+// Copyright 2022 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 workspace
+
+import (
+	"sort"
+	"strings"
+	"testing"
+
+	. "golang.org/x/tools/internal/lsp/regtest"
+)
+
+// This file contains regression tests for the directoryFilters setting.
+//
+// TODO:
+//  - consolidate some of these tests into a single test
+//  - add more tests for changing directory filters
+
+func TestDirectoryFilters(t *testing.T) {
+	WithOptions(
+		ProxyFiles(workspaceProxy),
+		WorkspaceFolders("pkg"),
+		Settings{
+			"directoryFilters": []string{"-inner"},
+		},
+	).Run(t, workspaceModule, func(t *testing.T, env *Env) {
+		syms := env.WorkspaceSymbol("Hi")
+		sort.Slice(syms, func(i, j int) bool { return syms[i].ContainerName < syms[j].ContainerName })
+		for _, s := range syms {
+			if strings.Contains(s.ContainerName, "inner") {
+				t.Errorf("WorkspaceSymbol: found symbol %q with container %q, want \"inner\" excluded", s.Name, s.ContainerName)
+			}
+		}
+	})
+}
+
+func TestDirectoryFiltersLoads(t *testing.T) {
+	// exclude, and its error, should be excluded from the workspace.
+	const files = `
+-- go.mod --
+module example.com
+
+go 1.12
+-- exclude/exclude.go --
+package exclude
+
+const _ = Nonexistant
+`
+
+	WithOptions(
+		Settings{"directoryFilters": []string{"-exclude"}},
+	).Run(t, files, func(t *testing.T, env *Env) {
+		env.Await(NoDiagnostics("exclude/x.go"))
+	})
+}
+
+func TestDirectoryFiltersTransitiveDep(t *testing.T) {
+	// Even though exclude is excluded from the workspace, it should
+	// still be importable as a non-workspace package.
+	const files = `
+-- go.mod --
+module example.com
+
+go 1.12
+-- include/include.go --
+package include
+import "example.com/exclude"
+
+const _ = exclude.X
+-- exclude/exclude.go --
+package exclude
+
+const _ = Nonexistant // should be ignored, since this is a non-workspace package
+const X = 1
+`
+
+	WithOptions(
+		Settings{"directoryFilters": []string{"-exclude"}},
+	).Run(t, files, func(t *testing.T, env *Env) {
+		env.Await(
+			NoDiagnostics("exclude/exclude.go"), // filtered out
+			NoDiagnostics("include/include.go"), // successfully builds
+		)
+	})
+}
+
+func TestDirectoryFiltersWorkspaceModules(t *testing.T) {
+	// Define a module include.com which should be in the workspace, plus a
+	// module exclude.com which should be excluded and therefore come from
+	// the proxy.
+	const files = `
+-- include/go.mod --
+module include.com
+
+go 1.12
+
+require exclude.com v1.0.0
+
+-- include/go.sum --
+exclude.com v1.0.0 h1:Q5QSfDXY5qyNCBeUiWovUGqcLCRZKoTs9XdBeVz+w1I=
+exclude.com v1.0.0/go.mod h1:hFox2uDlNB2s2Jfd9tHlQVfgqUiLVTmh6ZKat4cvnj4=
+
+-- include/include.go --
+package include
+
+import "exclude.com"
+
+var _ = exclude.X // satisfied only by the workspace version
+-- exclude/go.mod --
+module exclude.com
+
+go 1.12
+-- exclude/exclude.go --
+package exclude
+
+const X = 1
+`
+	const proxy = `
+-- exclude.com@v1.0.0/go.mod --
+module exclude.com
+
+go 1.12
+-- exclude.com@v1.0.0/exclude.go --
+package exclude
+`
+	WithOptions(
+		Modes(Experimental),
+		ProxyFiles(proxy),
+		Settings{"directoryFilters": []string{"-exclude"}},
+	).Run(t, files, func(t *testing.T, env *Env) {
+		env.Await(env.DiagnosticAtRegexp("include/include.go", `exclude.(X)`))
+	})
+}
+
+// Test for golang/go#46438: support for '**' in directory filters.
+func TestDirectoryFilters_Wildcard(t *testing.T) {
+	filters := []string{"-**/bye"}
+	WithOptions(
+		ProxyFiles(workspaceProxy),
+		WorkspaceFolders("pkg"),
+		Settings{
+			"directoryFilters": filters,
+		},
+	).Run(t, workspaceModule, func(t *testing.T, env *Env) {
+		syms := env.WorkspaceSymbol("Bye")
+		sort.Slice(syms, func(i, j int) bool { return syms[i].ContainerName < syms[j].ContainerName })
+		for _, s := range syms {
+			if strings.Contains(s.ContainerName, "bye") {
+				t.Errorf("WorkspaceSymbol: found symbol %q with container %q with filters %v", s.Name, s.ContainerName, filters)
+			}
+		}
+	})
+}
+
+// Test for golang/go#52993: wildcard directoryFilters should apply to
+// goimports scanning as well.
+func TestDirectoryFilters_ImportScanning(t *testing.T) {
+	const files = `
+-- go.mod --
+module mod.test
+
+go 1.12
+-- main.go --
+package main
+
+func main() {
+	bye.Goodbye()
+}
+-- p/bye/bye.go --
+package bye
+
+func Goodbye() {}
+`
+
+	WithOptions(
+		Settings{
+			"directoryFilters": []string{"-**/bye"},
+		},
+		// This test breaks in 'Experimental' mode, because with
+		// experimentalWorkspaceModule set we the goimports scan behaves
+		// differently.
+		//
+		// Since this feature is going away (golang/go#52897), don't investigate.
+		Modes(Default),
+	).Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("main.go")
+		beforeSave := env.Editor.BufferText("main.go")
+		env.OrganizeImports("main.go")
+		got := env.Editor.BufferText("main.go")
+		if got != beforeSave {
+			t.Errorf("after organizeImports code action, got modified buffer:\n%s", got)
+		}
+	})
+}
+
+// Test for golang/go#52993: non-wildcard directoryFilters should still be
+// applied relative to the workspace folder, not the module root.
+func TestDirectoryFilters_MultiRootImportScanning(t *testing.T) {
+	const files = `
+-- go.work --
+go 1.18
+
+use (
+	a
+	b
+)
+-- a/go.mod --
+module mod1.test
+
+go 1.18
+-- a/main.go --
+package main
+
+func main() {
+	hi.Hi()
+}
+-- a/hi/hi.go --
+package hi
+
+func Hi() {}
+-- b/go.mod --
+module mod2.test
+
+go 1.18
+-- b/main.go --
+package main
+
+func main() {
+	hi.Hi()
+}
+-- b/hi/hi.go --
+package hi
+
+func Hi() {}
+`
+
+	WithOptions(
+		Settings{
+			"directoryFilters": []string{"-hi"}, // this test fails with -**/hi
+		},
+	).Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("a/main.go")
+		beforeSave := env.Editor.BufferText("a/main.go")
+		env.OrganizeImports("a/main.go")
+		got := env.Editor.BufferText("a/main.go")
+		if got == beforeSave {
+			t.Errorf("after organizeImports code action, got identical buffer:\n%s", got)
+		}
+	})
+}

gopls/internal/regtest/workspace/workspace_test.go

@@ -7,7 +7,6 @@ package workspace
 import (
 	"fmt"
 	"path/filepath"
-	"sort"
 	"strings"
 	"testing"
 
@@ -137,24 +136,6 @@ func TestReferences(t *testing.T) {
 	}
 }
 
-func TestDirectoryFilters(t *testing.T) {
-	WithOptions(
-		ProxyFiles(workspaceProxy),
-		WorkspaceFolders("pkg"),
-		Settings{
-			"directoryFilters": []string{"-inner"},
-		},
-	).Run(t, workspaceModule, func(t *testing.T, env *Env) {
-		syms := env.WorkspaceSymbol("Hi")
-		sort.Slice(syms, func(i, j int) bool { return syms[i].ContainerName < syms[j].ContainerName })
-		for _, s := range syms {
-			if strings.Contains(s.ContainerName, "inner") {
-				t.Errorf("WorkspaceSymbol: found symbol %q with container %q, want \"inner\" excluded", s.Name, s.ContainerName)
-			}
-		}
-	})
-}
-
 // Make sure that analysis diagnostics are cleared for the whole package when
 // the only opened file is closed. This test was inspired by the experience in
 // VS Code, where clicking on a reference result triggers a
@@ -994,104 +975,6 @@ func main() {
 	})
 }
 
-func TestDirectoryFiltersLoads(t *testing.T) {
-	// exclude, and its error, should be excluded from the workspace.
-	const files = `
--- go.mod --
-module example.com
-
-go 1.12
--- exclude/exclude.go --
-package exclude
-
-const _ = Nonexistant
-`
-
-	WithOptions(
-		Settings{"directoryFilters": []string{"-exclude"}},
-	).Run(t, files, func(t *testing.T, env *Env) {
-		env.Await(NoDiagnostics("exclude/x.go"))
-	})
-}
-
-func TestDirectoryFiltersTransitiveDep(t *testing.T) {
-	// Even though exclude is excluded from the workspace, it should
-	// still be importable as a non-workspace package.
-	const files = `
--- go.mod --
-module example.com
-
-go 1.12
--- include/include.go --
-package include
-import "example.com/exclude"
-
-const _ = exclude.X
--- exclude/exclude.go --
-package exclude
-
-const _ = Nonexistant // should be ignored, since this is a non-workspace package
-const X = 1
-`
-
-	WithOptions(
-		Settings{"directoryFilters": []string{"-exclude"}},
-	).Run(t, files, func(t *testing.T, env *Env) {
-		env.Await(
-			NoDiagnostics("exclude/exclude.go"), // filtered out
-			NoDiagnostics("include/include.go"), // successfully builds
-		)
-	})
-}
-
-func TestDirectoryFiltersWorkspaceModules(t *testing.T) {
-	// Define a module include.com which should be in the workspace, plus a
-	// module exclude.com which should be excluded and therefore come from
-	// the proxy.
-	const files = `
--- include/go.mod --
-module include.com
-
-go 1.12
-
-require exclude.com v1.0.0
-
--- include/go.sum --
-exclude.com v1.0.0 h1:Q5QSfDXY5qyNCBeUiWovUGqcLCRZKoTs9XdBeVz+w1I=
-exclude.com v1.0.0/go.mod h1:hFox2uDlNB2s2Jfd9tHlQVfgqUiLVTmh6ZKat4cvnj4=
-
--- include/include.go --
-package include
-
-import "exclude.com"
-
-var _ = exclude.X // satisfied only by the workspace version
--- exclude/go.mod --
-module exclude.com
-
-go 1.12
--- exclude/exclude.go --
-package exclude
-
-const X = 1
-`
-	const proxy = `
--- exclude.com@v1.0.0/go.mod --
-module exclude.com
-
-go 1.12
--- exclude.com@v1.0.0/exclude.go --
-package exclude
-`
-	WithOptions(
-		Modes(Experimental),
-		ProxyFiles(proxy),
-		Settings{"directoryFilters": []string{"-exclude"}},
-	).Run(t, files, func(t *testing.T, env *Env) {
-		env.Await(env.DiagnosticAtRegexp("include/include.go", `exclude.(X)`))
-	})
-}
-
 // Confirm that a fix for a tidy module will correct all modules in the
 // workspace.
 func TestMultiModule_OneBrokenModule(t *testing.T) {

internal/imports/fix.go

@@ -807,6 +807,11 @@ type ProcessEnv struct {
 	ModFlag    string
 	ModFile    string
 
+	// SkipPathInScan returns true if the path should be skipped from scans of
+	// the RootCurrentModule root type. The function argument is a clean,
+	// absolute path.
+	SkipPathInScan func(string) bool
+
 	// Env overrides the OS environment, and can be used to specify
 	// GOPROXY, GO111MODULE, etc. PATH cannot be set here, because
 	// exec.Command will not honor it.

internal/imports/mod.go

@@ -466,6 +466,16 @@ func (r *ModuleResolver) scan(ctx context.Context, callback *scanCallback) error
 	// We assume cached directories are fully cached, including all their
 	// children, and have not changed. We can skip them.
 	skip := func(root gopathwalk.Root, dir string) bool {
+		if r.env.SkipPathInScan != nil && root.Type == gopathwalk.RootCurrentModule {
+			if root.Path == dir {
+				return false
+			}
+
+			if r.env.SkipPathInScan(filepath.Clean(dir)) {
+				return true
+			}
+		}
+
 		info, ok := r.cacheLoad(dir)
 		if !ok {
 			return false

internal/lsp/cache/imports.go

@@ -23,13 +23,14 @@ import (
 type importsState struct {
 	ctx context.Context
 
-	mu                   sync.Mutex
-	processEnv           *imports.ProcessEnv
-	cleanupProcessEnv    func()
-	cacheRefreshDuration time.Duration
-	cacheRefreshTimer    *time.Timer
-	cachedModFileHash    source.Hash
-	cachedBuildFlags     []string
+	mu                     sync.Mutex
+	processEnv             *imports.ProcessEnv
+	cleanupProcessEnv      func()
+	cacheRefreshDuration   time.Duration
+	cacheRefreshTimer      *time.Timer
+	cachedModFileHash      source.Hash
+	cachedBuildFlags       []string
+	cachedDirectoryFilters []string
 }
 
 func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot, fn func(*imports.Options) error) error {
@@ -70,9 +71,11 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot
 	snapshot.view.optionsMu.Lock()
 	localPrefix := snapshot.view.options.Local
 	currentBuildFlags := snapshot.view.options.BuildFlags
+	currentDirectoryFilters := snapshot.view.options.DirectoryFilters
 	changed := !reflect.DeepEqual(currentBuildFlags, s.cachedBuildFlags) ||
 		snapshot.view.options.VerboseOutput != (s.processEnv.Logf != nil) ||
-		modFileHash != s.cachedModFileHash
+		modFileHash != s.cachedModFileHash ||
+		!reflect.DeepEqual(snapshot.view.options.DirectoryFilters, s.cachedDirectoryFilters)
 	snapshot.view.optionsMu.Unlock()
 
 	// If anything relevant to imports has changed, clear caches and
@@ -92,6 +95,7 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot
 		}
 		s.cachedModFileHash = modFileHash
 		s.cachedBuildFlags = currentBuildFlags
+		s.cachedDirectoryFilters = currentDirectoryFilters
 		var err error
 		s.cleanupProcessEnv, err = s.populateProcessEnv(ctx, snapshot)
 		if err != nil {

internal/lsp/cache/session.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"fmt"
 	"strconv"
+	"strings"
 	"sync"
 	"sync/atomic"
 
@@ -228,6 +229,17 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
 		ctx: backgroundCtx,
 		processEnv: &imports.ProcessEnv{
 			GocmdRunner: s.gocmdRunner,
+			SkipPathInScan: func(dir string) bool {
+				prefix := strings.TrimSuffix(string(v.folder), "/") + "/"
+				uri := strings.TrimSuffix(string(span.URIFromPath(dir)), "/")
+				if !strings.HasPrefix(uri+"/", prefix) {
+					return false
+				}
+				filterer := source.NewFilterer(options.DirectoryFilters)
+				rel := strings.TrimPrefix(uri, prefix)
+				disallow := filterer.Disallow(rel)
+				return disallow
+			},
 		},
 	}
 	v.snapshot = &snapshot{

internal/lsp/cache/view.go

@@ -1136,6 +1136,8 @@ func pathExcludedByFilter(path string, filterer *source.Filterer) bool {
 }
 
 func buildFilterer(root, gomodcache string, opts *source.Options) *source.Filterer {
+	// TODO(rfindley): this looks wrong. If gomodcache isn't actually nested
+	// under root, this will do the wrong thing.
 	gomodcache = strings.TrimPrefix(filepath.ToSlash(strings.TrimPrefix(gomodcache, root)), "/")
 	filters := opts.DirectoryFilters
 	if gomodcache != "" {

internal/lsp/source/options.go

@@ -840,7 +840,7 @@ func validateDirectoryFilter(ifilter string) (string, error) {
 	if filter == "" || (filter[0] != '+' && filter[0] != '-') {
 		return "", fmt.Errorf("invalid filter %v, must start with + or -", filter)
 	}
-	segs := strings.Split(filter, "/")
+	segs := strings.Split(filter[1:], "/")
 	unsupportedOps := [...]string{"?", "*"}
 	for _, seg := range segs {
 		if seg != "**" {

internal/lsp/source/workspace_symbol.go

@@ -406,16 +406,15 @@ func (f *Filterer) Disallow(path string) bool {
 // Supporting glob-like operators:
 //   - **: match zero or more complete path segments
 func convertFilterToRegexp(filter string) *regexp.Regexp {
+	if filter == "" {
+		return regexp.MustCompile(".*")
+	}
 	var ret strings.Builder
+	ret.WriteString("^")
 	segs := strings.Split(filter, "/")
-	for i, seg := range segs {
+	for _, seg := range segs {
 		if seg == "**" {
-			switch i {
-			case 0:
-				ret.WriteString("^.*")
-			default:
-				ret.WriteString(".*")
-			}
+			ret.WriteString(".*")
 		} else {
 			ret.WriteString(regexp.QuoteMeta(seg))
 		}

internal/lsp/source/workspace_symbol_test.go

@@ -112,6 +112,11 @@ func TestFiltererDisallow(t *testing.T) {
 			[]string{"a/b/c.go"},
 			[]string{},
 		},
+		{
+			[]string{"-b"}, // should only filter paths prefixed with the "b" directory
+			[]string{"a/b/c.go", "bb"},
+			[]string{"b/c/d.go", "b"},
+		},
 	}
 
 	for _, test := range tests {