Replace api.updateSnapshot - #64204
Conversation
…hange, with RemoveProgram counterpart
…pshot() and getCurrentLanguageServerSnapshot()
There was a problem hiding this comment.
🟡 Changes recommended
Snapshot refresh and repeated-open handling contain correctness issues, and removePrograms exposes an overly broad project-ID type.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces latest-snapshot state with explicit snapshot creation and derivation, adding synthetic-program lifecycle support and branded project IDs.
Changes:
- Adds
createSnapshot,Snapshot.update, and LSP snapshot retrieval. - Supports creating, removing, and ensuring synthetic programs.
- Updates protocols, generators, caches, and tests for the new model.
File summaries
| File | Description |
|---|---|
tsc/internal/project/snapshothost.go |
Adds independent root snapshots. |
tsc/internal/project/snapshot.go |
Adds synthetic-program operations. |
tsc/internal/project/snapshot_test.go |
Tests synthetic lifecycle. |
tsc/internal/project/session.go |
Passes clients explicitly during cloning. |
tsc/internal/project/refcountcache_test.go |
Updates cache tests for synthetic programs. |
tsc/internal/project/projectcollectionbuilder.go |
Manages synthetic projects and program updates. |
tsc/internal/project/projectcollection.go |
Integrates synthetic projects into lookups. |
tsc/internal/project/project.go |
Defines synthetic projects and IDs. |
tsc/internal/project/project_stringer_generated.go |
Adds generated synthetic-kind text. |
tsc/internal/api/session.go |
Implements the redesigned snapshot API. |
tsc/internal/api/session_temporary_test.go |
Tests explicit-base updates. |
tsc/internal/api/session_createprogram_test.go |
Tests snapshot-created programs. |
tsc/internal/api/session_completion_test.go |
Migrates completion setup. |
tsc/internal/api/session_apistate_test.go |
Tests LSP snapshot state and ownership. |
tsc/internal/api/proto.go |
Defines new protocol methods and types. |
tsc/internal/api/proto_test.go |
Tests ensurePrograms decoding. |
tools/gen-proto/main.go |
Generates branded IDs and embedded interfaces. |
tools/gen-proto/main_test.go |
Verifies generated protocol output. |
packages/typescript/test/sync/astnav.test.ts |
Migrates synchronous AST navigation tests. |
packages/typescript/test/sync/ast.test.ts |
Migrates synchronous AST tests. |
packages/typescript/test/sync/api.bench.ts |
Migrates synchronous benchmarks. |
packages/typescript/test/sync/api-generators.test.ts |
Updates generator parity coverage. |
packages/typescript/test/diagnosticFormatter.test.ts |
Migrates diagnostic tests. |
packages/typescript/test/async/astnav.test.ts |
Migrates asynchronous AST navigation tests. |
packages/typescript/test/async/api.bench.ts |
Migrates asynchronous benchmarks. |
packages/typescript/src/api/sync/api.ts |
Exposes the synchronous snapshot model. |
packages/typescript/src/api/sourceFileCache.ts |
Supports branded project IDs in caching. |
packages/typescript/src/api/proto.ts |
Adds snapshot request compatibility conversion. |
packages/typescript/src/api/proto.generated.ts |
Updates generated wire declarations. |
packages/typescript/src/api/async/api.ts |
Exposes the asynchronous snapshot model. |
Review details
Files not reviewed (1)
- tsc/internal/project/project_stringer_generated.go: Generated file
- Files reviewed: 29/32 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
ea254df to
8399b74
Compare
|
I have a refactor on top of this to use strongly typed project IDs that are not just |
There was a problem hiding this comment.
🟡 Changes recommended
LSP reconciliation can mishandle close-and-reopen requests, and solution-wide operations may consume stale synthetic programs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- tsc/internal/project/project_stringer_generated.go: Generated file
Suppressed comments (1)
tsc/internal/api/session.go:1245
- The same pre-request filtering breaks close-and-reopen for files: when a currently owned file appears in both lists,
OpenFilesis removed here andCloseFilesis retained below, leaving the file closed. Independent snapshot reconciliation processes closes before opens, so the two APIs now produce different final states for the same change set. Compute both deltas from a temporary open-file state so the reopen wins.
for uri := range apiRequest.OpenFiles.Keys() {
path := s.toPath(uri.FileName())
if s.openFiles.Has(path) {
apiRequest.OpenFiles.Delete(uri)
} else {
- Files reviewed: 31/34 changed files
- Comments generated: 2
- Review effort level: Balanced
| for configFileName := range apiRequest.OpenProjects.Keys() { | ||
| configPath := s.toPath(configFileName) | ||
| if s.openProjects.Has(configPath) { | ||
| apiRequest.OpenProjects.Delete(configFileName) | ||
| } else { |
| // Projects returns all configured, synthetic, and inferred projects in a stable order. | ||
| func (c *ProjectCollection) Projects() []*Project { | ||
| if c.inferredProject == nil { | ||
| return c.ConfiguredProjects() | ||
| } | ||
| projects := make([]*Project, 0, len(c.configuredProjects)+1) | ||
| projects := make([]*Project, 0, len(c.configuredProjects)+len(c.syntheticProjects)+core.IfElse(c.inferredProject != nil, 1, 0)) | ||
| c.fillConfiguredProjects(&projects) | ||
| projects = append(projects, c.inferredProject) | ||
| projects = append(projects, c.SyntheticProjects()...) | ||
| if c.inferredProject != nil { | ||
| projects = append(projects, c.inferredProject) | ||
| } |
There will be a little more to do to integrate #64115 but I think we can say this closes #64154. Read that issue for the big picture overview—here’s the list of changes and decisions:
api.updateSnapshot()is replaced by:api.getCurrentLanguageServerSnapshot(changes?)available in LSP mode onlyapi.createSnapshot(changes?)available alwaysconst newSnapshot = snapshot.update(changes)changestakes two new operations:snap.update({ createPrograms: [/* ... */] })adds programs to any snapshotsnap.update({ ensurePrograms: [/* ... */] })returns a snapshot where projects with the given IDs have up-to-date programs.snap.update({ ensurePrograms: true })ensures all projects are up to date.oldProgramas an option in creating a program. Instead, a program can be incrementally updated by notifying the API of its changed files and usingensurePrograms:updateSnapshotrequest. Now, you have to useensurePrograms. (The exception is that projects returned byopenProjectsoropenFilesare automatically updated without need for a separateensurePrograms, even if they're already open/created.ensureProgramsis mainly needed in combination withfileChanges, so you can say which projects you care about.)operationwith information about the request that created it. Currently onlycreateProgramsandopenFilescontribute tooperation, since those result in the creation of projects with an ID that might not be known to the caller ahead of time.project.idnow has the typeProjectId = ConfiguredProjectId | InferredProjectId | SyntheticProjectId, each of which is a branded string.ConfiguredProjectIdis a subtype ofPath. The same ID is also exposed onprogram.idfor convenience.api.createProgram(rootFiles, options)is basically shorthand forapi.createSnapshot({ createPrograms: [{ rootFiles, options }] }).operation.createdPrograms[0].