Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions tsc/internal/project/bulkcache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,43 @@ func TestBulkCacheInvalidation(t *testing.T) {
})
})

t.Run("large all-created batch reloads cached file", func(t *testing.T) {
t.Parallel()
for _, test := range []struct {
name string
eventCount int
eventURI lsproto.DocumentUri
}{
{name: "single parent directory", eventURI: "file:///project/src"},
{name: "excessive exact file", eventCount: 1001, eventURI: "file:///project/src/helper.ts"},
{name: "excessive parent directory", eventCount: 1001, eventURI: "file:///project/src"},
} {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
session, utils := projecttestutil.Setup(baseFiles)

session.DidOpenFile(context.Background(), "file:///project/src/index.ts", 1, baseFiles["/project/src/index.ts"].(string), lsproto.LanguageKindTypeScript)
ls, err := session.GetLanguageService(context.Background(), "file:///project/src/index.ts")
assert.NilError(t, err)
assert.Equal(t, ls.GetProgram().GetSourceFile("/project/src/helper.ts").Text(), `export const helper = "test";`)

err = utils.FS().WriteFile("/project/src/helper.ts", `export const helper = "updated";`)
assert.NilError(t, err)

fileEvents := generateFileEvents(test.eventCount, "file:///project/generated/file%d.ts", lsproto.FileChangeTypeCreated)
fileEvents = append(fileEvents, &lsproto.FileEvent{
Uri: test.eventURI,
Type: lsproto.FileChangeTypeCreated,
})
session.DidChangeWatchedFiles(context.Background(), fileEvents)

ls, err = session.GetLanguageService(context.Background(), "file:///project/src/index.ts")
assert.NilError(t, err)
assert.Equal(t, ls.GetProgram().GetSourceFile("/project/src/helper.ts").Text(), `export const helper = "updated";`)
})
}
})

