diff --git a/Cargo.lock b/Cargo.lock index 6d68d2a..2013162 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1290,7 +1290,7 @@ dependencies = [ [[package]] name = "nufmt" -version = "0.1.1" +version = "0.1.2" dependencies = [ "clap", "criterion", diff --git a/Cargo.toml b/Cargo.toml index f1a4560..849d979 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "nufmt" -version = "0.1.1" +version = "0.1.2" edition = "2021" authors = ["The NuShell Contributors"] license = "MIT" diff --git a/src/formatting/calls.rs b/src/formatting/calls.rs index 4fa8ab9..2195d21 100644 --- a/src/formatting/calls.rs +++ b/src/formatting/calls.rs @@ -59,9 +59,19 @@ impl<'a> Formatter<'a> { return; } - // Write command name + // Write command name, normalizing multi-word command spacing if call.head.end != 0 { - self.write_span(call.head); + if let Some(ref head) = head_text { + // For multi-word commands, normalize spacing between words + if head.contains(' ') && head.split_whitespace().count() > 1 { + let normalized = head.split_whitespace().collect::>().join(" "); + self.write(&normalized); + } else { + self.write_span(call.head); + } + } else { + self.write_span(call.head); + } } if matches!(cmd_type, CommandType::Let) { @@ -167,9 +177,21 @@ impl<'a> Formatter<'a> { .filter(|arg| self.argument_belongs_to_call_source(call, arg)) .collect(); + let head_text = self.call_head_text(call); + self.write("("); if call.head.end != 0 { - self.write_span(call.head); + // Normalize multi-word command spacing + if let Some(ref head) = head_text { + if head.contains(' ') && head.split_whitespace().count() > 1 { + let normalized = head.split_whitespace().collect::>().join(" "); + self.write(&normalized); + } else { + self.write_span(call.head); + } + } else { + self.write_span(call.head); + } } self.newline(); self.indent_level += 1; @@ -1154,7 +1176,15 @@ impl<'a> Formatter<'a> { self.write(&rendered); } } - other => self.write(&other.to_string()), + other => { + let rendered = other.to_string(); + if rendered.contains("closure()") { + let normalized = rendered.replace("closure()", "closure"); + self.write(&normalized); + } else { + self.write(&rendered); + } + } } } } diff --git a/src/formatting/collections.rs b/src/formatting/collections.rs index e4ff8a1..ea25a05 100644 --- a/src/formatting/collections.rs +++ b/src/formatting/collections.rs @@ -382,8 +382,24 @@ impl<'a> Formatter<'a> { .map(|(pattern, expr)| self.render_match_arm_lhs(pattern, expr)) .collect(); - let should_align = self.should_preserve_match_arm_alignment(matches) + // Check if source has alignment preserved + let preserve_alignment = self.should_preserve_match_arm_alignment(matches) && rendered_lhs.iter().all(|lhs| !lhs.contains(&b'\n')); + + // Additionally check if all RHS expressions will format as single-line + // to ensure idempotency - don't align if any RHS becomes multiline + let all_rhs_single_line = if preserve_alignment { + matches.iter().all(|(_, expr)| { + let rendered = self.probe_format(|probe| { + probe.format_block_or_expr(expr); + }); + !rendered.contains(&b'\n') + }) + } else { + false + }; + + let should_align = preserve_alignment && all_rhs_single_line; let max_lhs_len = if should_align { rendered_lhs.iter().map(Vec::len).max().unwrap_or(0) } else { @@ -408,7 +424,11 @@ impl<'a> Formatter<'a> { } self.write("=> "); - self.format_block_or_expr(expr); + if self.should_force_multiline_match_arm_block(expr) { + self.format_match_arm_block_multiline(expr); + } else { + self.format_block_or_expr(expr); + } self.newline(); } @@ -417,6 +437,56 @@ impl<'a> Formatter<'a> { self.write("}"); } + /// Return `true` when a simple braced match-arm block would become + /// multiline once formatted, so we can emit multiline output in one pass. + fn should_force_multiline_match_arm_block(&self, expr: &Expression) -> bool { + let Expr::Block(block_id) = &expr.expr else { + return false; + }; + + let block = self.working_set.get_block(*block_id); + let source_has_newline = expr.span.end > expr.span.start + && self.source[expr.span.start..expr.span.end].contains(&b'\n'); + + let is_simple = block.pipelines.len() == 1 + && block.pipelines[0].elements.len() == 1 + && !self.block_has_nested_structures(block) + && !source_has_newline; + + if !is_simple { + return false; + } + + self.probe_format(|probe| probe.format_pipeline(&block.pipelines[0])) + .contains(&b'\n') + } + + /// Format a match-arm block body as multiline, preserving the same + /// conditional-depth reset semantics as regular block expressions. + fn format_match_arm_block_multiline(&mut self, expr: &Expression) { + let Expr::Block(block_id) = &expr.expr else { + self.format_block_or_expr(expr); + return; + }; + + let block = self.working_set.get_block(*block_id); + + self.write("{"); + + let saved_conditional_depth = self.conditional_context_depth; + self.conditional_context_depth = 0; + + self.newline(); + self.indent_level += 1; + self.format_block(block); + self.newline(); + self.indent_level -= 1; + self.write_indent(); + self.write("}"); + + self.conditional_context_depth = saved_conditional_depth; + } + /// Format a match pattern (value, variable, list, record, or, etc.). pub(super) fn format_match_pattern(&mut self, pattern: &MatchPattern) { match &pattern.pattern { diff --git a/tests/fixtures/expected/extraneous_spaces_in_multi_word_function_stripped_issue188.nu b/tests/fixtures/expected/extraneous_spaces_in_multi_word_function_stripped_issue188.nu new file mode 100644 index 0000000..7d1decd --- /dev/null +++ b/tests/fixtures/expected/extraneous_spaces_in_multi_word_function_stripped_issue188.nu @@ -0,0 +1 @@ +error make {msg: "hello"} diff --git a/tests/fixtures/expected/list_closure_type_annotation_issue187.nu b/tests/fixtures/expected/list_closure_type_annotation_issue187.nu new file mode 100644 index 0000000..4317628 --- /dev/null +++ b/tests/fixtures/expected/list_closure_type_annotation_issue187.nu @@ -0,0 +1 @@ +def par [jobs: list] { } diff --git a/tests/fixtures/expected/match_arm_formatting_is_idempotent_issue189.nu b/tests/fixtures/expected/match_arm_formatting_is_idempotent_issue189.nu new file mode 100644 index 0000000..f98a14f --- /dev/null +++ b/tests/fixtures/expected/match_arm_formatting_is_idempotent_issue189.nu @@ -0,0 +1,12 @@ +def foo [--editor: string] { + match $editor { + _ => { + error make { + msg: "unsupported editor" + label: { + span: (metadata $editor).span + } + } + } + } +} diff --git a/tests/fixtures/input/extraneous_spaces_in_multi_word_function_stripped_issue188.nu b/tests/fixtures/input/extraneous_spaces_in_multi_word_function_stripped_issue188.nu new file mode 100644 index 0000000..bd4f1f4 --- /dev/null +++ b/tests/fixtures/input/extraneous_spaces_in_multi_word_function_stripped_issue188.nu @@ -0,0 +1 @@ +error make {msg: "hello"} diff --git a/tests/fixtures/input/list_closure_type_annotation_issue187.nu b/tests/fixtures/input/list_closure_type_annotation_issue187.nu new file mode 100644 index 0000000..4317628 --- /dev/null +++ b/tests/fixtures/input/list_closure_type_annotation_issue187.nu @@ -0,0 +1 @@ +def par [jobs: list] { } diff --git a/tests/fixtures/input/match_arm_formatting_is_idempotent_issue189.nu b/tests/fixtures/input/match_arm_formatting_is_idempotent_issue189.nu new file mode 100644 index 0000000..bfdc671 --- /dev/null +++ b/tests/fixtures/input/match_arm_formatting_is_idempotent_issue189.nu @@ -0,0 +1,5 @@ +def foo [--editor: string] { + match $editor { + _ => {error make {msg: "unsupported editor", label: {span: (metadata $editor).span}}} + } +} diff --git a/tests/ground_truth.rs b/tests/ground_truth.rs index 94e91a1..a1b38de 100644 --- a/tests/ground_truth.rs +++ b/tests/ground_truth.rs @@ -684,4 +684,19 @@ fixture_tests!( ground_truth_stderr_pipeline_redirection_pipe_not_duplicated_issue181, idempotency_stderr_pipeline_redirection_pipe_not_duplicated_issue181 ), + ( + "list_closure_type_annotation_issue187", + ground_truth_list_closure_type_annotation_issue187, + idempotency_list_closure_type_annotation_issue187 + ), + ( + "extraneous_spaces_in_multi_word_function_stripped_issue188", + ground_truth_extraneous_spaces_in_multi_word_function_stripped_issue188, + idempotency_extraneous_spaces_in_multi_word_function_stripped_issue188 + ), + ( + "match_arm_formatting_is_idempotent_issue189", + ground_truth_match_arm_formatting_is_idempotent_issue189, + idempotency_match_arm_formatting_is_idempotent_issue189 + ), ); diff --git a/tests/main.rs b/tests/main.rs index 6c15d2e..af3d275 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -10,14 +10,30 @@ const VALID: &str = "# beginning of script comment let one = 1 "; -fn run_stdin(input: &str) -> std::process::Output { - let mut child = Command::new(get_test_binary()) +/// Filter parser diagnostic noise from stderr that appears when `RUST_LOG` is set. +/// Removes lines that contain parser diagnostic messages from the `nu_parser` module. +fn filter_parser_diagnostics(stderr: &str) -> String { + stderr + .lines() + .filter(|line| !line.contains("nu_parser::")) + .collect::>() + .join("\n") +} + +/// Run `nufmt --stdin` with optional environment overrides. +fn run_stdin_with_env(input: &str, env: &[(&str, &str)]) -> std::process::Output { + let mut command = Command::new(get_test_binary()); + command .arg("--stdin") .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - .expect("Failed to spawn nufmt"); + .stderr(std::process::Stdio::piped()); + + for (key, value) in env { + command.env(key, value); + } + + let mut child = command.spawn().expect("Failed to spawn nufmt"); child .stdin @@ -29,6 +45,28 @@ fn run_stdin(input: &str) -> std::process::Output { child.wait_with_output().expect("Failed to wait for nufmt") } +/// Run `nufmt --stdin` with default environment. +fn run_stdin(input: &str) -> std::process::Output { + run_stdin_with_env(input, &[]) +} + +/// Run `nufmt --stdin` with `RUST_LOG=error` enabled. +fn run_stdin_with_rust_log_error(input: &str) -> std::process::Output { + run_stdin_with_env(input, &[("RUST_LOG", "error")]) +} + +/// Assert that a formatter run completed successfully without parser noise. +fn assert_no_parser_error_noise(output: &std::process::Output) { + let stderr = String::from_utf8_lossy(&output.stderr); + let cleaned_stderr = filter_parser_diagnostics(&stderr); + + assert_eq!(output.status.code(), Some(0)); + assert!( + !cleaned_stderr.contains("compile_block_with_id called with parse errors"), + "unexpected parser error noise on stderr: {cleaned_stderr}" + ); +} + #[test] fn failure_with_invalid_config() { let dir = tempdir().unwrap(); @@ -315,25 +353,19 @@ fn format_fixtures_basic() { #[test] fn mixed_use_and_def_does_not_emit_parser_errors_issue136() { let output = run_stdin("use a.nu\ndef abc [] { }\ndef xyz [] { }\n"); - let stderr = String::from_utf8_lossy(&output.stderr); - - assert_eq!(output.status.code(), Some(0)); - assert!( - !stderr.contains("compile_block_with_id called with parse errors"), - "unexpected parser error noise on stderr: {stderr}" - ); + assert_no_parser_error_noise(&output); } #[test] fn cell_path_in_def_block_does_not_emit_parser_errors_issue141() { let output = run_stdin("def main [] {\n$var.state\n}\n"); - let stderr = String::from_utf8_lossy(&output.stderr); + assert_no_parser_error_noise(&output); +} - assert_eq!(output.status.code(), Some(0)); - assert!( - !stderr.contains("compile_block_with_id called with parse errors"), - "unexpected parser error noise on stderr: {stderr}" - ); +#[test] +fn cli_tests_do_not_fail_when_rust_log_error_is_set_issue190() { + let output = run_stdin_with_rust_log_error("use a.nu\ndef abc [] { }\ndef xyz [] { }\n"); + assert_no_parser_error_noise(&output); } #[test]