lsp: implement workspace/executeCommand#120
Open
bneiswander wants to merge 21 commits into
Open
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
OverviewThis PR implements LSP workspace/executeCommand support with four PureScript-specific commands ( Key ChangesBuild Command Support
Diagnostics Architecture
Reset Command
Clean Command
Analyzer Refresh
Document Lifecycle Tracking
Error Handling
Test CoverageComprehensive test suite validates:
Quality Indicators
Merge Confidence: 4.5/5Strengths:
Minor Considerations:
The implementation is production-ready with solid error handling, comprehensive tests, and a well-thought-out approach to preventing stale diagnostics in an async LSP environment. WalkthroughAdds an LSP build command (spago/purs) that parses JSON compiler errors into per‑URI build diagnostics, tracks analyzer vs build diagnostics and open document versions, merges/deduplicates diagnostics preferring build results, and wires executeCommand handlers (build/clean/reset/analyzerRefresh) with reset safety via a diagnostics_generation token. ChangesLSP Build Handler with Diagnostic Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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: 3
🧹 Nitpick comments (2)
compiler-bin/src/lsp/event.rs (1)
107-112: ⚖️ Poor tradeoffHardcoded exclusion list is reasonable but consider making it configurable.
The exclusion of
.spago,output,.git, andnode_modulesis sensible for typical PureScript projects, but consider whether this should be configurable for non-standard project layouts.🤖 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 107 - 112, The hardcoded exclusion list in the closure using path.components(), Component::Normal and matches!(...) should be made configurable: add a configurable list (e.g., Vec<String> or HashSet<String>) on your project/config struct and replace the inline matches! check with a helper function (e.g., is_excluded_path(path, &config.excluded_dirs)) that checks each Component::Normal against the configured names; update the code that constructs the LSP/server options to load defaults [".spago","output",".git","node_modules"] when no override is provided and wire the config through to where path.components() is evaluated.compiler-bin/src/lsp.rs (1)
352-364: ⚖️ Poor tradeoffThread spawn with sleep delay is a workaround for LSP buffering.
Lines 352-364 spawn a thread that sleeps before publishing diagnostics to work around async-lsp buffering behavior. This is fragile.
Consider if there's a more reliable way to ensure diagnostics are published after the response, such as:
- Using async-lsp's built-in notification ordering guarantees
- Deferring publication via the event system rather than spawning a raw thread
- Adding a synchronization primitive to confirm the response was flushed
However, if this workaround is necessary for client compatibility, document the specific clients/scenarios where this is required.
🤖 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.rs` around lines 352 - 364, The current raw std::thread::spawn + sleep workaround (using RESET_DIAGNOSTIC_CLEAR_DELAY, publish_jobs, client.publish_diagnostics) is fragile; replace it with a reliable, event-driven defer using the async runtime or the async-lsp event loop instead of sleeping: schedule the diagnostic-clear work as an async task (e.g., tokio::spawn or the crate's task scheduler) or post it via the server's event system after the executeCommand response is confirmed flushed, or use a synchronization primitive that awaits the response flush before iterating publish_jobs and calling client.publish_diagnostics; if keeping the delay is unavoidable, add a concise comment documenting the specific clients/scenarios requiring this workaround and why RESET_DIAGNOSTIC_CLEAR_DELAY is needed.
🤖 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/Cargo.toml`:
- Line 27: Remove the unused dependency entry once_cell = "1.21.3" from the
Cargo.toml for the compiler binary and update the lockfile; specifically delete
the once_cell line from the dependency list, run cargo update (or cargo clean &&
cargo build) to refresh Cargo.lock and ensure there are no remaining references
to once_cell in the codebase (search for "once_cell" usages to confirm).
In `@compiler-bin/src/lsp/build.rs`:
- Around line 162-175: The run_spago process currently blocks indefinitely;
convert run_spago to an async function and execute the external build under a
timeout: replace std::process::Command usage with tokio::process::Command (or
spawn_blocking and run in a cancellable task) and wrap the awaited output with
tokio::time::timeout, returning a clear LspError on timeout; ensure you forward
args the same way and preserve existing output handling. Apply the same
async/timeout pattern to the other build-invoking function in the file (the
sibling block at lines 177-189) so both spago invocations are time-limited and
return errors if the command exceeds the configured duration.
- Around line 296-318: The function error_to_diagnostic currently hardcodes
DiagnosticSeverity::ERROR; change its signature to accept a severity indicator
(e.g., an additional parameter like severity: DiagnosticSeverity or is_warning:
bool) and use that to set DiagnosticSeverity::WARNING or ERROR instead of the
hardcoded value, keeping existing behavior for cli::BuildTool and reusing
extract_message/extract_range; then update all callers (the code that iterates
PureScript JSON error and warning arrays) to pass the appropriate severity based
on whether the item came from the warnings array or errors array so diagnostics
reflect true severity.
---
Nitpick comments:
In `@compiler-bin/src/lsp.rs`:
- Around line 352-364: The current raw std::thread::spawn + sleep workaround
(using RESET_DIAGNOSTIC_CLEAR_DELAY, publish_jobs, client.publish_diagnostics)
is fragile; replace it with a reliable, event-driven defer using the async
runtime or the async-lsp event loop instead of sleeping: schedule the
diagnostic-clear work as an async task (e.g., tokio::spawn or the crate's task
scheduler) or post it via the server's event system after the executeCommand
response is confirmed flushed, or use a synchronization primitive that awaits
the response flush before iterating publish_jobs and calling
client.publish_diagnostics; if keeping the delay is unavoidable, add a concise
comment documenting the specific clients/scenarios requiring this workaround and
why RESET_DIAGNOSTIC_CLEAR_DELAY is needed.
In `@compiler-bin/src/lsp/event.rs`:
- Around line 107-112: The hardcoded exclusion list in the closure using
path.components(), Component::Normal and matches!(...) should be made
configurable: add a configurable list (e.g., Vec<String> or HashSet<String>) on
your project/config struct and replace the inline matches! check with a helper
function (e.g., is_excluded_path(path, &config.excluded_dirs)) that checks each
Component::Normal against the configured names; update the code that constructs
the LSP/server options to load defaults
[".spago","output",".git","node_modules"] when no override is provided and wire
the config through to where path.components() is evaluated.
🪄 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: 82ed035b-2b1a-4a2e-ab8c-c79393e10977
⛔ 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: 1
🤖 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/build.rs`:
- Around line 134-165: When build_map.is_empty() we currently call
snapshot.client.show_message twice (once with the detailed truncated msg and
then again with a generic "Build failed" via
ShowMessageParams/MessageType::ERROR), causing duplicate notifications; fix by
making the generic "Build failed" notification only sent when we did not already
send the detailed msg (e.g., return early after sending the detailed message or
wrap the second snapshot.client.show_message call in an else branch),
referencing the existing build_map check and the snapshot.client.show_message /
ShowMessageParams usage to locate where to change the control flow.
🪄 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: 35b2c7a5-3ab7-4271-9f94-5370a4c1c46c
⛔ 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
🚧 Files skipped from review as they are similar to previous changes (3)
- compiler-bin/src/cli.rs
- compiler-bin/src/lsp/error.rs
- compiler-bin/src/lsp.rs
bneiswander
changed the title
bneiswander
changed the title
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.
bneiswander
force-pushed
the
pr/lsp-execute-command-build-clean
branch
from
cb08f4f to
410ee89
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
==========================================
- Coverage 83.09% 83.07% -0.03%
==========================================
Files 130 130
Lines 23812 23812
Branches 23812 23812
==========================================
- Hits 19786 19781 -5
- Misses 2238 2241 +3
- Partials 1788 1790 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Add LSP Execute Commands for Build, Clean, Reset, and Analyzer Refresh
Summary
workspace/executeCommandfor a small PureScript-focused command surface:purescript.build,purescript.clean,purescript.reset, andpurescript.analyzerRefresh.spagoorpurs, parse PureScript compiler JSON errors, and publish file-scoped build diagnostics without flooding clients with empty diagnostics for every workspace file.Motivation
The LSP server did not previously respond to
workspace/executeCommand, which blocked editor workflows that expect PureScript commands such as build and clean to be available from command palettes or keybindings.This PR adds a compatible, intentionally small command surface for common build/clean/reset workflows while keeping default diagnostics behavior unchanged. Diagnostics are still automatically published on open/save by default, and on change only when explicitly enabled.
User-Facing Behavior
New Execute Commands
purescript.build: runs the configured external build tool and publishes compiler diagnostics.purescript.clean: deletes the workspaceoutput/directory and clears stale build diagnostics.purescript.reset: fast diagnostic reset that clears known/open file diagnostics and invalidates analyzer caches without reloading all workspace sources.purescript.analyzerRefresh: recomputes analyzer diagnostics for refreshable workspace source files.Capability Advertisement
The server now advertises
executeCommandProvider.commandswith exactly these implemented commands:purescript.buildpurescript.cleanpurescript.resetpurescript.analyzerRefreshUnknown
workspace/executeCommandrequests are rejected with an LSP request error instead of being silently ignored.Implementation Details
Build Command
--build-tool auto--build-tool spago--build-tool purs--build-argsupport for extra build arguments.spagoautomatically whenspago.lockis present, otherwise usespurswhen configured/selected.spago build --json-errorsfor Spago builds.purs compile --json-errors <workspace sources...>for Purs builds.src/Foo.purs, relative to the workspace root.lsp_types::Diagnosticvalues with build-specific sources such asbuild/spagoandbuild/purs.Diagnostic Publication Model
Clean Command
<workspaceRoot>/outputrecursively.output/directory as success.Reset Command
This is intentionally a fast reset. Users can explicitly recompute diagnostics afterward with
purescript.analyzerRefreshorpurescript.build.Analyzer Refresh Command
file://URIs.pursfiles.spagooutput.gitnode_modulesprim://Non-Goals
purescript-language-servercommand set, such as case split or add clause.Safety And Trade-Offs
purescript.cleanis restricted to<workspaceRoot>/outputto avoid deleting arbitrary paths.Testing
output/in a fixture workspace.Validation run on the clean PR branch:
Both commands passed. The only warnings were existing deprecation warnings for test construction of
InitializeParams.root_path/root_uri.