lsp: refresh dependency diagnostics incrementally (stacked on execute commands)#128
Open
bneiswander wants to merge 24 commits into
Open
lsp: refresh dependency diagnostics incrementally (stacked on execute commands)#128bneiswander wants to merge 24 commits into
bneiswander wants to merge 24 commits into
Conversation
Spago expects --json-errors as a spago flag, not forwarded to purs. Also parse JSON from both stdout/stderr and avoid publishing diagnostics for non-file URIs to prevent Emacs lsp-mode failures.
Track URIs for which build diagnostics were published and clear only those on the next build. This prevents emitting empty diagnostics for every known file, which can freeze Emacs lsp-mode on large workspaces.
Spago JSON errors report relative filenames (e.g. src/Foo.purs). Join these against the workspace root when building diagnostic URIs so Emacs receives file:// URIs and renders diagnostics correctly.
Persist build diagnostics alongside analyzer diagnostics and publish a merged, de-duplicated set per URI. This prevents build-only errors from disappearing when a file is reopened, and clears both sources on purescript.reset.
Stop reloading and parsing the full workspace on purescript.reset. Reset now clears published diagnostics and invalidates caches while keeping current file contents, making it usable on large projects.
purescript.clean deletes output artifacts; clear stored build diagnostics and republish merged diagnostics so clients don't keep showing stale build-only errors.
Bump a diagnostics generation counter on purescript.reset and have in-flight build/analyzer tasks skip publishing if a reset occurred. This ensures reset reliably clears diagnostics even when background tasks are still running.
Track open document URIs and include them in the reset clear set. This ensures purescript.reset reliably clears visible diagnostics even if the server didn't record them in its diagnostic maps.
Handle purescript.reset directly in the executeCommand handler instead of dispatching via an internal event. This makes reset reliably publish diagnostic clears immediately.
Emit a window/showMessage breadcrumb when purescript.reset is invoked, to confirm executeCommand handling in client traces while diagnosing reset diagnostic clearing.
Always emit a window/showMessage after clearing diagnostics in purescript.reset, to confirm in client traces that reset executed and published clears.
Handle purescript.reset synchronously in the executeCommand request handler by clearing stored diagnostics and publishing empty publishDiagnostics for affected URIs. This avoids relying on internal event dispatch and makes reset effective immediately.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughImplements LSP build integration (Spago/Purs), separate build vs analyzer diagnostic caches with merged publishing, new executeCommand handlers (build/clean/reset/analyzer-refresh), workspace import graph tracking, document lifecycle/version tracking, and a generation-aware, cancellable analyzer scheduling pipeline. ChangesLSP Build Integration & Diagnostics Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant LSP_Server
participant Build_Module
participant Compiler
participant Event_Handler
Client->>LSP_Server: executeCommand(purescript.build)
LSP_Server->>Build_Module: build(state, Build)
Build_Module->>Compiler: spawn compiler with timeout
Compiler-->>Build_Module: stdout/stderr
Build_Module->>Build_Module: parse JSON diagnostics
Build_Module->>LSP_Server: store build_diagnostics
LSP_Server->>LSP_Server: merge build + analyzer diagnostics
LSP_Server-->>Client: publishDiagnostics
Client->>LSP_Server: executeCommand(purescript.reset)
LSP_Server->>LSP_Server: cancel work, bump diagnostics_generation
LSP_Server->>LSP_Server: clear diagnostic caches
LSP_Server-->>Client: publish empty diagnostics (delayed)
Client->>LSP_Server: didChange(uri)
LSP_Server->>LSP_Server: invalidate build diagnostics for uri
LSP_Server->>Event_Handler: schedule_dependant_diagnostics(uri)
Event_Handler->>Event_Handler: compute ordered dependants
Event_Handler->>LSP_Server: collect and merge diagnostics
LSP_Server-->>Client: publishDiagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
compiler-bin/src/lsp/event.rs (2)
646-660: 💤 Low valueConsider combining write lock acquisitions.
Lines 647-648 acquire the write lock twice in succession. This could be combined into a single lock acquisition for slightly better efficiency:
♻️ Suggested change
pub(crate) fn rebuild_workspace_graph(state: &mut State) -> Result<(), LspError> { - state.workspace_graph.write().imports.clear(); - state.workspace_graph.write().dependants.clear(); + { + let mut graph = state.workspace_graph.write(); + graph.imports.clear(); + graph.dependants.clear(); + } let file_ids = { let files = state.files.read(); files.iter_id().collect_vec() };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-bin/src/lsp/event.rs` around lines 646 - 660, The code in rebuild_workspace_graph repeatedly calls state.workspace_graph.write() twice to clear imports and dependants; acquire the write lock once (e.g., let mut wg = state.workspace_graph.write()) and call wg.imports.clear() and wg.dependants.clear() with that single guard to avoid two successive write lock acquisitions while preserving the rest of the function and calls to update_workspace_graph_for_file.
178-194: 💤 Low valueMinor: Redundant mutable binding.
The snapshot is created and then immediately assigned to a mutable binding on the next line. Consider combining:
♻️ Suggested simplification
fn publish_uri_if_changed(state: &mut State, uri: Url) -> Result<(), LspError> { - let snapshot = StateSnapshot { + let mut snapshot = StateSnapshot { client: state.client.clone(), config: std::sync::Arc::clone(&state.config), engine: state.engine.snapshot(), files: std::sync::Arc::clone(&state.files), workspace_symbols_cache: std::sync::Arc::clone(&state.workspace_symbols_cache), suggestions_cache: std::sync::Arc::clone(&state.suggestions_cache), build_diagnostics: std::sync::Arc::clone(&state.build_diagnostics), analyzer_diagnostics: std::sync::Arc::clone(&state.analyzer_diagnostics), published_diagnostics: std::sync::Arc::clone(&state.published_diagnostics), diagnostics_generation: std::sync::Arc::clone(&state.diagnostics_generation), root: state.root.clone(), }; - let mut snapshot = snapshot; snapshot.publish_merged_diagnostics_if_changed(uri) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-bin/src/lsp/event.rs` around lines 178 - 194, In publish_uri_if_changed, remove the redundant two-step binding by declaring the snapshot as mutable in one statement (replace the separate let snapshot = StateSnapshot { ... }; let mut snapshot = snapshot; with let mut snapshot = StateSnapshot { ... };) and then call snapshot.publish_merged_diagnostics_if_changed(uri); this uses the StateSnapshot struct and the publish_merged_diagnostics_if_changed method directly without the unnecessary reassignment.compiler-bin/src/lsp/build.rs (4)
358-362: 💤 Low valueConsider using
unreachable!()forBuildTool::Autocase.Since
Autois resolved to eitherSpagoorPursinbuild_core(line 68 marks it as unreachable), usingunreachable!()here would make the invariant explicit and help catch any future misuse.♻️ Suggested change
let source = match tool { cli::BuildTool::Spago => "build/spago", cli::BuildTool::Purs => "build/purs", - cli::BuildTool::Auto => "build", + cli::BuildTool::Auto => unreachable!("Auto should be resolved before diagnostics"), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-bin/src/lsp/build.rs` around lines 358 - 362, The match that sets `source` for `tool` includes `cli::BuildTool::Auto => "build"`, but `Auto` is supposed to be resolved in `build_core`; replace the `Auto` arm with an `unreachable!()` invocation (e.g., `unreachable!("BuildTool::Auto should be resolved in build_core")`) so the invariant is explicit and any future misuse triggers a clear panic; update the match in the code that assigns `source` (the `match tool { ... }` block) accordingly.
204-226: 💤 Low valuePotential hang if stdout/stderr tasks don't complete after child exits.
After
child.wait()completes (or times out and the child is killed), the code awaitsstdout_taskandstderr_task. These tasks read until EOF, which should occur when the child's pipes close. However, if the child is killed abruptly, there's a small risk the tasks could hang if the pipe isn't properly closed by the OS. This is typically fine on Unix/Windows, but worth noting.Additionally, on timeout, the stdout/stderr tasks are not explicitly cancelled—they'll continue running until they complete reading. This is generally acceptable since killing the child should close the pipes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-bin/src/lsp/build.rs` around lines 204 - 226, The stdout/stderr reader tasks (stdout_task, stderr_task) can hang if the child pipes don't close; to fix, ensure those JoinHandles are cancelled on timeout or when the child is killed and protect awaiting them with timeouts: when Err(_) from time::timeout(BUILD_COMMAND_TIMEOUT, child.wait()) do child.start_kill(), then call stdout_task.abort() and stderr_task.abort() and await them (handle JoinError) before returning the BuildTimeout error; similarly, after a successful child.wait() result, await stdout_task and stderr_task with a short timeout (e.g., time::timeout) and abort and handle them if they don’t finish to avoid permanent hangs (references: stdout_task, stderr_task, child.wait, BUILD_COMMAND_TIMEOUT).
51-58: 💤 Low valueSimplify redundant match arms.
The match arms for
SpagoandPurssimply return themselves, which is unnecessary. Consider simplifying:♻️ Suggested simplification
let tool = match build_tool_cfg { - cli::BuildTool::Spago => cli::BuildTool::Spago, - cli::BuildTool::Purs => cli::BuildTool::Purs, + cli::BuildTool::Spago | cli::BuildTool::Purs => build_tool_cfg, cli::BuildTool::Auto => { let has_lock = root.join("spago.lock").exists(); if has_lock { cli::BuildTool::Spago } else { cli::BuildTool::Purs } } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-bin/src/lsp/build.rs` around lines 51 - 58, The match on build_tool_cfg has redundant arms returning the same variant; replace the explicit Spago and Purs arms with a catch-all that returns the incoming variant and keep the Auto branch for the lock-file check — e.g. match build_tool_cfg { cli::BuildTool::Auto => { let has_lock = root.join("spago.lock").exists(); if has_lock { cli::BuildTool::Spago } else { cli::BuildTool::Purs } }, other => other } so the variable tool is set without repeating BuildTool::Spago/BuildTool::Purs.
236-242: 💤 Low valueNaive command splitting doesn't handle quoted arguments.
Line 237 uses
split(' ')which doesn't handle shell-style quoting. Commands likemy-command --arg='foo bar'would break. Since the source_command is user-provided config, document this limitation clearly or use theshell-wordscrate (https://crates.io/crates/shell-words) for proper POSIX shell argument parsing if users need this flexibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-bin/src/lsp/build.rs` around lines 236 - 242, The function sources_from_command currently uses command.split(' ') which fails on quoted arguments; replace the naive split with a POSIX-aware parser such as shell_words::split(command) and map its error into LspError (e.g., return Err(LspError::InvalidSourceCommand) or a new variant) so quoted args like "--arg='foo bar'" are preserved; locate sources_from_command, remove split(' '), call shell_words::split(command) to get Vec<String>, take the first element as program and pass the remaining elements to cmd.args(...); add shell-words to Cargo.toml if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compiler-bin/src/lsp.rs`:
- Around line 429-470: The reset path clears state.build_diagnostics and
state.analyzer_diagnostics but never touches state.published_diagnostics, which
lets fast follow-up diagnostics get lost when the delayed empty publishes run;
update the reset logic to also remove the same uris from
state.published_diagnostics (or atomically replace their entries) before
spawning the thread that sends empty PublishDiagnostics, and/or update
state.published_diagnostics when enqueuing publish_jobs so the cache matches the
in-flight empty publishes (use the same uris_to_clear/publish_jobs and the
published_diagnostics map in the state to remove or set entries accordingly so
future publishes aren’t skipped). Ensure you reference and modify the same Url
keys used when building uris_to_clear, e.g. access state.published_diagnostics
in the block that clears build_diagnostics/analyzer_diagnostics and when
creating publish_jobs (and keep the delayed client.publish_diagnostics call and
RESET_DIAGNOSTIC_CLEAR_DELAY unchanged).
---
Nitpick comments:
In `@compiler-bin/src/lsp/build.rs`:
- Around line 358-362: The match that sets `source` for `tool` includes
`cli::BuildTool::Auto => "build"`, but `Auto` is supposed to be resolved in
`build_core`; replace the `Auto` arm with an `unreachable!()` invocation (e.g.,
`unreachable!("BuildTool::Auto should be resolved in build_core")`) so the
invariant is explicit and any future misuse triggers a clear panic; update the
match in the code that assigns `source` (the `match tool { ... }` block)
accordingly.
- Around line 204-226: The stdout/stderr reader tasks (stdout_task, stderr_task)
can hang if the child pipes don't close; to fix, ensure those JoinHandles are
cancelled on timeout or when the child is killed and protect awaiting them with
timeouts: when Err(_) from time::timeout(BUILD_COMMAND_TIMEOUT, child.wait()) do
child.start_kill(), then call stdout_task.abort() and stderr_task.abort() and
await them (handle JoinError) before returning the BuildTimeout error;
similarly, after a successful child.wait() result, await stdout_task and
stderr_task with a short timeout (e.g., time::timeout) and abort and handle them
if they don’t finish to avoid permanent hangs (references: stdout_task,
stderr_task, child.wait, BUILD_COMMAND_TIMEOUT).
- Around line 51-58: The match on build_tool_cfg has redundant arms returning
the same variant; replace the explicit Spago and Purs arms with a catch-all that
returns the incoming variant and keep the Auto branch for the lock-file check —
e.g. match build_tool_cfg { cli::BuildTool::Auto => { let has_lock =
root.join("spago.lock").exists(); if has_lock { cli::BuildTool::Spago } else {
cli::BuildTool::Purs } }, other => other } so the variable tool is set without
repeating BuildTool::Spago/BuildTool::Purs.
- Around line 236-242: The function sources_from_command currently uses
command.split(' ') which fails on quoted arguments; replace the naive split with
a POSIX-aware parser such as shell_words::split(command) and map its error into
LspError (e.g., return Err(LspError::InvalidSourceCommand) or a new variant) so
quoted args like "--arg='foo bar'" are preserved; locate sources_from_command,
remove split(' '), call shell_words::split(command) to get Vec<String>, take the
first element as program and pass the remaining elements to cmd.args(...); add
shell-words to Cargo.toml if missing.
In `@compiler-bin/src/lsp/event.rs`:
- Around line 646-660: The code in rebuild_workspace_graph repeatedly calls
state.workspace_graph.write() twice to clear imports and dependants; acquire the
write lock once (e.g., let mut wg = state.workspace_graph.write()) and call
wg.imports.clear() and wg.dependants.clear() with that single guard to avoid two
successive write lock acquisitions while preserving the rest of the function and
calls to update_workspace_graph_for_file.
- Around line 178-194: In publish_uri_if_changed, remove the redundant two-step
binding by declaring the snapshot as mutable in one statement (replace the
separate let snapshot = StateSnapshot { ... }; let mut snapshot = snapshot; with
let mut snapshot = StateSnapshot { ... };) and then call
snapshot.publish_merged_diagnostics_if_changed(uri); this uses the StateSnapshot
struct and the publish_merged_diagnostics_if_changed method directly without the
unnecessary reassignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 494f41b2-b738-4fb5-a766-46edb7446da1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
compiler-bin/Cargo.tomlcompiler-bin/src/cli.rscompiler-bin/src/lsp.rscompiler-bin/src/lsp/build.rscompiler-bin/src/lsp/error.rscompiler-bin/src/lsp/event.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compiler-bin/src/lsp/event.rs`:
- Around line 25-49: The pruning step uses state.root directly even when you
just fell back to the None scope, causing recovered files to be misclassified;
capture the actual refresh scope you used (e.g. let refresh_root =
state.root.as_deref(); and set refresh_root = None if you called
refreshable_file_ids_and_excluded_uris(state, None) for the fallback) and then
pass refresh_root into is_refreshable_workspace_source_uri instead of
state.root.as_deref() when building excluded_uris (references:
refreshable_file_ids_and_excluded_uris, is_refreshable_workspace_source_uri,
file_ids, excluded_uris, analyzer).
- Around line 319-329: The code clears build diagnostics and republishes merged
diagnostics even when the clean failed; only perform
snapshot.build_diagnostics.write().clear() and the for loop calling
snapshot.publish_merged_diagnostics_if_changed(uri) when the clean succeeded
(i.e., when res is Ok). Move or wrap those statements behind an if res.is_ok()
(or match on res) so that on Err you log the error (as already done) and
return/skip clearing and republishing; reference res,
snapshot.build_diagnostics, snapshot.publish_merged_diagnostics_if_changed, and
build_uris to locate and modify the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f8de2ec1-4f6a-4873-93dd-b49a03a3b5af
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
compiler-bin/Cargo.tomlcompiler-bin/src/lsp.rscompiler-bin/src/lsp/build.rscompiler-bin/src/lsp/event.rstests-package-set/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- compiler-bin/Cargo.toml
- compiler-bin/src/lsp/build.rs
- compiler-bin/src/lsp.rs
… diagnostics clear on clean failure - Capture variable tracking the actual scope used (including fallback) and pass it through when pruning stale analyzer diagnostics. - Only clear and republish merged diagnostics when succeeds; on failure log the error and return without clearing.
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
1 participant
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.
lsp: refresh dependency diagnostics incrementally
Base branch:
pr/lsp-execute-command-build-cleanReview Order
This PR is stacked on
pr/lsp-execute-command-build-cleanand should be reviewed after that PR.Base branch for review:
pr/lsp-execute-command-build-cleanIt builds on the diagnostics storage, reset, and analyzer-refresh infrastructure introduced by the execute-command PR.
Summary
Motivation
Before this change, analyzer diagnostics could become stale when a source module changed in a way that affected modules importing it. Users would often need an explicit refresh or reset to see downstream errors update.
This matters for editor workflows where changing an exported name or type in one file should quickly update diagnostics in files that import it, without forcing a full workspace reload or publishing diagnostics for unrelated dependencies.
Motivating Example
If
A.pursrenames an exported value,B.pursshould refresh because it importsA:After saving
A.purs, the analyzer should refreshB.pursand report thatoldNameis unknown, while avoiding unnecessary refreshes for unrelated modules.Implementation Notes
Tests
Added/updated unit coverage for:
Validation run locally: