Skip to content

Commit ae242ec

Browse files
findleyrfindleyr
committed
gopls: fix windows file corruption
Fix the bug that gopls finds the wrong content when formatting an open URI whose spelling does not match the spelling on disk (i.e. because of case insensitivity). Remove the whole View.filesByBase mechanism: it is problematic as we can't generally know whether or not we want to associate two different spellings of the same file: for the purposes of finding packages we may want to treat Foo.go as foo.go, but we don't want to treat a symlink of foo.go in another directory the same. Instead, use robustio.FileID to de-duplicate content in the cache, and otherwise treat URIs as we receive them. This fixes the formatting corruption, but means that we don't find packages for the corresponding file (because go/packages.Load("file=foo.go") fails). A failing test is added for the latter bug. Also: use a seenFiles map in the view to satisfy the concern of tracking relevant files, with a TODO to delete this problematic map. Along the way, refactor somewhat to separate and normalize the implementations of source.FileSource. For golang/go#57081 Change-Id: I02971a1702f057b644fa18a873790e8f0d98a323 Reviewed-on: https://go-review.googlesource.com/c/tools/+/462819 gopls-CI: kokoro <[email protected]> Run-TryBot: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
1 parent 6f65213 commit ae242ec

File tree

12 files changed

+429
-325
lines changed

12 files changed

+429
-325
lines changed

‎gopls/internal/lsp/cache/cache.go

+13-109
Original file line numberDiff line numberDiff line change
@@ -11,20 +11,16 @@ import (
1111
"go/token"
1212
"go/types"
1313
"html/template"
14-
"os"
1514
"reflect"
1615
"sort"
1716
"strconv"
18-
"sync"
1917
"sync/atomic"
20-
"time"
2118

2219
"golang.org/x/tools/gopls/internal/lsp/source"
23-
"golang.org/x/tools/gopls/internal/span"
2420
"golang.org/x/tools/internal/event"
25-
"golang.org/x/tools/internal/event/tag"
2621
"golang.org/x/tools/internal/gocommand"
2722
"golang.org/x/tools/internal/memoize"
23+
"golang.org/x/tools/internal/robustio"
2824
)
2925

3026
// New Creates a new cache for gopls operation results, using the given file
@@ -47,124 +43,32 @@ func New(fset *token.FileSet, store *memoize.Store) *Cache {
4743
}
4844

4945
c := &Cache{
50-
id: strconv.FormatInt(index, 10),
51-
fset: fset,
52-
store: store,
53-
fileContent: map[span.URI]*DiskFile{},
46+
id: strconv.FormatInt(index, 10),
47+
fset: fset,
48+
store: store,
49+
memoizedFS: &memoizedFS{filesByID: map[robustio.FileID][]*DiskFile{}},
5450
}
5551
return c
5652
}
5753

