lsp: implement textDocument/formatting#111
Open
bneiswander wants to merge 19 commits into
Open
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds optional external document formatter support to the PureScript LSP server. Users set ChangesExternal Formatter Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
compiler-bin/src/lsp.rs (1)
119-140:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat empty
--format-commandas “disabled”.Line 119 enables formatting on
Some(""), but formatting requests then fail later as empty command. Gate capability on non-empty trimmed content.Suggested patch
- let formatting_enabled = state.config.format_command.is_some(); + let formatting_enabled = state + .config + .format_command + .as_deref() + .is_some_and(|cmd| !cmd.trim().is_empty());- let Some(format_command) = snapshot.config.format_command.as_deref() else { + let Some(format_command) = snapshot + .config + .format_command + .as_deref() + .map(str::trim) + .filter(|cmd| !cmd.is_empty()) + else { return Ok(None); };🤖 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 119 - 140, The capability for document formatting currently treats Some("") as enabled; update the check that sets formatting_enabled so it only returns true for a Some value whose trimmed string is non-empty (inspect state.config.format_command, e.g. via as_ref() and trim() or as_deref()), and keep using formatting_enabled for document_formatting_provider and any other gating; this ensures empty or whitespace-only format_command is treated as disabled.
🤖 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 662-777: The tests call hard-coded Unix paths (e.g., "/bin/cat",
"/usr/bin/tr") via base_config and thus fail on non-Unix systems; make them
OS-portable by marking these tests as Unix-only: add #[cfg(unix)] above each
test fn (formatting_returns_full_document_edit,
formatting_noop_returns_empty_edits, formatting_parses_quoted_args,
formatting_tolerates_outer_quoted_command_string) or, alternatively, change the
base_config calls to pass portable command names found on PATH (e.g., "cat" or
"tr") instead of absolute paths; update the mk_state_with/base_config usage
accordingly so CI on non-Unix platforms skips or uses a portable command.
- Around line 362-375: The formatter child process started via cmd.spawn
(variable child) can hang; wrap the blocking wait_with_output step with a
bounded timeout: after writing input to child.stdin, poll child.try_wait in a
loop with a small sleep (or use a wait_timeout helper) up to a configurable
duration, and if the timeout expires call child.kill() and child.wait() to reap
it, then return an LspError::FormattingFailed indicating timeout; ensure any
errors from kill/wait are mapped into the same error path and preserve context
(include format_command/input context) when replacing the current
wait_with_output map_err handling.
---
Outside diff comments:
In `@compiler-bin/src/lsp.rs`:
- Around line 119-140: The capability for document formatting currently treats
Some("") as enabled; update the check that sets formatting_enabled so it only
returns true for a Some value whose trimmed string is non-empty (inspect
state.config.format_command, e.g. via as_ref() and trim() or as_deref()), and
keep using formatting_enabled for document_formatting_provider and any other
gating; this ensures empty or whitespace-only format_command is treated as
disabled.
🪄 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: e557566d-ca13-49fa-a760-029c7e181425
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
compiler-bin/Cargo.tomlcompiler-bin/src/cli.rscompiler-bin/src/lsp.rscompiler-bin/src/lsp/error.rs
bneiswander
force-pushed
the
lsp-formatting-provider
branch
from
39ba571 to
aad3419
Compare
bneiswander
changed the title
bneiswander
changed the title
purefunctor
requested changes
May 18, 2026
purefunctor
left a comment
purefunctor
left a comment
Owner
There was a problem hiding this comment.
Some small comments before this can be merged
Comment on lines
+632
to
+831
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| use async_lsp::lsp_types::{ | ||
| ClientCapabilities, DocumentFormattingParams, InitializeParams, Position, | ||
| TextDocumentIdentifier, Url, WorkspaceFolder, | ||
| }; | ||
|
|
||
| fn mk_state_with(config: cli::Config) -> State { | ||
| // ClientSocket isn't used by initialize/formatting logic in tests. | ||
| let client = ClientSocket::new_closed(); | ||
| State::new(Arc::new(config), client) | ||
| } | ||
|
|
||
| fn mk_init_params(root: &std::path::Path) -> InitializeParams { | ||
| InitializeParams { | ||
| process_id: None, | ||
| root_path: None, | ||
| root_uri: None, | ||
| initialization_options: None, | ||
| capabilities: ClientCapabilities::default(), | ||
| trace: None, | ||
| workspace_folders: Some(vec![WorkspaceFolder { | ||
| uri: Url::from_file_path(root).unwrap(), | ||
| name: "workspace".to_string(), | ||
| }]), | ||
| work_done_progress_params: WorkDoneProgressParams::default(), | ||
| client_info: None, | ||
| locale: None, | ||
| } | ||
| } | ||
|
|
||
| fn base_config(format_command: Option<String>) -> cli::Config { | ||
| cli::Config { | ||
| stdio: true, | ||
| log_file: false, | ||
| query_log: tracing::level_filters::LevelFilter::OFF, | ||
| lsp_log: tracing::level_filters::LevelFilter::INFO, | ||
| checking_log: tracing::level_filters::LevelFilter::OFF, | ||
| source_command: None, | ||
| format_command, | ||
| diagnostics_on_open: true, | ||
| diagnostics_on_save: true, | ||
| diagnostics_on_change: false, | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn formatting_capability_not_advertised_without_flag() { | ||
| let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); | ||
| let mut state = mk_state_with(base_config(None)); | ||
|
|
||
| let initialize_params = mk_init_params(root); | ||
| let res = initialize( | ||
| &mut state, | ||
| extension::CustomInitializeParams { initialize_params, work_done_token: None }, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!(res.capabilities.document_formatting_provider.is_none()); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn formatting_capability_advertised_with_flag() { | ||
| let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); | ||
| // Capability advertising doesn't execute the formatter; any non-empty value should enable it. | ||
| let mut state = mk_state_with(base_config(Some("purs-tidy".to_string()))); | ||
|
|
||
| let initialize_params = mk_init_params(root); | ||
| let res = initialize( | ||
| &mut state, | ||
| extension::CustomInitializeParams { initialize_params, work_done_token: None }, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!(res.capabilities.document_formatting_provider.is_some()); | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| #[test] | ||
| fn formatting_returns_full_document_edit() { | ||
| let mut state = mk_state_with(base_config(Some("tr a-z A-Z".to_string()))); | ||
|
|
||
| let uri = Url::parse("file:///test/Main.purs").unwrap(); | ||
| let input = "module Main where\nfoo = bar\n"; | ||
| on_change(&mut state, uri.as_str(), input).unwrap(); | ||
|
|
||
| let snapshot = StateSnapshot { | ||
| client: state.client.clone(), | ||
| config: Arc::clone(&state.config), | ||
| engine: state.engine.snapshot(), | ||
| files: Arc::clone(&state.files), | ||
| workspace_symbols_cache: Arc::clone(&state.workspace_symbols_cache), | ||
| suggestions_cache: Arc::clone(&state.suggestions_cache), | ||
| }; | ||
|
|
||
| let p = DocumentFormattingParams { | ||
| text_document: TextDocumentIdentifier { uri: uri.clone() }, | ||
| options: FormattingOptions { | ||
| tab_size: 2, | ||
| insert_spaces: true, | ||
| ..FormattingOptions::default() | ||
| }, | ||
| work_done_progress_params: WorkDoneProgressParams::default(), | ||
| }; | ||
|
|
||
| let edits = formatting(snapshot, p).unwrap().unwrap(); | ||
| assert_eq!(edits.len(), 1); | ||
| assert_eq!(edits[0].new_text, "MODULE MAIN WHERE\nFOO = BAR\n"); | ||
| assert_eq!(edits[0].range.start, Position::new(0, 0)); | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| #[test] | ||
| fn formatting_noop_returns_empty_edits() { | ||
| let mut state = mk_state_with(base_config(Some("cat".to_string()))); | ||
|
|
||
| let uri = Url::parse("file:///test/Main.purs").unwrap(); | ||
| let input = "module Main where\nfoo = bar\n"; | ||
| on_change(&mut state, uri.as_str(), input).unwrap(); | ||
|
|
||
| let snapshot = StateSnapshot { | ||
| client: state.client.clone(), | ||
| config: Arc::clone(&state.config), | ||
| engine: state.engine.snapshot(), | ||
| files: Arc::clone(&state.files), | ||
| workspace_symbols_cache: Arc::clone(&state.workspace_symbols_cache), | ||
| suggestions_cache: Arc::clone(&state.suggestions_cache), | ||
| }; | ||
|
|
||
| let p = DocumentFormattingParams { | ||
| text_document: TextDocumentIdentifier { uri }, | ||
| options: FormattingOptions::default(), | ||
| work_done_progress_params: WorkDoneProgressParams::default(), | ||
| }; | ||
|
|
||
| let edits = formatting(snapshot, p).unwrap().unwrap(); | ||
| assert!(edits.is_empty()); | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| #[test] | ||
| fn formatting_parses_quoted_args() { | ||
| let mut state = mk_state_with(base_config(Some("tr 'a-z' 'A-Z'".to_string()))); | ||
|
|
||
| let uri = Url::parse("file:///test/Main.purs").unwrap(); | ||
| let input = "module Main where\nfoo = bar\n"; | ||
| on_change(&mut state, uri.as_str(), input).unwrap(); | ||
|
|
||
| let snapshot = StateSnapshot { | ||
| client: state.client.clone(), | ||
| config: Arc::clone(&state.config), | ||
| engine: state.engine.snapshot(), | ||
| files: Arc::clone(&state.files), | ||
| workspace_symbols_cache: Arc::clone(&state.workspace_symbols_cache), | ||
| suggestions_cache: Arc::clone(&state.suggestions_cache), | ||
| }; | ||
|
|
||
| let p = DocumentFormattingParams { | ||
| text_document: TextDocumentIdentifier { uri }, | ||
| options: FormattingOptions::default(), | ||
| work_done_progress_params: WorkDoneProgressParams::default(), | ||
| }; | ||
|
|
||
| let edits = formatting(snapshot, p).unwrap().unwrap(); | ||
| assert_eq!(edits.len(), 1); | ||
| assert_eq!(edits[0].new_text, "MODULE MAIN WHERE\nFOO = BAR\n"); | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| #[test] | ||
| fn formatting_tolerates_outer_quoted_command_string() { | ||
| let mut state = mk_state_with(base_config(Some("\"tr a-z A-Z\"".to_string()))); | ||
|
|
||
| let uri = Url::parse("file:///test/Main.purs").unwrap(); | ||
| let input = "module Main where\nfoo = bar\n"; | ||
| on_change(&mut state, uri.as_str(), input).unwrap(); | ||
|
|
||
| let snapshot = StateSnapshot { | ||
| client: state.client.clone(), | ||
| config: Arc::clone(&state.config), | ||
| engine: state.engine.snapshot(), | ||
| files: Arc::clone(&state.files), | ||
| workspace_symbols_cache: Arc::clone(&state.workspace_symbols_cache), | ||
| suggestions_cache: Arc::clone(&state.suggestions_cache), | ||
| }; | ||
|
|
||
| let p = DocumentFormattingParams { | ||
| text_document: TextDocumentIdentifier { uri }, | ||
| options: FormattingOptions::default(), | ||
| work_done_progress_params: WorkDoneProgressParams::default(), | ||
| }; | ||
|
|
||
| let edits = formatting(snapshot, p).unwrap().unwrap(); | ||
| assert_eq!(edits.len(), 1); | ||
| assert_eq!(edits[0].new_text, "MODULE MAIN WHERE\nFOO = BAR\n"); | ||
| } |
Owner
There was a problem hiding this comment.
Could these be made part of tests-integration?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 83.35% 82.59% -0.76%
==========================================
Files 132 140 +8
Lines 24024 25173 +1149
Branches 24024 25173 +1149
==========================================
+ Hits 20025 20792 +767
- Misses 2183 2543 +360
- Partials 1816 1838 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bneiswander
force-pushed
the
lsp-formatting-provider
branch
from
44abe78 to
90abda5
Compare
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/command.rs`:
- Around line 84-86: cleanup_child currently calls child.wait() which can block
indefinitely; replace that with a bounded wait_timeout (e.g.,
Duration::from_secs(1)) and set ChildCleanup.wait_error based on the
wait_timeout outcome: if wait_timeout returns Ok(Some(status)) keep wait_error
as None, if Ok(None) treat it as a timeout and set wait_error to an appropriate
error, and if Err(e) set wait_error to that error. Update the function
cleanup_child and its usage of ChildCleanup so the kill_error still records
child.kill().err() and wait_error reflects the timed wait result (use
wait_timeout on the Child with ~1s).
🪄 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: f67ca95e-57ce-4a8f-94e2-fa6e726f9373
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
compiler-bin/Cargo.tomlcompiler-bin/src/cli.rscompiler-bin/src/lsp.rscompiler-bin/src/lsp/command.rscompiler-bin/src/lsp/error.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- compiler-bin/src/lsp/error.rs
- compiler-bin/Cargo.toml
- compiler-bin/src/cli.rs
- compiler-bin/src/lsp.rs
bneiswander
force-pushed
the
lsp-formatting-provider
branch
from
f657b51 to
f0dbb1f
Compare
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/formatting.rs`:
- Around line 355-365: The test exited_error_with_stdout_only_omits_newline uses
the shell command "sh" which is Unix-only; annotate the test with a Unix-only
cfg attribute (e.g. add #[cfg(unix)] above the fn
exited_error_with_stdout_only_omits_newline or use #[cfg(target_family =
"unix")]) so it only runs on Unix platforms; keep the test body unchanged and
ensure the attribute is placed immediately above the test function (and keep any
existing #[test] attribute alongside the cfg).
- Around line 26-27: When command::write_stdin(&mut child, input) returns an Err
and you construct FormattingError::WriteStdin, first ensure the spawned
formatter process (child) is reaped: attempt to kill the child (child.kill()) if
necessary and then wait on it (child.wait() or child.wait_with_output()) to
avoid leaving a zombie; perform these cleanup steps before returning the mapped
FormattingError (FormattingErrorKind::WriteStdin { source }). Make the cleanup
best-effort (ignore kill/wait errors but still return the original write error)
and keep this logic adjacent to the existing mapping so it executes when
write_stdin fails.
🪄 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: 363410e7-7483-47a9-be2a-11cf2dc0c042
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
compiler-bin/Cargo.tomlcompiler-bin/src/cli.rscompiler-bin/src/lsp.rscompiler-bin/src/lsp/command.rscompiler-bin/src/lsp/error.rscompiler-bin/src/lsp/formatting.rsjustfiletests-compatibility/src/lib.rstests-integration/Cargo.toml
✅ Files skipped from review due to trivial changes (3)
- tests-integration/Cargo.toml
- compiler-bin/Cargo.toml
- justfile
🚧 Files skipped from review as they are similar to previous changes (2)
- compiler-bin/src/lsp/error.rs
- compiler-bin/src/cli.rs
Treat empty format_command as disabled, add a timeout around formatter execution, and make formatting tests unix-only/portable.
Co-authored-by: Justin Garcia <purefunctor@gmail.com>
bneiswander
force-pushed
the
lsp-formatting-provider
branch
from
19a9cc8 to
6a53a87
Compare
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
2 participants
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.
Summary
This PR adds opt-in LSP document formatting support to
purescript-analyzer.textDocument/formattingby running an external formatter command over stdin/stdout.documentFormattingProvideronly when--format-commandis provided.--format-commandusing shell-style quoting (and tolerates an extra outer quoted string) to make editor configuration reliable.User-Facing Behavior
--format-command <cmd...>, the server will respond totextDocument/formattingrequests.Configuration
Examples:
purescript-analyzer --format-command "purs-tidy format"Quoted args are supported, for example:
purescript-analyzer --format-command "/usr/bin/tr 'a-z' 'A-Z'"If your editor passes an extra layer of quoting (for example
"purs-tidy format"as a single string), the server tolerates it.Implementation Notes
TextEditwhen the formatter output differs.Testing
cargo test -p purescript-analyzer --testsCovered scenarios: