From ac612affd56b7105342f1d1dd0e56fd7fe773d3b Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Mon, 2 Nov 2020 17:18:20 -0500 Subject: [PATCH] internal/lsp: fix the logic to avoid duplicate file watching The logic that checked if a file was already being watched by the default glob pattern was incorrect. Fix it, and use the newly added InDir function. Change-Id: Ia7e3851ab5b9fa1fa7590cae3b370676201a9141 Reviewed-on: https://go-review.googlesource.com/c/tools/+/267119 Trust: Rebecca Stambler Run-TryBot: Rebecca Stambler gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/snapshot.go | 4 +- internal/lsp/cache/view.go | 76 +-------------------------------- internal/lsp/cache/workspace.go | 2 +- internal/lsp/general.go | 11 ++++- internal/lsp/source/util.go | 74 ++++++++++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 80 deletions(-) diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index d916de3c35..15b9855add 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -694,7 +694,7 @@ func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Pac func (s *snapshot) GoModForFile(ctx context.Context, uri span.URI) span.URI { var match span.URI for modURI := range s.workspace.activeModFiles() { - if !inDir(dirURI(modURI).Filename(), uri.Filename()) { + if !source.InDir(dirURI(modURI).Filename(), uri.Filename()) { continue } if len(modURI) > len(match) { @@ -1377,7 +1377,7 @@ func (s *snapshot) shouldInvalidateMetadata(ctx context.Context, newSnapshot *sn } // If a go.mod in the workspace has been changed, invalidate metadata. if kind := originalFH.Kind(); kind == source.Mod { - return inDir(filepath.Dir(s.view.rootURI.Filename()), filepath.Dir(originalFH.URI().Filename())) + return source.InDir(filepath.Dir(s.view.rootURI.Filename()), filepath.Dir(originalFH.URI().Filename())) } // Get the original and current parsed files in order to check package name // and imports. Use the new snapshot to parse to avoid modifying the diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index f27d8ca1af..598908a1cc 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -698,87 +698,13 @@ func validBuildConfiguration(folder span.URI, ws *workspaceInformation, modFiles // The user may have a multiple directories in their GOPATH. // Check if the workspace is within any of them. for _, gp := range filepath.SplitList(ws.gopath) { - if inDir(filepath.Join(gp, "src"), folder.Filename()) { + if source.InDir(filepath.Join(gp, "src"), folder.Filename()) { return true } } return false } -// Copied and slightly adjusted from go/src/cmd/go/internal/search/search.go. -// -// inDir checks whether path is in the file tree rooted at dir. -// If so, InDir returns an equivalent path relative to dir. -// If not, InDir returns an empty string. -// InDir makes some effort to succeed even in the presence of symbolic links. -func inDir(dir, path string) bool { - if rel := inDirLex(path, dir); rel != "" { - return true - } - xpath, err := filepath.EvalSymlinks(path) - if err != nil || xpath == path { - xpath = "" - } else { - if rel := inDirLex(xpath, dir); rel != "" { - return true - } - } - - xdir, err := filepath.EvalSymlinks(dir) - if err == nil && xdir != dir { - if rel := inDirLex(path, xdir); rel != "" { - return true - } - if xpath != "" { - if rel := inDirLex(xpath, xdir); rel != "" { - return true - } - } - } - return false -} - -// Copied from go/src/cmd/go/internal/search/search.go. -// -// inDirLex is like inDir but only checks the lexical form of the file names. -// It does not consider symbolic links. -// TODO(rsc): This is a copy of str.HasFilePathPrefix, modified to -// return the suffix. Most uses of str.HasFilePathPrefix should probably -// be calling InDir instead. -func inDirLex(path, dir string) string { - pv := strings.ToUpper(filepath.VolumeName(path)) - dv := strings.ToUpper(filepath.VolumeName(dir)) - path = path[len(pv):] - dir = dir[len(dv):] - switch { - default: - return "" - case pv != dv: - return "" - case len(path) == len(dir): - if path == dir { - return "." - } - return "" - case dir == "": - return path - case len(path) > len(dir): - if dir[len(dir)-1] == filepath.Separator { - if path[:len(dir)] == dir { - return path[len(dir):] - } - return "" - } - if path[len(dir)] == filepath.Separator && path[:len(dir)] == dir { - if len(path) == len(dir)+1 { - return "." - } - return path[len(dir)+1:] - } - return "" - } -} - // getGoEnv gets the view's various GO* values. func (s *Session) getGoEnv(ctx context.Context, folder string, configEnv []string) (environmentVariables, map[string]string, error) { envVars := environmentVariables{} diff --git a/internal/lsp/cache/workspace.go b/internal/lsp/cache/workspace.go index 673843a57a..4d05adc5eb 100644 --- a/internal/lsp/cache/workspace.go +++ b/internal/lsp/cache/workspace.go @@ -258,7 +258,7 @@ func (wm *workspace) invalidate(ctx context.Context, changes map[span.URI]*fileC // Legacy mode only considers a module a workspace root. continue } - if !inDir(wm.root.Filename(), uri.Filename()) { + if !source.InDir(wm.root.Filename(), uri.Filename()) { // Otherwise, the module must be contained within the workspace root. continue } diff --git a/internal/lsp/general.go b/internal/lsp/general.go index 0dd0ad274a..1c2a8bd826 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -348,13 +348,20 @@ func (s *Server) registerWatchedDirectoriesLocked(ctx context.Context, dirs map[ }} for dir := range dirs { filename := dir.Filename() + // If the directory is within a workspace folder, we're already // watching it via the relative path above. + var matched bool for _, view := range s.session.Views() { - if isSubdirectory(view.Folder().Filename(), filename) { - continue + if source.InDir(view.Folder().Filename(), filename) { + matched = true + break } } + if matched { + continue + } + // If microsoft/vscode#100870 is resolved before // microsoft/vscode#104387, we will need a work-around for Windows // drive letter casing. diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 4e01099672..b3d87f9311 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -441,3 +441,77 @@ func isDirective(c string) bool { } return true } + +// InDir checks whether path is in the file tree rooted at dir. +// If so, InDir returns an equivalent path relative to dir. +// If not, InDir returns an empty string. +// InDir makes some effort to succeed even in the presence of symbolic links. +// +// Copied and slightly adjusted from go/src/cmd/go/internal/search/search.go. +func InDir(dir, path string) bool { + if rel := inDirLex(path, dir); rel != "" { + return true + } + xpath, err := filepath.EvalSymlinks(path) + if err != nil || xpath == path { + xpath = "" + } else { + if rel := inDirLex(xpath, dir); rel != "" { + return true + } + } + + xdir, err := filepath.EvalSymlinks(dir) + if err == nil && xdir != dir { + if rel := inDirLex(path, xdir); rel != "" { + return true + } + if xpath != "" { + if rel := inDirLex(xpath, xdir); rel != "" { + return true + } + } + } + return false +} + +// Copied from go/src/cmd/go/internal/search/search.go. +// +// inDirLex is like inDir but only checks the lexical form of the file names. +// It does not consider symbolic links. +// TODO(rsc): This is a copy of str.HasFilePathPrefix, modified to +// return the suffix. Most uses of str.HasFilePathPrefix should probably +// be calling InDir instead. +func inDirLex(path, dir string) string { + pv := strings.ToUpper(filepath.VolumeName(path)) + dv := strings.ToUpper(filepath.VolumeName(dir)) + path = path[len(pv):] + dir = dir[len(dv):] + switch { + default: + return "" + case pv != dv: + return "" + case len(path) == len(dir): + if path == dir { + return "." + } + return "" + case dir == "": + return path + case len(path) > len(dir): + if dir[len(dir)-1] == filepath.Separator { + if path[:len(dir)] == dir { + return path[len(dir):] + } + return "" + } + if path[len(dir)] == filepath.Separator && path[:len(dir)] == dir { + if len(path) == len(dir)+1 { + return "." + } + return path[len(dir)+1:] + } + return "" + } +}