diff --git a/gopls/internal/regtest/workspace/directoryfilters_test.go b/gopls/internal/regtest/workspace/directoryfilters_test.go new file mode 100644 index 0000000000..bdc60a06ee --- /dev/null +++ b/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) + } + }) +} diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index d7a0c7f7e1..86da9d1c93 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/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) { diff --git a/internal/imports/fix.go b/internal/imports/fix.go index 9e373d64eb..45a492ef03 100644 --- a/internal/imports/fix.go +++ b/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. diff --git a/internal/imports/mod.go b/internal/imports/mod.go index 46693f2433..dec388bc09 100644 --- a/internal/imports/mod.go +++ b/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 diff --git a/internal/lsp/cache/imports.go b/internal/lsp/cache/imports.go index a08953db64..6510bbd573 100644 --- a/internal/lsp/cache/imports.go +++ b/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 { diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index a77deb6211..984e8c1e75 100644 --- a/internal/lsp/cache/session.go +++ b/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{ diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index cddd4fa5d6..61501098c0 100644 --- a/internal/lsp/cache/view.go +++ b/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 != "" { diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 6881a7cb82..971fa069d4 100644 --- a/internal/lsp/source/options.go +++ b/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 != "**" { diff --git a/internal/lsp/source/workspace_symbol.go b/internal/lsp/source/workspace_symbol.go index 6c33effc1a..e9da569d02 100644 --- a/internal/lsp/source/workspace_symbol.go +++ b/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)) } diff --git a/internal/lsp/source/workspace_symbol_test.go b/internal/lsp/source/workspace_symbol_test.go index 633550ed94..24fb8b4521 100644 --- a/internal/lsp/source/workspace_symbol_test.go +++ b/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 {