Do not coalesce LSP responses as mutations - #4502
Open
heejaechang wants to merge 3 commits into
Open
Conversation
Client responses are FIFO messages, but they do not guarantee a validation that can replace work skipped by an earlier document mutation. Treating them as subsequent mutations could therefore suppress didOpen diagnostics permanently. Classifying responses as non-mutations preserves FIFO handling while ensuring didOpen performs its required validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Document event kinds in terms of whether later events make earlier mutation work redundant, and keep the classification match focused on classification rather than the motivating bug. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the existing Query event-kind vocabulary while documenting that queries and responses are normal-priority events that do not count as mutations for coalescing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D115504842. (Because this pull request was imported automatically, there will not be any future comments.) |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LSP responses are processed on the normal FIFO queue, but they were classified as state mutations. That classification also updates the queue's
last_mutationmarker, which tells earlier event handlers that a later mutation is waiting and that expensive work such as validation can be deferred.This can lose diagnostics during startup:
textDocument/didOpen.workspace/configuration.didOpenreportssubsequent_mutation = true.didOpenskips open-file validation under the assumption that the later mutation will make that work redundant.The server can then remain healthy and responsive while never publishing diagnostics for the opened file until some unrelated later event triggers validation.
This change classifies
LspResponsewith normal-priority query events instead. Responses still preserve FIFO ordering and are handled exactly as before, but they no longer claim that work triggered by preceding document mutations is redundant. This matches the queue's coalescing invariant: only events that can safely supersede prior mutation work should updatelast_mutation.A deterministic regression enqueues
didOpenfollowed by a configuration-shaped response and verifies that:didOpenis not reported as having a subsequent mutation, so validation is not suppressed.Test Plan
cargo test lsp::non_wasm::queue::tests --quietpython3 test.py --no-test --no-tensor-shapes --no-conformance --no-jsonschema