internal/lsp: Update FilterDisallow to support matching directories at arbitrary depth.

In FilterDisallow, change filter to regex form to match with file paths. Add a unit regtest for FilterDisallow.

For golang/go#46438

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

Author: Dylan Le

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
 			}
 		}

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,24 +1074,24 @@ 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
 		}
 	}
 	return true
 }
 
 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)
 }

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)
 			}
 		}

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), "/"))

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

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)
+			}
+		}
+	}
+}