Repository navigation
Add createFileSystemFromSnapshot(oldSnapshot, extraFSLayer) - #64679
Wesley Wigham (weswigham) wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Missed invalidations can retain stale target contents or removed files in rebased snapshots.
3 open findings
What changed in this PR
Adds Snapshot.rebase to reuse synthetic filesystem changes over another snapshot without resending file contents.
Changes:
- Adds server, protocol, async, sync, and generator support.
- Implements filesystem composition and change notifications.
- Tests rebasing, invalidation, disposal, and auto-import behavior.
| File | Description |
|---|---|
| tsc/internal/api/session.go | Handles rebase requests. |
| tsc/internal/api/session_requestfilesystem_test.go | Tests self-rebase incremental state. |
| tsc/internal/api/session_rebase_test.go | Tests composition and snapshot lifetimes. |
| tsc/internal/api/session_rebase_changes_test.go | Tests invalidation and update parity. |
| tsc/internal/api/session_completion_test.go | Tests rebase auto-import preparation. |
| tsc/internal/api/requestfilesystem/requestfilesystem.go | Composes rebased filesystems. |
| tsc/internal/api/requestfilesystem/rebase_test.go | Tests input immutability. |
| tsc/internal/api/requestfilesystem/pathtree.go | Preserves removed-path metadata. |
| tsc/internal/api/requestfilesystem/filechanges.go | Generates rebase change notifications. |
| tsc/internal/api/requestfilesystem/filechanges_test.go | Tests notifications and path casing. |
| tsc/internal/api/proto.go | Defines the rebase protocol. |
| packages/typescript/test/sync/api.test.ts | Adds generated sync coverage. |
| packages/typescript/test/sync/api-generators.test.ts | Tests generator parity. |
| packages/typescript/test/async/api.test.ts | Adds async rebase coverage. |
| packages/typescript/src/api/sync/api.ts | Adds generated sync and generator APIs. |
| packages/typescript/src/api/proto.generated.ts | Adds generated protocol types. |
| packages/typescript/src/api/async/api.ts | Exposes the async rebase API. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| updatedSnapshot := session.snapshots[updated.Snapshot].snapshot | ||
| updatedProject := updatedSnapshot.ProjectCollection.GetProject(project.ID("/a/tsconfig.json")) | ||
| assert.Assert(t, updatedProject.GetProgram() != baseProgram) | ||
| assert.Equal(t, updatedProject.ProgramUpdateKind, project.ProgramUpdateKindCloned) |
There was a problem hiding this comment.
Related to my comment on session.go: this only hits ProgramUpdateKindCloned here because it's a self-rebase (Snapshot: params.Snapshot, NewSnapshot: params.Snapshot). In TestRebaseSnapshot (rebasing a layered snapshot with extra files onto a new getCurrentLanguageServerSnapshot()), the project ends up with ProgramUpdateKindNewFiles.
| } | ||
| apiRequest.FileSystem = fileSystem | ||
| apiRequest.ReplaceFileSystem = changes.FileSystem != nil && changes.FileSystem.Kind == requestfilesystem.KindFull | ||
| snapshot, err := s.snapshotHost.CloneSnapshot(ctx, target.snapshot, fileChanges, apiRequest) |
There was a problem hiding this comment.
In our case source.fileSystem has hundreds of virtual .ngtypecheck.ts files that don't exist in target.snapshot. Since handleRebaseSnapshot clones from target.snapshot and marks all of source.fileSystem's files as Created relative to target, will every rebase onto a new snapshot invalidate target's tsconfig file list and force compiler.NewProgram instead of ReuseProgram (ProgramUpdateKindCloned) when a user edits a single .ts file?
There was a problem hiding this comment.
Yeah, changes are related to target and not source so... yes, they are always "new" files (relative to the actual underlying base FS snapshot). If you wanted otherwise, we'd have to actually pull reused program state (and ASTs) from multiple parent programs, which is... I don't wanna think about it until it's shown we really have to. I'm not really sure how much we'd save by supporting it in practice - I'd need benchmarks showing it's actually majorly useful.
Alternatively, you could provide the chatty underlying fs lookup overrides to the lsp setup and retain the generated files on the "underlying real fs" that way, but I think ensuring consistency between that and the actual-real-fs is a headache you can potentially avoid with the snapshot-layering strategy.
|
I think the naming here led me to believe this would encompass more than just file system state, but we really don't carry enough info in snapshots for that to be possible. Maybe this should just be a variant of const initial = api.getCurrentLanguageServiceSnapshot()
const updated = initial.update(/* layer on custom additions */)
// fetch diagnostics for updated or whatever else you want
// actual disk/editor updates LSP snapshot
// ... later ...
const newInitial = api.getCurrentLanguageServiceSnapshot()
const reappliedUpdate = newInitial.update({
fileSystem: createFileSystemWithBase(updated)
});
// work with reappliedUpdate without needing to recalculate synthetic FS state
|
What more would you want to be merged in a snapshot "rebase"? The only thing I can think of would be reusing the ASTs from both snapshot's programs, if possible, like suggested above - which is doable, I think, just not here at present since it'd require reworking program construction a little bit. |
|
create/delete programs, open/close projects and files, user preferences changes, auto import preparation—everything you can do in a snapshot change, and anything we might add to those operations in the future. A lot of this works automatically when you rebase on top of an LSP snapshot since those don't create synthetic programs, but this API doesn't prevent you from calling To me, "rebase" conjures "take all the changes that happened between X and Y, and apply those onto Z." ( |
|
Hm... I guess that while the FS structure does represent a rebase pretty easily, for the changes (open projects, user settings, etc), we're not really "replaying" anything, just reusing the settings from the thing we're placing "on top", and, unlike the FSes which could be layering from multiple intermediate snapshots, we'd only be taking the "final" state, not "replaying" all the intermediate snapshots (unless we go out of our way to do so, which seems like a lot of extra work for basically nothing). I guess that's a good enough argument for moving/renaming it? (Though, in that case, I don't think we have a great reason to ever have a "real" |
|
Andrew Branch (@andrewbranch) does |

createFileSystemFromSnapshotallows using data from two old snapshots to make a new snapshot - the one you update from, and the one you create the FS from.Given
const rebased = new.update({ fileSystem: createFileSystemFromSnapshot(old) }), we take the synthetic FS stored inoldand reapply it overnew's existing FS data, if any (akin to agitrebase) to produce a new snapshot. (Do note: afullFS override inoldwill still replace the FS innew- it's a full replacement! The only thing that'd change here is refreshing underlying real FS files!) Optionally, we also take an extrafileSystemto layer over the snapshot filesystem, if there are additional changes on the resulting snapshot update you wish to apply -createFileSystemFromSnapshot(oldSnapshot, createFileSystemLayer(...)).What does this let you do? Well, for our LSP API users,
allowing you to retain the same synthetic FS structure you already generated across multiple real-FS updates without a lot of repeated traffic over the API protocol.