diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 8937f93403..d0942b51bf 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -156,6 +156,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf } moduleErrs := make(map[string][]packages.Error) // module path -> errors + filterer := buildFilterer(s.view.rootURI.Filename(), s.view.gomodcache, s.view.options) newMetadata := make(map[PackageID]*KnownMetadata) for _, pkg := range pkgs { // The Go command returns synthetic list results for module queries that @@ -201,7 +202,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf // // TODO(rfindley): why exclude metadata arbitrarily here? It should be safe // to capture all metadata. - if s.view.allFilesExcluded(pkg) { + if s.view.allFilesExcluded(pkg, filterer) { continue } if err := buildMetadata(ctx, PackagePath(pkg.PkgPath), pkg, cfg, query, newMetadata, nil); err != nil { @@ -581,10 +582,11 @@ func containsPackageLocked(s *snapshot, m *Metadata) bool { uris[uri] = struct{}{} } + filterFunc := s.view.filterFunc() for uri := range uris { // Don't use view.contains here. go.work files may include modules // outside of the workspace folder. - if !strings.Contains(string(uri), "/vendor/") && !s.view.filters(uri) { + if !strings.Contains(string(uri), "/vendor/") && !filterFunc(uri) { return true } } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 15a2f90da5..e33e340045 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -385,13 +385,14 @@ func (s *snapshot) locateTemplateFiles(ctx context.Context) { relativeTo := s.view.folder.Filename() searched := 0 + filterer := buildFilterer(dir, s.view.gomodcache, s.view.options) // Change to WalkDir when we move up to 1.16 err := filepath.Walk(dir, func(path string, fi os.FileInfo, err error) error { if err != nil { return err } relpath := strings.TrimPrefix(path, relativeTo) - excluded := pathExcludedByFilter(relpath, dir, s.view.gomodcache, s.view.options) + excluded := pathExcludedByFilter(relpath, filterer) if fileHasExtension(path, suffixes) && !excluded && !fi.IsDir() { k := span.URIFromPath(path) _, err := s.GetVersionedFile(ctx, k) @@ -421,17 +422,20 @@ func (v *View) contains(uri span.URI) bool { return false } - return !v.filters(uri) + return !v.filterFunc()(uri) } -// filters reports whether uri is filtered by the currently configured +// filterFunc returns a func that reports whether uri is filtered by the currently configured // directoryFilters. -func (v *View) filters(uri span.URI) bool { - // Only filter relative to the configured root directory. - if source.InDirLex(v.folder.Filename(), uri.Filename()) { - return pathExcludedByFilter(strings.TrimPrefix(uri.Filename(), v.folder.Filename()), v.rootURI.Filename(), v.gomodcache, v.Options()) +func (v *View) filterFunc() func(span.URI) bool { + filterer := buildFilterer(v.rootURI.Filename(), v.gomodcache, v.Options()) + return func(uri span.URI) bool { + // Only filter relative to the configured root directory. + if source.InDirLex(v.folder.Filename(), uri.Filename()) { + return pathExcludedByFilter(strings.TrimPrefix(uri.Filename(), v.folder.Filename()), filterer) + } + return false } - return false } func (v *View) mapFile(uri span.URI, f *fileBase) { @@ -1070,15 +1074,14 @@ func (s *snapshot) vendorEnabled(ctx context.Context, modURI span.URI, modConten return vendorEnabled, nil } -func (v *View) allFilesExcluded(pkg *packages.Package) bool { - opts := v.Options() +func (v *View) allFilesExcluded(pkg *packages.Package, filterer *source.Filterer) bool { folder := filepath.ToSlash(v.folder.Filename()) for _, f := range pkg.GoFiles { f = filepath.ToSlash(f) if !strings.HasPrefix(f, folder) { return false } - if !pathExcludedByFilter(strings.TrimPrefix(f, folder), v.rootURI.Filename(), v.gomodcache, opts) { + if !pathExcludedByFilter(strings.TrimPrefix(f, folder), filterer) { return false } } @@ -1086,8 +1089,9 @@ func (v *View) allFilesExcluded(pkg *packages.Package) bool { } func pathExcludedByFilterFunc(root, gomodcache string, opts *source.Options) func(string) bool { + filterer := buildFilterer(root, gomodcache, opts) return func(path string) bool { - return pathExcludedByFilter(path, root, gomodcache, opts) + return pathExcludedByFilter(path, filterer) } } @@ -1097,12 +1101,16 @@ func pathExcludedByFilterFunc(root, gomodcache string, opts *source.Options) fun // TODO(rfindley): passing root and gomodcache here makes it confusing whether // path should be absolute or relative, and has already caused at least one // bug. -func pathExcludedByFilter(path, root, gomodcache string, opts *source.Options) bool { +func pathExcludedByFilter(path string, filterer *source.Filterer) bool { path = strings.TrimPrefix(filepath.ToSlash(path), "/") + return filterer.Disallow(path) +} + +func buildFilterer(root, gomodcache string, opts *source.Options) *source.Filterer { gomodcache = strings.TrimPrefix(filepath.ToSlash(strings.TrimPrefix(gomodcache, root)), "/") filters := opts.DirectoryFilters if gomodcache != "" { filters = append(filters, "-"+gomodcache) } - return source.FiltersDisallow(path, filters) + return source.NewFilterer(filters) } diff --git a/internal/lsp/cache/view_test.go b/internal/lsp/cache/view_test.go index d76dcda8ed..59684ea361 100644 --- a/internal/lsp/cache/view_test.go +++ b/internal/lsp/cache/view_test.go @@ -161,15 +161,14 @@ func TestFilters(t *testing.T) { } for _, tt := range tests { - opts := &source.Options{} - opts.DirectoryFilters = tt.filters + filterer := source.NewFilterer(tt.filters) for _, inc := range tt.included { - if pathExcludedByFilter(inc, "root", "root/gopath/pkg/mod", opts) { + if pathExcludedByFilter(inc, filterer) { t.Errorf("filters %q excluded %v, wanted included", tt.filters, inc) } } for _, exc := range tt.excluded { - if !pathExcludedByFilter(exc, "root", "root/gopath/pkg/mod", opts) { + if !pathExcludedByFilter(exc, filterer) { t.Errorf("filters %q included %v, wanted excluded", tt.filters, exc) } } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 09401bc6ef..c386eee45e 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -806,6 +806,30 @@ func (o *Options) enableAllExperimentMaps() { } } +// validateDirectoryFilter validates if the filter string +// - is not empty +// - start with either + or - +// - doesn't contain currently unsupported glob operators: *, ? +func validateDirectoryFilter(ifilter string) (string, error) { + filter := fmt.Sprint(ifilter) + if filter == "" || (filter[0] != '+' && filter[0] != '-') { + return "", fmt.Errorf("invalid filter %v, must start with + or -", filter) + } + segs := strings.Split(filter, "/") + unsupportedOps := [...]string{"?", "*"} + for _, seg := range segs { + if seg != "**" { + for _, op := range unsupportedOps { + if strings.Contains(seg, op) { + return "", fmt.Errorf("invalid filter %v, operator %v not supported. If you want to have this operator supported, consider filing an issue.", filter, op) + } + } + } + } + + return strings.TrimRight(filepath.FromSlash(filter), "/"), nil +} + func (o *Options) set(name string, value interface{}, seen map[string]struct{}) OptionResult { // Flatten the name in case we get options with a hierarchy. split := strings.Split(name, ".") @@ -850,9 +874,9 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) } var filters []string for _, ifilter := range ifilters { - filter := fmt.Sprint(ifilter) - if filter == "" || (filter[0] != '+' && filter[0] != '-') { - result.errorf("invalid filter %q, must start with + or -", filter) + filter, err := validateDirectoryFilter(fmt.Sprintf("%v", ifilter)) + if err != nil { + result.errorf(err.Error()) return result } filters = append(filters, strings.TrimRight(filepath.FromSlash(filter), "/")) diff --git a/internal/lsp/source/workspace_symbol.go b/internal/lsp/source/workspace_symbol.go index 6167c586a9..6c33effc1a 100644 --- a/internal/lsp/source/workspace_symbol.go +++ b/internal/lsp/source/workspace_symbol.go @@ -8,7 +8,9 @@ import ( "context" "fmt" "go/types" + "path" "path/filepath" + "regexp" "runtime" "sort" "strings" @@ -305,11 +307,12 @@ func collectSymbols(ctx context.Context, views []View, matcherType SymbolMatcher roots = append(roots, strings.TrimRight(string(v.Folder()), "/")) filters := v.Options().DirectoryFilters + filterer := NewFilterer(filters) folder := filepath.ToSlash(v.Folder().Filename()) for uri, syms := range snapshot.Symbols(ctx) { norm := filepath.ToSlash(uri.Filename()) nm := strings.TrimPrefix(norm, folder) - if FiltersDisallow(nm, filters) { + if filterer.Disallow(nm) { continue } // Only scan each file once. @@ -358,28 +361,70 @@ func collectSymbols(ctx context.Context, views []View, matcherType SymbolMatcher return unified.results(), nil } -// FilterDisallow is code from the body of cache.pathExcludedByFilter in cache/view.go -// Exporting and using that function would cause an import cycle. -// Moving it here and exporting it would leave behind view_test.go. -// (This code is exported and used in the body of cache.pathExcludedByFilter) -func FiltersDisallow(path string, filters []string) bool { +type Filterer struct { + // Whether a filter is excluded depends on the operator (first char of the raw filter). + // Slices filters and excluded then should have the same length. + filters []*regexp.Regexp + excluded []bool +} + +// NewFilterer computes regular expression form of all raw filters +func NewFilterer(rawFilters []string) *Filterer { + var f Filterer + for _, filter := range rawFilters { + filter = path.Clean(filepath.ToSlash(filter)) + op, prefix := filter[0], filter[1:] + // convertFilterToRegexp adds "/" at the end of prefix to handle cases where a filter is a prefix of another filter. + // For example, it prevents [+foobar, -foo] from excluding "foobar". + f.filters = append(f.filters, convertFilterToRegexp(filepath.ToSlash(prefix))) + f.excluded = append(f.excluded, op == '-') + } + + return &f +} + +// Disallow return true if the path is excluded from the filterer's filters. +func (f *Filterer) Disallow(path string) bool { path = strings.TrimPrefix(path, "/") var excluded bool - for _, filter := range filters { - op, prefix := filter[0], filter[1:] - // Non-empty prefixes have to be precise directory matches. - if prefix != "" { - prefix = prefix + "/" - path = path + "/" + + for i, filter := range f.filters { + path := path + if !strings.HasSuffix(path, "/") { + path += "/" } - if !strings.HasPrefix(path, prefix) { + if !filter.MatchString(path) { continue } - excluded = op == '-' + excluded = f.excluded[i] } + return excluded } +// convertFilterToRegexp replaces glob-like operator substrings in a string file path to their equivalent regex forms. +// Supporting glob-like operators: +// - **: match zero or more complete path segments +func convertFilterToRegexp(filter string) *regexp.Regexp { + var ret strings.Builder + segs := strings.Split(filter, "/") + for i, seg := range segs { + if seg == "**" { + switch i { + case 0: + ret.WriteString("^.*") + default: + ret.WriteString(".*") + } + } else { + ret.WriteString(regexp.QuoteMeta(seg)) + } + ret.WriteString("/") + } + + return regexp.MustCompile(ret.String()) +} + // symbolFile holds symbol information for a single file. type symbolFile struct { uri span.URI diff --git a/internal/lsp/source/workspace_symbol_test.go b/internal/lsp/source/workspace_symbol_test.go index 314ef785df..633550ed94 100644 --- a/internal/lsp/source/workspace_symbol_test.go +++ b/internal/lsp/source/workspace_symbol_test.go @@ -44,3 +44,88 @@ func TestParseQuery(t *testing.T) { } } } + +func TestFiltererDisallow(t *testing.T) { + tests := []struct { + filters []string + included []string + excluded []string + }{ + { + []string{"+**/c.go"}, + []string{"a/c.go", "a/b/c.go"}, + []string{}, + }, + { + []string{"+a/**/c.go"}, + []string{"a/b/c.go", "a/b/d/c.go", "a/c.go"}, + []string{}, + }, + { + []string{"-a/c.go", "+a/**"}, + []string{"a/c.go"}, + []string{}, + }, + { + []string{"+a/**/c.go", "-**/c.go"}, + []string{}, + []string{"a/b/c.go"}, + }, + { + []string{"+a/**/c.go", "-a/**"}, + []string{}, + []string{"a/b/c.go"}, + }, + { + []string{"+**/c.go", "-a/**/c.go"}, + []string{}, + []string{"a/b/c.go"}, + }, + { + []string{"+foobar", "-foo"}, + []string{"foobar", "foobar/a"}, + []string{"foo", "foo/a"}, + }, + { + []string{"+", "-"}, + []string{}, + []string{"foobar", "foobar/a", "foo", "foo/a"}, + }, + { + []string{"-", "+"}, + []string{"foobar", "foobar/a", "foo", "foo/a"}, + []string{}, + }, + { + []string{"-a/**/b/**/c.go"}, + []string{}, + []string{"a/x/y/z/b/f/g/h/c.go"}, + }, + // tests for unsupported glob operators + { + []string{"+**/c.go", "-a/*/c.go"}, + []string{"a/b/c.go"}, + []string{}, + }, + { + []string{"+**/c.go", "-a/?/c.go"}, + []string{"a/b/c.go"}, + []string{}, + }, + } + + for _, test := range tests { + filterer := NewFilterer(test.filters) + for _, inc := range test.included { + if filterer.Disallow(inc) { + t.Errorf("Filters %v excluded %v, wanted included", test.filters, inc) + } + } + + for _, exc := range test.excluded { + if !filterer.Disallow(exc) { + t.Errorf("Filters %v included %v, wanted excluded", test.filters, exc) + } + } + } +}