54+
// A Cache holds caching stores that are bundled together for consistency.
55+
//
56+
// TODO(rfindley): once fset and store need not be bundled together, the Cache
57+
// type can be eliminated.
5858
type Cache struct {
5959
id string
6060
fset *token.FileSet
6161

6262
store *memoize.Store
6363

64-
fileMu sync.Mutex
65-
fileContent map[span.URI]*DiskFile
66-
}
67-
68-
// A DiskFile is a file on the filesystem, or a failure to read one.
69-
// It implements the source.FileHandle interface.
70-
type DiskFile struct {
71-
uri span.URI
72-
modTime time.Time
73-
content []byte
74-
hash source.Hash
75-
err error
76-
}
77-
78-
func (h *DiskFile) URI() span.URI { return h.uri }
79-
80-
func (h *DiskFile) FileIdentity() source.FileIdentity {
81-
return source.FileIdentity{
82-
URI: h.uri,
83-
Hash: h.hash,
84-
}
85-
}
86-
87-
func (h *DiskFile) Saved() bool { return true }
88-
func (h *DiskFile) Version() int32 { return 0 }
89-
90-
func (h *DiskFile) Read() ([]byte, error) {
91-
return h.content, h.err
92-
}
93-
94-
// GetFile stats and (maybe) reads the file, updates the cache, and returns it.
95-
func (c *Cache) GetFile(ctx context.Context, uri span.URI) (source.FileHandle, error) {
96-
fi, statErr := os.Stat(uri.Filename())
97-
if statErr != nil {
98-
return &DiskFile{
99-
err: statErr,
100-
uri: uri,
101-
}, nil
102-
}
103-
104-
// We check if the file has changed by comparing modification times. Notably,
105-
// this is an imperfect heuristic as various systems have low resolution
106-
// mtimes (as much as 1s on WSL or s390x builders), so we only cache
107-
// filehandles if mtime is old enough to be reliable, meaning that we don't
108-
// expect a subsequent write to have the same mtime.
109-
//
110-
// The coarsest mtime precision we've seen in practice is 1s, so consider
111-
// mtime to be unreliable if it is less than 2s old. Capture this before
112-
// doing anything else.
113-
recentlyModified := time.Since(fi.ModTime()) < 2*time.Second
114-
115-
c.fileMu.Lock()
116-
fh, ok := c.fileContent[uri]
117-
c.fileMu.Unlock()
118-
119-
if ok && fh.modTime.Equal(fi.ModTime()) {
120-
return fh, nil
121-
}
122-
123-
fh, err := readFile(ctx, uri, fi) // ~25us
124-
if err != nil {
125-
return nil, err
126-
}
127-
c.fileMu.Lock()
128-
if !recentlyModified {
129-
c.fileContent[uri] = fh
130-
} else {
131-
delete(c.fileContent, uri)
132-
}
133-
c.fileMu.Unlock()
134-
return fh, nil
135-
}
136-
137-
// ioLimit limits the number of parallel file reads per process.
138-
var ioLimit = make(chan struct{}, 128)
139-
140-
func readFile(ctx context.Context, uri span.URI, fi os.FileInfo) (*DiskFile, error) {
141-
select {
142-
case ioLimit <- struct{}{}:
143-
case <-ctx.Done():
144-
return nil, ctx.Err()
145-
}
146-
defer func() { <-ioLimit }()
147-
148-
ctx, done := event.Start(ctx, "cache.readFile", tag.File.Of(uri.Filename()))
149-
_ = ctx
150-
defer done()
151-
152-
content, err := os.ReadFile(uri.Filename()) // ~20us
153-
if err != nil {
154-
content = nil // just in case
155-
}
156-
return &DiskFile{
157-
modTime: fi.ModTime(),
158-
uri: uri,
159-
content: content,
160-
hash: source.HashOf(content),
161-
err: err,
162-
}, nil
64+
*memoizedFS // implements source.FileSource
16365
}
16466

