fix use markup for diagnostics in lsp to codify backtics #1177#2352
fix use markup for diagnostics in lsp to codify backtics #1177#2352asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
Conversation
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds LSP 3.18-style markup support for diagnostic messages so clients can render diagnostic text as Markdown (e.g., backticks as code spans), and updates initialization plumbing to propagate this capability through both LSP and TSP entrypoints.
Changes:
- Introduce
InitializeInfoto carryInitializeParamsplus asupports_diagnostic_markdownflag derived from client capabilities. - When supported, wrap
Diagnostic.messagestrings into{ kind: "markdown", value: ... }for bothpublishDiagnosticsandtextDocument/diagnosticresponses. - Add a regression test that opts into markup support and asserts Markdown-shaped diagnostic messages.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/lsp/non_wasm/server.rs | Detect markup support at initialize time; conditionally rewrite diagnostic JSON to MarkupContent for push/pull diagnostics. |
| pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs | Add test verifying Markdown-shaped diagnostics when capability is enabled. |
| pyrefly/lib/commands/lsp.rs | Thread InitializeInfo through LSP startup instead of bare InitializeParams. |
| pyrefly/lib/commands/tsp.rs | Thread InitializeInfo through TSP startup and into Server::new. |
| pyrefly/lib/tsp/server.rs | Update tsp_loop signature to accept the new initialization wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| params | ||
| .get("capabilities") | ||
| .and_then(|caps| caps.get("textDocument")) | ||
| .and_then(|text_document| text_document.get("diagnostic")) | ||
| .and_then(|diagnostic| diagnostic.get("markupMessageSupport")) | ||
| .and_then(Value::as_bool) |
There was a problem hiding this comment.
diagnostic_markdown_support only checks capabilities.textDocument.diagnostic.markupMessageSupport. In the existing initialize params builder used by tests/clients, diagnostics capabilities are placed under capabilities.textDocument.publishDiagnostics (e.g., relatedInformation, versionSupport). To avoid never enabling markup for clients that follow that structure, consider also checking capabilities.textDocument.publishDiagnostics.markupMessageSupport (or switching to that path) and treating either location as opt-in.
| params | |
| .get("capabilities") | |
| .and_then(|caps| caps.get("textDocument")) | |
| .and_then(|text_document| text_document.get("diagnostic")) | |
| .and_then(|diagnostic| diagnostic.get("markupMessageSupport")) | |
| .and_then(Value::as_bool) | |
| let text_document = match params | |
| .get("capabilities") | |
| .and_then(|caps| caps.get("textDocument")) | |
| { | |
| Some(text_document) => text_document, | |
| None => return false, | |
| }; | |
| // First, honor the `textDocument.diagnostic.markupMessageSupport` setting if present. | |
| if let Some(supported) = text_document | |
| .get("diagnostic") | |
| .and_then(|diagnostic| diagnostic.get("markupMessageSupport")) | |
| .and_then(Value::as_bool) | |
| { | |
| return supported; | |
| } | |
| // Fall back to `textDocument.publishDiagnostics.markupMessageSupport`. | |
| text_document | |
| .get("publishDiagnostics") | |
| .and_then(|publish_diagnostics| publish_diagnostics.get("markupMessageSupport")) | |
| .and_then(Value::as_bool) |
| interaction | ||
| .client | ||
| .expect_any_message() | ||
| .expect("Failed to receive configuration request"); | ||
| interaction.client.send_response::<WorkspaceConfiguration>( | ||
| RequestId::from(1), |
There was a problem hiding this comment.
This test hard-codes the request id RequestId::from(1) for the server’s workspace/configuration request and uses expect_any_message() instead of asserting the request type/id. This is brittle if the server sends any other request first (or changes its outgoing request id sequence). Prefer expect_configuration_request(...) / expect_request::<WorkspaceConfiguration>(...) to capture the actual id and respond to the correct request.
| interaction | |
| .client | |
| .expect_any_message() | |
| .expect("Failed to receive configuration request"); | |
| interaction.client.send_response::<WorkspaceConfiguration>( | |
| RequestId::from(1), | |
| let request_id = interaction | |
| .client | |
| .expect_configuration_request() | |
| .expect("Failed to receive configuration request"); | |
| interaction.client.send_response::<WorkspaceConfiguration>( | |
| request_id, |
Summary
Fixes #1177
Added detection of markupMessageSupport from client capabilities and emit markdown-formatted diagnostic messages for both textDocument/publishDiagnostics and textDocument/diagnostic when supported.
Threaded the capability through initialization and server construction.
Test Plan
added an LSP interaction test covering markdown diagnostics.