impl Support LSP $/progress messages #422#2591
impl Support LSP $/progress messages #422#2591asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements LSP work-done progress reporting (window/workDoneProgress/create + $/progress) during background recheck transactions, while preserving existing streaming diagnostics behavior.
Changes:
- Added an LSP progress subscriber (throttled) and progress token generation with client capability gating.
- Introduced
CompositeSubscriberto fan out subscriber events to both diagnostics publishing and progress reporting. - Added an LSP interaction test to validate
window/workDoneProgress/createand$/progressbegin behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
pyrefly/lib/lsp/non_wasm/server.rs |
Adds progress token support and a progress subscriber, and composes it with existing diagnostics subscriber during rechecks. |
pyrefly/lib/state/subscriber.rs |
Adds CompositeSubscriber to forward Subscriber events to multiple inner subscribers. |
pyrefly/lib/test/lsp/lsp_interaction/progress.rs |
Adds an interaction test covering progress create + begin notifications. |
pyrefly/lib/test/lsp/lsp_interaction/mod.rs |
Registers the new progress interaction test module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert!( | ||
| !subscribers.is_empty(), | ||
| "CompositeSubscriber requires at least one subscriber" | ||
| ); |
There was a problem hiding this comment.
CompositeSubscriber::new is a public constructor that panics on an empty subscriber list. Since this is in a library module, it would be safer to avoid a hard panic (e.g., make the type/ctor pub(crate), return a Result<Self, ...>, or allow an empty list as a no-op subscriber) to prevent unexpected crashes if used elsewhere.
| assert!( | |
| !subscribers.is_empty(), | |
| "CompositeSubscriber requires at least one subscriber" | |
| ); | |
| // Allow an empty subscriber list; this results in a no-op composite subscriber. |
| fn new(server: &'a Server, title: &'static str) -> Option<Self> { | ||
| if !server.supports_work_done_progress() { | ||
| return None; | ||
| } | ||
| let token = server.new_progress_token(); | ||
| server.send_request::<WorkDoneProgressCreate>(WorkDoneProgressCreateParams { | ||
| token: token.clone(), | ||
| }); |
There was a problem hiding this comment.
window/workDoneProgress/create is sent, but the server may start emitting $/progress notifications immediately (on the first start_work) without waiting for the create request to be acknowledged. Per LSP, clients may ignore progress updates until the token is created. Consider tracking the create request/response (e.g., store the request id/token in Server, mark it ready on the matching response, and have LspProgressSubscriber suppress Begin/Report until ready) so Begin cannot be sent before the create response.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
No diffs to classify. |
Summary
Fixes #422
Added an LSP progress subscriber that emits $/progress with window/workDoneProgress/create for recheck transactions, throttled to avoid spam.
Composed progress + streaming diagnostics via a new CompositeSubscriber so existing behavior remains unchanged while progress is reported.
Introduced progress token generation and client capability gating.
Test Plan
Added an LSP interaction test to verify window/workDoneProgress/create and $/progress begin notifications when a recheck is triggered.