16567
// NewSession creates a new gopls session with the given cache and options overrides.
16668
//
16769
// The provided optionsOverrides may be nil.
70+
//
71+
// TODO(rfindley): move this to session.go.
16872
func NewSession(ctx context.Context, c *Cache, optionsOverrides func(*source.Options)) *Session {
16973
index := atomic.AddInt64(&sessionIndex, 1)
17074
options := source.DefaultOptions().Clone()
@@ -176,7 +80,7 @@ func NewSession(ctx context.Context, c *Cache, optionsOverrides func(*source.Opt
17680
cache: c,
17781
gocmdRunner: &gocommand.Runner{},
17882
options: options,
179-
overlays: make(map[span.URI]*Overlay),
83+
overlayFS: newOverlayFS(c),
18084
}
18185
event.Log(ctx, "New session", KeyCreateSession.Of(s))
18286
return s

‎gopls/internal/lsp/cache/fs_memoized.go

+149
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
// Copyright 2023 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package cache
6+
7+
import (
8+
"context"
9+
"os"
10+
"sync"
11+
"time"
12+
13+
"golang.org/x/tools/gopls/internal/lsp/source"
14+
"golang.org/x/tools/gopls/internal/span"
15+
"golang.org/x/tools/internal/event"
16+
"golang.org/x/tools/internal/event/tag"
17+
"golang.org/x/tools/internal/robustio"
18+
)
19+
20+
// A memoizedFS is a file source that memoizes reads, to reduce IO.
21+
type memoizedFS struct {
22+
mu sync.Mutex
23+
24+
// filesByID maps existing file inodes to the result of a read.
25+
// (The read may have failed, e.g. due to EACCES or a delete between stat+read.)
26+
// Each slice is a non-empty list of aliases: different URIs.
27+
filesByID map[robustio.FileID][]*DiskFile
28+
}
29+
30+
func newMemoizedFS() *memoizedFS {
31+
return &memoizedFS{filesByID: make(map[robustio.FileID][]*DiskFile)}
32+
}
33+
34+
// A DiskFile is a file on the filesystem, or a failure to read one.
35+
// It implements the source.FileHandle interface.
36+
type DiskFile struct {
37+
uri span.URI
38+
modTime time.Time
39+
content []byte
40+
hash source.Hash
41+
err error
42+
}
43+
44+
func (h *DiskFile) URI() span.URI { return h.uri }
45+
46+
func (h *DiskFile) FileIdentity() source.FileIdentity {
47+
return source.FileIdentity{
48+
URI: h.uri,
49+
Hash: h.hash,
50+
}
51+
}
52+
53+
func (h *DiskFile) Saved() bool { return true }
54+
func (h *DiskFile) Version() int32 { return 0 }
55+
func (h *DiskFile) Read() ([]byte, error) { return h.content, h.err }
56+
57+
// GetFile stats and (maybe) reads the file, updates the cache, and returns it.
58+
func (fs *memoizedFS) GetFile(ctx context.Context, uri span.URI) (source.FileHandle, error) {
59+
id, mtime, err := robustio.GetFileID(uri.Filename())
60+
if err != nil {
61+
// file does not exist
62+
return &DiskFile{
63+
err: err,
64+
uri: uri,
65+
}, nil
66+
}
67+
68+
// We check if the file has changed by comparing modification times. Notably,
69+
// this is an imperfect heuristic as various systems have low resolution
70+
// mtimes (as much as 1s on WSL or s390x builders), so we only cache
71+
// filehandles if mtime is old enough to be reliable, meaning that we don't
72+
// expect a subsequent write to have the same mtime.
73+
//
74+
// The coarsest mtime precision we've seen in practice is 1s, so consider
75+
// mtime to be unreliable if it is less than 2s old. Capture this before
76+
// doing anything else.
77+
recentlyModified := time.Since(mtime) < 2*time.Second
78+
79+
fs.mu.Lock()
80+
fhs, ok := fs.filesByID[id]
81+
if ok && fhs[0].modTime.Equal(mtime) {
82+
var fh *DiskFile
83+
// We have already seen this file and it has not changed.
84+
for _, h := range fhs {
85+
if h.uri == uri {
86+
fh = h
87+
break
88+
}
89+
}
90+
// No file handle for this exact URI. Create an alias, but share content.
91+
if fh == nil {
92+
newFH := *fhs[0]
93+
newFH.uri = uri
94+
fh = &newFH
95+
fhs = append(fhs, fh)
96+
fs.filesByID[id] = fhs
97+
}
98+
fs.mu.Unlock()
99+
return fh, nil
100+
}
101+
fs.mu.Unlock()
102+
103+
// Unknown file, or file has changed. Read (or re-read) it.
104+
fh, err := readFile(ctx, uri, mtime) // ~25us
105+
if err != nil {
106+
return nil, err // e.g. cancelled (not: read failed)
107+
}
108+
109+
fs.mu.Lock()
110+
if !recentlyModified {
111+
fs.filesByID[id] = []*DiskFile{fh}
112+
} else {
113+
delete(fs.filesByID, id)
114+
}
115+
fs.mu.Unlock()
116+
return fh, nil
117+
}
118+
119+
// ioLimit limits the number of parallel file reads per process.
120+
var ioLimit = make(chan struct{}, 128)
121+
122+
func readFile(ctx context.Context, uri span.URI, mtime time.Time) (*DiskFile, error) {
123+
select {
124+
case ioLimit <- struct{}{}:
125+
case <-ctx.Done():
126+
return nil, ctx.Err()
127+
}
128+
defer func() { <-ioLimit }()
129+
130+
ctx, done := event.Start(ctx, "cache.readFile", tag.File.Of(uri.Filename()))
131+
_ = ctx
132+
defer done()
133+
134+
// It is possible that a race causes us to read a file with different file
135+
// ID, or whose mtime differs from the given mtime. However, in these cases
136+
// we expect the client to notify of a subsequent file change, and the file
137+
// content should be eventually consistent.
138+
content, err := os.ReadFile(uri.Filename()) // ~20us
139+
if err != nil {
140+
content = nil // just in case
141+
}
142+
return &DiskFile{
143+
modTime: mtime,
144+
uri: uri,
145+
content: content,
146+
hash: source.HashOf(content),
147+
err: err,
148+
}, nil
149+
}

‎gopls/internal/lsp/cache/fs_overlay.go

+76
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// Copyright 2023 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package cache
6+
7+
import (
8+
"context"
9+
"sync"
10+
11+
"golang.org/x/tools/gopls/internal/lsp/source"
12+
"golang.org/x/tools/gopls/internal/span"
13+
)
14+
15+
// An overlayFS is a source.FileSource that keeps track of overlays on top of a
16+
// delegate FileSource.
17+
type overlayFS struct {
18+
delegate source.FileSource
19+
20+
mu sync.Mutex
21+
overlays map[span.URI]*Overlay
22+
}
23+
24+
func newOverlayFS(delegate source.FileSource) *overlayFS {
25+
return &overlayFS{
26+
delegate: delegate,
27+
overlays: make(map[span.URI]*Overlay),
28+
}
29+
}
30+
31+
// Overlays returns a new unordered array of overlays.
32+
func (fs *overlayFS) Overlays() []*Overlay {
33+
overlays := make([]*Overlay, 0, len(fs.overlays))
34+
for _, overlay := range fs.overlays {
35+
overlays = append(overlays, overlay)
36+
}
37+
return overlays
38+
}
39+
40+
func (fs *overlayFS) GetFile(ctx context.Context, uri span.URI) (source.FileHandle, error) {
41+
fs.mu.Lock()
42+
overlay, ok := fs.overlays[uri]
43+
fs.mu.Unlock()
44+
if ok {
45+
return overlay, nil
46+
}
47+
return fs.delegate.GetFile(ctx, uri)
48+
}
49+
50+
// An Overlay is a file open in the editor. It may have unsaved edits.
51+
// It implements the source.FileHandle interface.
52+
type Overlay struct {
53+
uri span.URI
54+
content []byte
55+
hash source.Hash
56+
version int32
57+
kind source.FileKind
58+
59+
// saved is true if a file matches the state on disk,
60+
// and therefore does not need to be part of the overlay sent to go/packages.
61+
saved bool
62+
}
63+
64+
func (o *Overlay) URI() span.URI { return o.uri }
65+
66+
func (o *Overlay) FileIdentity() source.FileIdentity {
67+
return source.FileIdentity{
68+
URI: o.uri,
69+
Hash: o.hash,
70+
}
71+
}
72+
73+
func (o *Overlay) Read() ([]byte, error) { return o.content, nil }
74+
func (o *Overlay) Version() int32 { return o.version }
75+
func (o *Overlay) Saved() bool { return o.saved }
76+
func (o *Overlay) Kind() source.FileKind { return o.kind }

0 commit comments

Comments
 (0)