t.Run("large number of changes outside node_modules", func(t *testing.T) {
t.Parallel()
test := func(t *testing.T, fileEvents []*lsproto.FileEvent, expectConfigReload bool) {
Expand Down
40 changes: 17 additions & 23 deletions tsc/internal/project/overlayfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,13 +417,14 @@ func (fs *overlayFS) processChanges(changes []FileChange) (FileChangeSummary, ma

// Reduced collection of changes that occurred on a single file
type fileEvents struct {
openChange *FileChange
closeChange *FileChange
watchChanged bool
changes []*FileChange
saved bool
created bool
deleted bool
openChange *FileChange
closeChange *FileChange
watchChanged bool
changes []*FileChange
saved bool
created bool
deleted bool
watchObserved bool
}

fileEventMap := make(map[lsproto.DocumentUri]*fileEvents)
Expand All @@ -439,6 +440,7 @@ func (fs *overlayFS) processChanges(changes []FileChange) (FileChangeSummary, ma
events = &fileEvents{}
fileEventMap[uri] = events
}
events.watchObserved = events.watchObserved || change.Kind.IsWatchKind()

if !result.IncludesWatchChangeOutsideNodeModules && change.Kind.IsWatchKind() && !strings.Contains(string(uri), "/node_modules/") {
result.IncludesWatchChangeOutsideNodeModules = true
Expand Down Expand Up @@ -529,16 +531,8 @@ func (fs *overlayFS) processChanges(changes []FileChange) (FileChangeSummary, ma
o = nil
}

if events.watchChanged {
if o == nil {
result.Changed.Add(uri)
} else if o != nil && !events.saved {
if matchesDiskText, _ := o.computeMatchesDiskText(fs.host); matchesDiskText != o.MatchesDiskText() {
o = newOverlay(o.FileName(), o.Content(), o.Version(), o.kind)
o.matchesDiskText = matchesDiskText
newOverlays[path] = o
}
}
if events.watchChanged && o == nil {
result.Changed.Add(uri)
}

if len(events.changes) > 0 && o != nil {
Expand Down Expand Up @@ -567,15 +561,15 @@ func (fs *overlayFS) processChanges(changes []FileChange) (FileChangeSummary, ma
}
}

if events.saved {
if o != nil {
if o != nil && (events.watchObserved || events.saved) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: with this change, every didSave reads the file from disk while holding fs.mu. Before, an open file was simply assumed to match the disk after a save. Comparing against the disk is more accurate, so this is probably fine, but I wanted to point out the extra read on every save.

if matchesDiskText, _ := o.computeMatchesDiskText(fs.host); matchesDiskText != o.MatchesDiskText() {
o = newOverlay(o.FileName(), o.Content(), o.Version(), o.kind)
o.matchesDiskText = true
o.matchesDiskText = matchesDiskText
newOverlays[path] = o
} else if !events.watchChanged {
// File was saved but never opened via didOpen; treat as a disk change.
result.Changed.Add(uri)
}
} else if events.saved && o == nil && !events.watchChanged {
// File was saved but never opened via didOpen; treat as a disk change.
result.Changed.Add(uri)
}

if events.created && o == nil {
Expand Down
49 changes: 49 additions & 0 deletions tsc/internal/project/overlayfs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ func TestProcessChanges(t *testing.T) {
},
})
// Then save
assert.NilError(t, fs.host.WriteFile(testURI1.FileName(), "const x = 1;"))
result, _ := fs.processChanges([]FileChange{
{
Kind: FileChangeKindSave,
Expand Down Expand Up @@ -228,6 +229,7 @@ func TestProcessChanges(t *testing.T) {
assert.Assert(t, !fs.GetFile(testURI1.FileName()).MatchesDiskText())

// Then save
assert.NilError(t, fs.host.WriteFile(testURI1.FileName(), "const x = 1;"))
fs.processChanges([]FileChange{
{
Kind: FileChangeKindSave,
Expand All @@ -237,6 +239,7 @@ func TestProcessChanges(t *testing.T) {
assert.Assert(t, fs.GetFile(testURI1.FileName()).MatchesDiskText())

// Now process a watch change
assert.NilError(t, fs.host.WriteFile(testURI1.FileName(), "const x = 2;"))
fs.processChanges([]FileChange{
{
Kind: FileChangeKindWatchChange,
Expand All @@ -246,6 +249,52 @@ func TestProcessChanges(t *testing.T) {
assert.Assert(t, !fs.GetFile(testURI1.FileName()).MatchesDiskText())
})

t.Run("save and watch create compare overlay with disk in either order", func(t *testing.T) {
t.Parallel()
for _, test := range []struct {
name string
changes []FileChange
}{
{
name: "save then create",
changes: []FileChange{
{Kind: FileChangeKindSave, URI: testURI1},
{Kind: FileChangeKindWatchCreate, URI: testURI1},
},
},
{
name: "create then save",
changes: []FileChange{
{Kind: FileChangeKindWatchCreate, URI: testURI1},
{Kind: FileChangeKindSave, URI: testURI1},
},
},
} {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
fs := createOverlayFS()
const overlayContent = "const value = 'overlay';"
fs.processChanges([]FileChange{
{
Kind: FileChangeKindOpen,
URI: testURI1,
Version: 1,
Content: overlayContent,
LanguageKind: lsproto.LanguageKindTypeScript,
},
})
assert.NilError(t, fs.host.WriteFile(testURI1.FileName(), "const value = 'disk';"))

fs.processChanges(test.changes)

file := fs.GetFile(testURI1.FileName())
assert.Assert(t, file != nil)
assert.Equal(t, file.Content(), overlayContent)
assert.Assert(t, !file.MatchesDiskText())
})
}
})

t.Run("save without overlay should not panic", func(t *testing.T) {
t.Parallel()
fs := createOverlayFS()
Expand Down
103 changes: 103 additions & 0 deletions tsc/internal/project/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,41 @@ func TestSession(t *testing.T) {
snapshot = session.Snapshot()
assert.Equal(t, snapshot.GetFile("/home/projects/TS/p1/src/index.ts").MatchesDiskText(), true)
})

t.Run("created watch event compares overlay with disk in either order", func(t *testing.T) {
t.Parallel()
for _, saveFirst := range []bool{true, false} {
name := "watch event first"
if saveFirst {
name = "save event first"
}
t.Run(name, func(t *testing.T) {
t.Parallel()
session, utils := projecttestutil.Setup(defaultFiles)
const uri = lsproto.DocumentUri("file:///home/projects/TS/p1/src/index.ts")
overlayContent := defaultFiles["/home/projects/TS/p1/src/index.ts"].(string)
session.DidOpenFile(context.Background(), uri, 1, overlayContent, lsproto.LanguageKindTypeScript)
assert.NilError(t, utils.FS().WriteFile(uri.FileName(), "const disk = true;"))

watch := func() {
session.DidChangeWatchedFiles(context.Background(), []*lsproto.FileEvent{{Type: lsproto.FileChangeTypeCreated, Uri: uri}})
}
if saveFirst {
session.DidSaveFile(context.Background(), uri)
watch()
} else {
watch()
session.DidSaveFile(context.Background(), uri)
}

session.WaitForBackgroundTasks()
session.DidOpenFile(context.Background(), "file:///home/projects/TS/p1/src/x.ts", 1, defaultFiles["/home/projects/TS/p1/src/x.ts"].(string), lsproto.LanguageKindTypeScript)
file := session.Snapshot().GetFile(uri.FileName())
assert.Equal(t, file.Content(), overlayContent)
assert.Assert(t, !file.MatchesDiskText())
})
}
})
})

t.Run("Source file sharing", func(t *testing.T) {
Expand Down Expand Up @@ -661,6 +696,74 @@ func TestSession(t *testing.T) {
assert.Check(t, lsAfter.GetProgram() != programBefore)
})

t.Run("create closed program file reloads content (atomic save)", func(t *testing.T) {
t.Parallel()
files := maps.Clone(defaultFiles)
session, utils := projecttestutil.Setup(files)

session.DidOpenFile(context.Background(), "file:///home/projects/TS/p1/src/index.ts", 1, files["/home/projects/TS/p1/src/index.ts"].(string), lsproto.LanguageKindTypeScript)

lsBefore, err := session.GetLanguageService(context.Background(), "file:///home/projects/TS/p1/src/index.ts")
assert.NilError(t, err)
programBefore := lsBefore.GetProgram()

err = utils.FS().WriteFile("/home/projects/TS/p1/src/x.ts", `export const x = 2;`)
assert.NilError(t, err)

// Simulate atomic save / rename where watcher reports Created (type 1)
session.DidChangeWatchedFiles(context.Background(), []*lsproto.FileEvent{
{
Type: lsproto.FileChangeTypeCreated,
Uri: "file:///home/projects/TS/p1/src/x.ts",
},
})

lsAfter, err := session.GetLanguageService(context.Background(), "file:///home/projects/TS/p1/src/index.ts")
assert.NilError(t, err)
assert.Check(t, lsAfter.GetProgram() != programBefore)
srcX := lsAfter.GetProgram().GetSourceFile("/home/projects/TS/p1/src/x.ts")
assert.Assert(t, srcX != nil)
assert.Equal(t, srcX.Text(), "export const x = 2;")
})

t.Run("atomic save sequence with temp file reloads content", func(t *testing.T) {
t.Parallel()
files := maps.Clone(defaultFiles)
session, utils := projecttestutil.Setup(files)

session.DidOpenFile(context.Background(), "file:///home/projects/TS/p1/src/index.ts", 1, files["/home/projects/TS/p1/src/index.ts"].(string), lsproto.LanguageKindTypeScript)

lsBefore, err := session.GetLanguageService(context.Background(), "file:///home/projects/TS/p1/src/index.ts")
assert.NilError(t, err)
programBefore := lsBefore.GetProgram()

err = utils.FS().WriteFile("/home/projects/TS/p1/src/x.ts", `export const x = 42;`)
assert.NilError(t, err)

// Sequence sent by editors like VS Code on atomic save: tmp create -> target create -> tmp delete
session.DidChangeWatchedFiles(context.Background(), []*lsproto.FileEvent{
{
Type: lsproto.FileChangeTypeCreated,
Uri: "file:///home/projects/TS/p1/src/x.ts.tmp",
},
{
Type: lsproto.FileChangeTypeCreated,
Uri: "file:///home/projects/TS/p1/src/x.ts",
},
{
Type: lsproto.FileChangeTypeDeleted,
Uri: "file:///home/projects/TS/p1/src/x.ts.tmp",
},
})

lsAfter, err := session.GetLanguageService(context.Background(), "file:///home/projects/TS/p1/src/index.ts")
assert.NilError(t, err)
assert.Check(t, lsAfter.GetProgram() != programBefore)
srcX := lsAfter.GetProgram().GetSourceFile("/home/projects/TS/p1/src/x.ts")
assert.Assert(t, srcX != nil)
assert.Equal(t, srcX.Text(), "export const x = 42;")
})

t.Run("change program file not in tsconfig root files", func(t *testing.T) {
t.Parallel()
for _, workspaceDir := range []string{"/", "/home/projects/TS/p1", "/somewhere/else/entirely"} {
Expand Down
4 changes: 3 additions & 1 deletion tsc/internal/project/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ func (s *Snapshot) processFileChanges(
}
previousOpenFiles := overlayFileHandles(previousOverlays)
openFiles := overlayFileHandles(overlays)
fileChanges = s.fs.expandRealpathAliases(fileChanges, fs.fs)
Comment thread
johnfav03 marked this conversation as resolved.
fileChanges = fs.expandCreatedCacheEntries(fileChanges)
Comment on lines +147 to +148

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two expansions now run before the HasExcessiveWatchEvents() check, and both call DirectoryExists on every created URI. A large npm install or branch switch can send tens of thousands of Created events, so we'd make tens of thousands of disk calls before we decide to take the cheap path for large batches. That could be noticeable on Windows.

I don't think the disk check is needed (see the comments on the two DirectoryExists calls). Without it, this stays a set of in-memory lookups, as it was before.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the expansion happens before the threshold check, one Created event for a directory that has many cached files under it can push the batch over excessiveChangeThreshold. If that directory is outside node_modules, the whole file cache gets cleared. Is that intended? If it is, a test showing it would help.

if fileChanges.HasExcessiveWatchEvents() {
invalidateStart := time.Now()
if fileChanges.InvalidateAll {
Expand Down Expand Up @@ -178,7 +180,7 @@ func (s *Snapshot) processFileChanges(
}
_, contentMapperWatchedFiles := s.contentMapperWatchState()
fileChanges = fs.expandAndFilterWatchEvents(fileChanges, contentMapperExtensions, contentMapperWatchedFiles, previousOpenFiles, openFiles)
fileChanges = s.fs.expandRealpathAliases(fileChanges)
fileChanges = s.fs.expandRealpathAliases(fileChanges, fs.fs)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the non-excessive branch, expandRealpathAliases now runs twice. I see the second call is still needed for directory deletes expanded by expandAndFilterWatchEvents. Could the first call move into the excessive branch, or could the second call only expand Deleted?

fileChanges = fs.markDirtyFiles(fileChanges)
fileChanges = fs.convertOpenAndCloseToChanges(fileChanges, previousOpenFiles, openFiles)
}
Expand Down
Loading
Loading