gopls/internal/lsp: avoid I/O in URI comparison operations
Previously, URI comparison would look at both the text
and then the inodes of the files, calling stat(2).
This was a muddling of two concerns, and caused I/O
operations in surprsing places, often when locks are
held.
Most callers need only a stable sort order for determinism;
they can use a simple textual comparison. Of the remainder:
- workspace.Clone uses a forked copy of the original,
out of unrationalized fear.
- Session.NewView uses span.SameExistingFile, the pure I/O-based check.
- ColumnMapper.Range uses a weakened syntactic check;
it just just an assertion (bug.Report).
Also, commit to honorSymlink=false and simplify the code
so that InDir and InDirLex become identical.
Closes golang/go#42833 (won't fix)
Change-Id: I984656a96144991908089eec101fd0df3c0e2477
Reviewed-on: https://go-review.googlesource.com/c/tools/+/454355
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go
index 5144111..c4d289a 100644
--- a/gopls/internal/lsp/cache/session.go
+++ b/gopls/internal/lsp/cache/session.go
@@ -167,7 +167,7 @@
s.viewMu.Lock()
defer s.viewMu.Unlock()
for _, view := range s.views {
- if span.CompareURI(view.folder, folder) == 0 {
+ if span.SameExistingFile(view.folder, folder) {
return nil, nil, nil, source.ErrViewExists
}
}
diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go
index 4adcfb1..54b4122 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -486,7 +486,7 @@
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()) {
+ if source.InDir(v.folder.Filename(), uri.Filename()) {
return pathExcludedByFilter(strings.TrimPrefix(uri.Filename(), v.folder.Filename()), filterer)
}
return false
diff --git a/gopls/internal/lsp/cache/workspace.go b/gopls/internal/lsp/cache/workspace.go
index b280ef3..a97894a 100644
--- a/gopls/internal/lsp/cache/workspace.go
+++ b/gopls/internal/lsp/cache/workspace.go
@@ -309,9 +309,7 @@
for d := range w.wsDirs {
dirs = append(dirs, d)
}
- sort.Slice(dirs, func(i, j int) bool {
- return source.CompareURI(dirs[i], dirs[j]) < 0
- })
+ sort.Slice(dirs, func(i, j int) bool { return dirs[i] < dirs[j] })
return dirs
}
@@ -349,6 +347,13 @@
result.activeModFiles[k] = v
}
+ equalURI := func(a, b span.URI) (r bool) {
+ // This query is a strange mix of syntax and file system state:
+ // deletion of a file causes a false result if the name doesn't change.
+ // Our tests exercise only the first clause.
+ return a == b || span.SameExistingFile(a, b)
+ }
+
// First handle changes to the go.work or gopls.mod file. This must be
// considered before any changes to go.mod or go.sum files, as these files
// determine which modules we care about. If go.work/gopls.mod has changed
@@ -362,7 +367,7 @@
continue
}
changed = true
- active := result.moduleSource != legacyWorkspace || source.CompareURI(modURI(w.root), uri) == 0
+ active := result.moduleSource != legacyWorkspace || equalURI(modURI(w.root), uri)
needReinit = needReinit || (active && change.fileHandle.Saved())
// Don't mess with the list of mod files if using go.work or gopls.mod.
if result.moduleSource == goplsModWorkspace || result.moduleSource == goWorkWorkspace {
diff --git a/gopls/internal/lsp/cmd/highlight.go b/gopls/internal/lsp/cmd/highlight.go
index 0737e9c..d8b3d22 100644
--- a/gopls/internal/lsp/cmd/highlight.go
+++ b/gopls/internal/lsp/cmd/highlight.go
@@ -8,7 +8,6 @@
"context"
"flag"
"fmt"
- "sort"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/span"
@@ -78,9 +77,7 @@
results = append(results, s)
}
// Sort results to make tests deterministic since DocumentHighlight uses a map.
- sort.SliceStable(results, func(i, j int) bool {
- return span.Compare(results[i], results[j]) == -1
- })
+ span.SortSpans(results)
for _, s := range results {
fmt.Println(s)
diff --git a/gopls/internal/lsp/fake/editor.go b/gopls/internal/lsp/fake/editor.go
index f73301d..df2f975 100644
--- a/gopls/internal/lsp/fake/editor.go
+++ b/gopls/internal/lsp/fake/editor.go
@@ -1274,7 +1274,7 @@
for path := range e.buffers {
abs := e.sandbox.Workdir.AbsPath(path)
- if oldAbs == abs || source.InDirLex(oldAbs, abs) {
+ if oldAbs == abs || source.InDir(oldAbs, abs) {
rel, err := filepath.Rel(oldAbs, abs)
if err != nil {
return nil, nil, fmt.Errorf("filepath.Rel(%q, %q): %v", oldAbs, abs, err)
diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go
index e9da7e3..23941be 100644
--- a/gopls/internal/lsp/lsp_test.go
+++ b/gopls/internal/lsp/lsp_test.go
@@ -783,13 +783,9 @@
}
results = append(results, imp)
}
- // Sort results and expected to make tests deterministic.
- sort.SliceStable(results, func(i, j int) bool {
- return span.Compare(results[i], results[j]) == -1
- })
- sort.SliceStable(impls, func(i, j int) bool {
- return span.Compare(impls[i], impls[j]) == -1
- })
+ span.SortSpans(results) // to make tests
+ span.SortSpans(impls) // deterministic
+
for i := range results {
if results[i] != impls[i] {
t.Errorf("for %dth implementation of %v got %v want %v", i, spn, results[i], impls[i])
@@ -830,9 +826,7 @@
results = append(results, h)
}
// Sort results to make tests deterministic since DocumentHighlight uses a map.
- sort.SliceStable(results, func(i, j int) bool {
- return span.Compare(results[i], results[j]) == -1
- })
+ span.SortSpans(results)
// Check to make sure all the expected highlights are found.
for i := range results {
if results[i] != locations[i] {
diff --git a/gopls/internal/lsp/protocol/span.go b/gopls/internal/lsp/protocol/span.go
index 2d87c08..86b14c2 100644
--- a/gopls/internal/lsp/protocol/span.go
+++ b/gopls/internal/lsp/protocol/span.go
@@ -38,10 +38,13 @@
"bytes"
"fmt"
"go/token"
+ "path/filepath"
+ "strings"
"unicode/utf8"
"golang.org/x/tools/gopls/internal/lsp/safetoken"
"golang.org/x/tools/gopls/internal/span"
+ "golang.org/x/tools/internal/bug"
)
// A ColumnMapper maps between UTF-8 oriented positions (e.g. token.Pos,
@@ -93,9 +96,16 @@
}
func (m *ColumnMapper) Range(s span.Span) (Range, error) {
- if span.CompareURI(m.URI, s.URI()) != 0 {
- return Range{}, fmt.Errorf("column mapper is for file %q instead of %q", m.URI, s.URI())
+ // Assert that we aren't using the wrong mapper.
+ // We check only the base name, and case insensitively,
+ // because we can't assume clean paths, no symbolic links,
+ // case-sensitive directories. The authoritative answer
+ // requires querying the file system, and we don't want
+ // to do that.
+ if !strings.EqualFold(filepath.Base(string(m.URI)), filepath.Base(string(s.URI()))) {
+ return Range{}, bug.Errorf("column mapper is for file %q instead of %q", m.URI, s.URI())
}
+
s, err := s.WithOffset(m.TokFile)
if err != nil {
return Range{}, err
diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go
index f147108..bf3784b 100644
--- a/gopls/internal/lsp/source/references.go
+++ b/gopls/internal/lsp/source/references.go
@@ -9,6 +9,7 @@
"errors"
"fmt"
"go/ast"
+ "strings"
"go/token"
"go/types"
@@ -122,11 +123,11 @@
toSort = refs[1:]
}
sort.Slice(toSort, func(i, j int) bool {
- x := CompareURI(toSort[i].URI(), toSort[j].URI())
- if x == 0 {
- return toSort[i].ident.Pos() < toSort[j].ident.Pos()
+ x, y := toSort[i], toSort[j]
+ if cmp := strings.Compare(string(x.URI()), string(y.URI())); cmp != 0 {
+ return cmp < 0
}
- return x < 0
+ return x.ident.Pos() < y.ident.Pos()
})
return refs, nil
}
diff --git a/gopls/internal/lsp/source/source_test.go b/gopls/internal/lsp/source/source_test.go
index 3ca62c3..98bac6d 100644
--- a/gopls/internal/lsp/source/source_test.go
+++ b/gopls/internal/lsp/source/source_test.go
@@ -614,13 +614,8 @@
}
results = append(results, imp)
}
- // Sort results and expected to make tests deterministic.
- sort.SliceStable(results, func(i, j int) bool {
- return span.Compare(results[i], results[j]) == -1
- })
- sort.SliceStable(impls, func(i, j int) bool {
- return span.Compare(impls[i], impls[j]) == -1
- })
+ span.SortSpans(results) // to make tests
+ span.SortSpans(impls) // deterministic
for i := range results {
if results[i] != impls[i] {
t.Errorf("for %dth implementation of %v got %v want %v", i, spn, results[i], impls[i])
@@ -655,9 +650,7 @@
results = append(results, h)
}
// Sort results to make tests deterministic since DocumentHighlight uses a map.
- sort.SliceStable(results, func(i, j int) bool {
- return span.Compare(results[i], results[j]) == -1
- })
+ span.SortSpans(results)
// Check to make sure all the expected highlights are found.
for i := range results {
if results[i] != locations[i] {
diff --git a/gopls/internal/lsp/source/util.go b/gopls/internal/lsp/source/util.go
index 55267c9..32c690f 100644
--- a/gopls/internal/lsp/source/util.go
+++ b/gopls/internal/lsp/source/util.go
@@ -445,62 +445,12 @@
return true
}
-// honorSymlinks toggles whether or not we consider symlinks when comparing
-// file or directory URIs.
-const honorSymlinks = false
-
-func CompareURI(left, right span.URI) int {
- if honorSymlinks {
- return span.CompareURI(left, right)
- }
- if left == right {
- return 0
- }
- if left < right {
- return -1
- }
- return 1
-}
-
// InDir checks whether path is in the file tree rooted at dir.
-// 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 InDirLex(dir, path) {
- return true
- }
- if !honorSymlinks {
- return false
- }
- xpath, err := filepath.EvalSymlinks(path)
- if err != nil || xpath == path {
- xpath = ""
- } else {
- if InDirLex(dir, xpath) {
- return true
- }
- }
-
- xdir, err := filepath.EvalSymlinks(dir)
- if err == nil && xdir != dir {
- if InDirLex(xdir, path) {
- return true
- }
- if xpath != "" {
- if InDirLex(xdir, xpath) {
- return true
- }
- }
- }
- return false
-}
-
-// InDirLex is like inDir but only checks the lexical form of the file names.
+// It checks only the lexical form of the file names.
// It does not consider symbolic links.
//
// Copied from go/src/cmd/go/internal/search/search.go.
-func InDirLex(dir, path string) bool {
+func InDir(dir, path string) bool {
pv := strings.ToUpper(filepath.VolumeName(path))
dv := strings.ToUpper(filepath.VolumeName(dir))
path = path[len(pv):]
diff --git a/gopls/internal/span/parse.go b/gopls/internal/span/parse.go
index c4cec16..715d5fe 100644
--- a/gopls/internal/span/parse.go
+++ b/gopls/internal/span/parse.go
@@ -92,8 +92,9 @@
return suffix{"", "", -1}
}
remains := input
+
+ // Remove optional trailing decimal number.
num := -1
- // first see if we have a number at the end
last := strings.LastIndexFunc(remains, func(r rune) bool { return r < '0' || r > '9' })
if last >= 0 && last < len(remains)-1 {
number, err := strconv.ParseInt(remains[last+1:], 10, 64)
@@ -104,6 +105,7 @@
}
// now see if we have a trailing separator
r, w := utf8.DecodeLastRuneInString(remains)
+ // TODO(adonovan): this condition is clearly wrong. Should the third byte be '-'?
if r != ':' && r != '#' && r == '#' {
return suffix{input, "", -1}
}
diff --git a/gopls/internal/span/span.go b/gopls/internal/span/span.go
index 333048a..00c7b3f 100644
--- a/gopls/internal/span/span.go
+++ b/gopls/internal/span/span.go
@@ -11,6 +11,8 @@
"fmt"
"go/token"
"path"
+ "sort"
+ "strings"
)
// Span represents a source code range in standardized form.
@@ -54,12 +56,23 @@
return p
}
-func Compare(a, b Span) int {
- if r := CompareURI(a.URI(), b.URI()); r != 0 {
- return r
+// SortSpans sorts spans into a stable but unspecified order.
+func SortSpans(spans []Span) {
+ sort.SliceStable(spans, func(i, j int) bool {
+ return compare(spans[i], spans[j]) < 0
+ })
+}
+
+// compare implements a three-valued ordered comparison of Spans.
+func compare(a, b Span) int {
+ // This is a textual comparison. It does not peform path
+ // cleaning, case folding, resolution of symbolic links,
+ // testing for existence, or any I/O.
+ if cmp := strings.Compare(string(a.URI()), string(b.URI())); cmp != 0 {
+ return cmp
}
- if r := comparePoint(a.v.Start, b.v.Start); r != 0 {
- return r
+ if cmp := comparePoint(a.v.Start, b.v.Start); cmp != 0 {
+ return cmp
}
return comparePoint(a.v.End, b.v.End)
}
diff --git a/gopls/internal/span/uri.go b/gopls/internal/span/uri.go
index adcc54a..8f6b033 100644
--- a/gopls/internal/span/uri.go
+++ b/gopls/internal/span/uri.go
@@ -8,7 +8,6 @@
"fmt"
"net/url"
"os"
- "path"
"path/filepath"
"runtime"
"strings"
@@ -101,28 +100,9 @@
return URI(u.String())
}
-// CompareURI performs a three-valued comparison of two URIs.
-// Lexically unequal URIs may compare equal if they are "file:" URIs
-// that share the same base name (ignoring case) and denote the same
-// file device/inode, according to stat(2).
-func CompareURI(a, b URI) int {
- if equalURI(a, b) {
- return 0
- }
- if a < b {
- return -1
- }
- return 1
-}
-
-func equalURI(a, b URI) bool {
- if a == b {
- return true
- }
- // If we have the same URI basename, we may still have the same file URIs.
- if !strings.EqualFold(path.Base(string(a)), path.Base(string(b))) {
- return false
- }
+// SameExistingFile reports whether two spans denote the
+// same existing file by querying the file system.
+func SameExistingFile(a, b URI) bool {
fa, err := filename(a)
if err != nil {
return false
@@ -131,7 +111,6 @@
if err != nil {
return false
}
- // Stat the files to check if they are equal.
infoa, err := os.Stat(filepath.FromSlash(fa))
if err != nil {
return false
