Push per-file diagnostics for clients without pull diagnostics support - #63921
Push per-file diagnostics for clients without pull diagnostics support#63921Christian Vuerings (christianvuerings) wants to merge 2 commits into
Conversation
Ported from microsoft/typescript-go#4297 after the repository move.
typescript-automation
Bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
There was a problem hiding this comment.
Pull request overview
Adds push diagnostics for open files when clients lack pull-diagnostics support.
Changes:
- Detects push-only clients and schedules debounced snapshot updates.
- Publishes and clears version-aware per-file diagnostics.
- Adds project-session coverage for open, change, close, version, and disabled behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tsc/internal/project/snapshot.go |
Logs new snapshot update reasons. |
tsc/internal/project/session.go |
Schedules and publishes open-file diagnostics. |
tsc/internal/project/project_test.go |
Tests push-diagnostic session behavior. |
tsc/internal/lsp/server.go |
Enables pushes based on client capabilities. |
tsc/internal/ls/diagnostics.go |
Computes push-formatted file diagnostics. |
| } | ||
| _ = s.updateContentMapperRegistrations(ctx, newSnapshot) | ||
| s.publishProgramDiagnostics(oldSnapshot, newSnapshot) | ||
| s.publishOpenFileDiagnostics(oldSnapshot, newSnapshot) |
There was a problem hiding this comment.
Fixed in 2e7b1b3. Publishes are now serialized under a mutex and gated by snapshot ID: a pass whose snapshot is older than one already published is skipped, and clears are driven by a tracking map of published files rather than the previous snapshot's overlays, so a skipped intermediate pass cannot leave a closed file uncleared or let a stale pass publish after a close. Added a rapid didChange/didClose test, and the push diagnostics tests pass under -race with -count=3.
| _, wasOpen := oldSnapshot.fs.overlays[path] | ||
| if wasOpen && project.ProgramLastUpdate != newSnapshot.ID() { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Fixed in 2e7b1b3. The publish pass now receives the SnapshotChange and forces a full republish when the change carries new user preferences or came from a diagnostics refresh, so toggling validate.enable or reportStyleChecksAsWarnings clears or republishes open-file diagnostics even though the program did not update. Covered by a new test toggling validate.enable off and back on.
| lspDiagnostics := make([]*lsproto.Diagnostic, 0, len(diagnostics)) | ||
| for _, diag := range diagnostics { | ||
| lspDiagnostics = append(lspDiagnostics, lsconv.DiagnosticToLSPPush(ctx, l.converters, diag)) |
There was a problem hiding this comment.
Fixed in 2e7b1b3. The normalization is now shared: toLSPDiagnosticsWith holds the style-checks-as-warnings and synthesized content-mapper aggregation logic, parameterized by the pull or push converter, and DiagnosticToLSPPush gained the reportStyleChecksAsWarnings option. Push and pull clients now receive equivalent file diagnostics.
| return | ||
| } | ||
|
|
||
| ctx := s.backgroundContext() |
There was a problem hiding this comment.
Fixed in 2e7b1b3. The push pass now runs its context with core.CheckerLifetimeDiagnostics, matching the pull handler, so it uses the dedicated diagnostics checker instead of consuming query checkers.
| if capabilities := s.initializeParams.Capabilities; capabilities != nil && capabilities.TextDocument != nil { | ||
| clientSupportsPullDiagnostics = capabilities.TextDocument.Diagnostic != nil | ||
| } | ||
| pushFileDiagnostics := !disablePushDiagnostics && !clientSupportsPullDiagnostics |
There was a problem hiding this comment.
Added in 2e7b1b3: TestPushFileDiagnosticsGate in tsc/internal/lsp runs the real initialize handshake and covers all three cases: a push-only client receives per-file publishes, a pull-capable client receives none, and disablePushDiagnostics disables them.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Opened #63928 as the linked issue. Arsh Verma (@ArshVermaGit) thanks for the review. 2e7b1b3 addresses the race concern: publishes are now serialized and gated by snapshot ID, so a slow pass for an older snapshot can no longer publish after a later change or close. New coverage: a rapid didChange sequence followed by an immediate didClose (asserting the final publish is the empty clear), a validation-preference toggle, and a server-level initialization gate test over the real LSP handshake; the push diagnostics tests also pass under -race with -count=3. On LSP restarts: the server keeps no cross-session state, so after a restart a reopened file gets fresh diagnostics on didOpen (covered by the open tests); diagnostics a client keeps showing for files it never reopens are client-side state that a server-side test cannot observe, and clearing those on server exit is the client's responsibility. |
Fixes #63928.
Recreated from microsoft/typescript-go#4297, which was closed unmerged when development moved back to this repository (microsoft/typescript-go#4918). The change is ported onto the new
tsc/layout and rebased against currentmain. cc Jake Bailey (@jakebailey), who reviewed the original PR.Problem
The LSP server only supports pull diagnostics (
textDocument/diagnostic); push diagnostics are only used for config file errors. Clients that do not implement pull diagnostics get no file diagnostics at all. This affects eglot before Dec 2025, Nova, Claude Code, and other lighter LSP clients (see the discussion in microsoft/typescript-go#2362 and anthropics/claude-code#40282).Change
When the client advertises
textDocument.publishDiagnosticsbut nottextDocument.diagnostic, the server now pushes per-file diagnostics for open files:didOpenpublishes initial diagnostics for the opened filedidChangeschedules a debounced (500ms) snapshot update that requests open documents, so dirty programs are rebuilt eagerly, and republishes diagnostics for open files whose program updateddidCloseclears diagnostics for the closed fileworkspace/diagnostic/refresh(watched file changes, config changes, ATA updates) schedule a snapshot update instead, since push-only clients cannot re-pullClients that support pull diagnostics are unaffected (verified: zero per-file pushes when
textDocument.diagnosticis declared), and the existingdisablePushDiagnosticsinitialization option also disables the new path. Diagnostics are converted with the existingDiagnosticToLSPPushpath and include the document version.Two adaptations were made while porting to current
main:ProvidePushDiagnosticshonors theEnableValidationuser preference, matching the pull pathDidChangeFile, push-only clients return after scheduling the snapshot update, skipping the content-mapperworkspace/diagnostic/refreshpath since they cannot act on a refresh requestValidation
tsc/internal/project/project_test.go(open/change/close/version support/disabled)go test ./tsc/internal/project/... ./tsc/internal/ls/... ./tsc/internal/lsp/...passesdidOpenand ~500ms after eachdidChange, are cleared ondidClose, and pull requests keep working alongsideNotes
The original review raised that push diagnostics get tricky around LS restarts and racy conditions, and would need testing in VS Code with pull diagnostics disabled; happy to help validate that or adjust the approach (for example gating behind an initialization option instead of capability detection).
Disclosure per CONTRIBUTING.md: this patch was authored with AI assistance (Claude Code). I have read and understand the change and will respond to review feedback myself.