feat: add diagnostics#8
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces LSP diagnostics support end-to-end (IDE → LSP conversions → server handlers), including pull diagnostics (textDocument/diagnostic, workspace/diagnostic), push diagnostics for legacy clients (textDocument/publishDiagnostics), and a user-configurable toggle to disable semantic diagnostics.
Changes:
- Add a new
ide::diagnosticsmodule and expose diagnostics APIs onAnalysis. - Implement LSP diagnostics plumbing: conversion to
lsp_types::Diagnostic, request handlers for document/workspace diagnostic pull, and push publishing for non-pull clients. - Add
semantic_diagnostics_enableconfiguration and tests validating pull vs publish behavior and semantic diagnostics filtering.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Adds integration-style tests around diagnostic behavior (pull vs publish, semantic toggle). |
| src/lsp_ext/to_proto.rs | Adds IDE→LSP diagnostic conversion and threads diagnostics through code actions. |
| src/global_state/snapshot.rs | Adds snapshot helpers for diagnostics retrieval and enumerating file IDs. |
| src/global_state/process_changes.rs | Triggers diagnostics publication/refresh in response to VFS changes. |
| src/global_state/main_loop.rs | Adds diagnostics task type and push-based publish implementation. |
| src/global_state/handlers/request.rs | Implements textDocument/diagnostic + workspace/diagnostic handlers and code action diagnostic attachment/filtering. |
| src/global_state.rs | Stores last-published diagnostics for deduplication. |
| src/config/user_config.rs | Adds semantic_diagnostics_enable and config accessor. |
| src/config/caps.rs | Advertises diagnostics server capabilities and checks for client diagnostic/refresh support. |
| crates/ide/src/lib.rs | Exposes the new diagnostics module. |
| crates/ide/src/diagnostics.rs | Implements parse + semantic diagnostics aggregation and ranges. |
| crates/ide/src/analysis.rs | Adds Analysis::{diagnostics, parse_diagnostics} APIs. |
| crates/base-db/src/source_db.rs | Adds salsa queries for parse + semantic diagnostics via SyntaxTree / Compilation. |
| Cargo.toml | Adds syntax workspace dependency needed for diagnostic severity mapping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn request_diagnostics(&mut self, files: Vec<FileId>) { | ||
| if files.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| if !self.config.cli_pull_diagnostics_support() { | ||
| let snapshot = self.make_snapshot(); |
There was a problem hiding this comment.
request_diagnostics will compute/publish diagnostics for every changed FileId. Because the VFS loader can be asked to invalidate() arbitrary paths (e.g. vizsla_config.toml is watched via client-side file watching), non-SystemVerilog files can enter the VFS and would now produce/publish SystemVerilog parse diagnostics. Consider filtering files to supported extensions (*.v/*.sv) before computing/publishing diagnostics or requesting a diagnostics refresh.
| fn request_diagnostics(&mut self, files: Vec<FileId>) { | |
| if files.is_empty() { | |
| return; | |
| } | |
| if !self.config.cli_pull_diagnostics_support() { | |
| let snapshot = self.make_snapshot(); | |
| fn is_supported_diagnostics_uri(uri: &lsp_types::Url) -> bool { | |
| matches!(uri.path(), path if path.ends_with(".sv") || path.ends_with(".v")) | |
| } | |
| fn request_diagnostics(&mut self, files: Vec<FileId>) { | |
| if files.is_empty() { | |
| return; | |
| } | |
| let snapshot = self.make_snapshot(); | |
| let files = files | |
| .into_iter() | |
| .filter(|&file_id| Self::is_supported_diagnostics_uri(&snapshot.url(file_id))) | |
| .collect_vec(); | |
| if files.is_empty() { | |
| return; | |
| } | |
| if !self.config.cli_pull_diagnostics_support() { |
| unchanged_document_diagnostic_report: | ||
| lsp_types::UnchangedDocumentDiagnosticReport { | ||
| result_id: result_id.expect("matching previous result id must exist"), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
document_diagnostic_report can panic when both result_id and previous_result_id are None: the equality check treats them as matching, and then result_id.expect(...) unwraps None. This can happen if a client requests diagnostics for a file without a tracked version (e.g. not opened). Consider only returning Unchanged when result_id is Some and matches previous_result_id (otherwise return Full).
| uri, | ||
| version, | ||
| unchanged_document_diagnostic_report: | ||
| lsp_types::UnchangedDocumentDiagnosticReport { | ||
| result_id: result_id.expect("matching previous result id must exist"), |
There was a problem hiding this comment.
workspace_diagnostic_report has the same None-matching panic risk as document_diagnostic_report: if both result_id and previous_result_id are None, the code enters the Unchanged branch and then unwraps result_id with expect(...). This is likely for unopened files (no version) and for the "removed URI" reports where result_id is passed as None. Gate the Unchanged case on result_id.is_some() (or provide a non-optional stable result_id).
No description provided.