diff --git a/src/formatting.rs b/src/formatting.rs index 26617c3..e75fcae 100644 --- a/src/formatting.rs +++ b/src/formatting.rs @@ -21,7 +21,7 @@ use nu_utils::NuCow; const BLOCK_COMMANDS: &[&str] = &["for", "while", "loop", "module"]; const CONDITIONAL_COMMANDS: &[&str] = &["if", "try"]; const DEF_COMMANDS: &[&str] = &["def", "def-env", "export def"]; -const EXTERN_COMMANDS: &[&str] = &["extern"]; +const EXTERN_COMMANDS: &[&str] = &["extern", "export extern"]; const LET_COMMANDS: &[&str] = &["let", "let-env", "mut", "const"]; /// Get the default engine state with built-in commands @@ -49,6 +49,8 @@ struct Formatter<'a> { written_comments: Vec, /// Current position in source being processed last_pos: usize, + /// Track nested conditional argument formatting to preserve explicit parens + conditional_context_depth: usize, } impl<'a> Formatter<'a> { @@ -65,6 +67,7 @@ impl<'a> Formatter<'a> { comments, written_comments, last_pos: 0, + conditional_context_depth: 0, } } @@ -208,7 +211,15 @@ impl<'a> Formatter<'a> { } if i < num_pipelines - 1 { - self.newline(); + let separator_newlines = if self.indent_level == 0 && self.config.margin > 1 { + self.config.margin.saturating_add(1) + } else { + 1 + }; + + for _ in 0..separator_newlines { + self.newline(); + } } } } @@ -393,12 +404,79 @@ impl<'a> Formatter<'a> { self.write_span(call.head); } + if matches!(cmd_type, CommandType::Let) { + self.format_let_call(call); + return; + } + // Format arguments based on command type for arg in &call.arguments { self.format_call_argument(arg, &cmd_type); } } + /// Format let/mut/const calls while preserving explicit type annotations. + fn format_let_call(&mut self, call: &nu_protocol::ast::Call) { + let positional: Vec<&Expression> = call + .arguments + .iter() + .filter_map(|arg| match arg { + Argument::Positional(expr) | Argument::Unknown(expr) => Some(expr), + _ => None, + }) + .collect(); + + if positional.is_empty() { + for arg in &call.arguments { + self.format_call_argument(arg, &CommandType::Let); + } + return; + } + + self.space(); + self.format_expression(positional[0]); + + if let Some(rhs) = positional.get(1) { + let lhs = positional[0]; + let between = if lhs.span.end <= rhs.span.start { + &self.source[lhs.span.end..rhs.span.start] + } else { + &[] + }; + + if let Some(eq_pos) = between.iter().position(|b| *b == b'=') { + let annotation = between[..eq_pos].trim_ascii(); + if !annotation.is_empty() { + if !annotation.starts_with(b":") { + self.space(); + } + self.write_bytes(annotation); + } + } + + self.write(" = "); + + match &rhs.expr { + Expr::Block(block_id) | Expr::Subexpression(block_id) => { + let block = self.working_set.get_block(*block_id); + self.format_block(block); + } + _ => self.format_expression(rhs), + } + + for extra in positional.iter().skip(2) { + self.space(); + self.format_expression(extra); + } + } + + for arg in &call.arguments { + if !matches!(arg, Argument::Positional(_) | Argument::Unknown(_)) { + self.format_call_argument(arg, &CommandType::Let); + } + } + } + /// Classify a command by its formatting requirements fn classify_command(name: &str) -> CommandType { if DEF_COMMANDS.contains(&name) { @@ -431,7 +509,18 @@ impl<'a> Formatter<'a> { self.write_span(short.span); } if let Some(value) = &named.2 { - self.space(); + let separator_start = named + .1 + .as_ref() + .map_or(named.0.span.end, |short| short.span.end); + let has_equals = separator_start <= value.span.start + && self.source[separator_start..value.span.start].contains(&b'='); + + if has_equals { + self.write("="); + } else { + self.space(); + } self.format_expression(value); } } @@ -449,7 +538,12 @@ impl<'a> Formatter<'a> { match cmd_type { CommandType::Def => self.format_def_argument(positional), CommandType::Extern => self.format_extern_argument(positional), - CommandType::Conditional | CommandType::Block => { + CommandType::Conditional => { + self.conditional_context_depth += 1; + self.format_block_or_expr(positional); + self.conditional_context_depth -= 1; + } + CommandType::Block => { self.format_block_or_expr(positional); } CommandType::Let => self.format_let_argument(positional), @@ -628,6 +722,14 @@ impl<'a> Formatter<'a> { fn write_shape(&mut self, shape: &SyntaxShape) { match shape { SyntaxShape::Closure(Option::None) => self.write("closure"), + SyntaxShape::Closure(_) => { + let rendered = shape.to_string(); + if rendered == "closure()" { + self.write("closure"); + } else { + self.write(&rendered); + } + } _ => self.write(&format!("{}", shape)), } } @@ -744,6 +846,18 @@ impl<'a> Formatter<'a> { self.write_indent(); } self.write("]"); + + if !sig.input_output_types.is_empty() { + self.write(": "); + for (i, (input, output)) in sig.input_output_types.iter().enumerate() { + if i > 0 { + self.write(", "); + } + self.write(&input.to_string()); + self.write(" -> "); + self.write(&output.to_string()); + } + } } /// Format a `CellPath` @@ -770,16 +884,16 @@ impl<'a> Formatter<'a> { fn format_cell_path_member(&mut self, member: &PathMember) { match member { PathMember::String { val, optional, .. } => { + self.write(val); if *optional { self.write("?"); } - self.write(val); } PathMember::Int { val, optional, .. } => { + self.write(&val.to_string()); if *optional { self.write("?"); } - self.write(&val.to_string()); } } } @@ -794,6 +908,14 @@ impl<'a> Formatter<'a> { self.format_block(block); return; } + + if self.conditional_context_depth > 0 { + self.write("("); + self.format_block(block); + self.write(")"); + return; + } + // Special case: subexpressions containing only a string interpolation don't need parentheses if block.pipelines.len() == 1 && block.pipelines[0].elements.len() == 1 { if let Expr::StringInterpolation(_) = &block.pipelines[0].elements[0].expr.expr { @@ -837,18 +959,24 @@ impl<'a> Formatter<'a> { fn format_block_expression( &mut self, block_id: nu_protocol::BlockId, - _span: Span, + span: Span, with_braces: bool, ) { let block = self.working_set.get_block(block_id); + let source_has_newline = with_braces + && self.conditional_context_depth > 0 + && span.end > span.start + && self.source[span.start..span.end].contains(&b'\n'); + if with_braces { self.write("{"); } let is_simple = block.pipelines.len() == 1 && block.pipelines[0].elements.len() == 1 - && !self.block_has_nested_structures(block); + && !self.block_has_nested_structures(block) + && !source_has_newline; if is_simple && with_braces { self.write(" "); @@ -1486,4 +1614,15 @@ mod tests { let second = format(&first); assert_eq!(first, second, "Formatting should be idempotent"); } + + #[test] + fn issue98_margin_has_effect() { + let input = "def foo [] {\n let out = 1\n out\n}\n\ndef bar [] {\n let out = 1\n out\n}"; + let config = Config::new(4, 80, 2); + let result = format_inner(input.as_bytes(), &config).expect("formatting failed"); + let output = String::from_utf8(result).expect("invalid utf8"); + + let expected = "def foo [] {\n let out = 1\n out\n}\n\n\ndef bar [] {\n let out = 1\n out\n}"; + assert_eq!(output, expected); + } } diff --git a/tests/fixtures/expected/issue85.nu b/tests/fixtures/expected/issue85.nu new file mode 100644 index 0000000..4d81341 --- /dev/null +++ b/tests/fixtures/expected/issue85.nu @@ -0,0 +1 @@ +$env.FOO? | default 'foo' diff --git a/tests/fixtures/expected/issue86.nu b/tests/fixtures/expected/issue86.nu new file mode 100644 index 0000000..1b88dda --- /dev/null +++ b/tests/fixtures/expected/issue86.nu @@ -0,0 +1 @@ +def foo [cls: closure] { } diff --git a/tests/fixtures/expected/issue87.nu b/tests/fixtures/expected/issue87.nu new file mode 100644 index 0000000..b45d98a --- /dev/null +++ b/tests/fixtures/expected/issue87.nu @@ -0,0 +1,6 @@ +def "nu-complete cargo packages" [] { [] } +export extern "cargo build" [ + --package(-p): string@"nu-complete cargo packages" + -h + --help +] diff --git a/tests/fixtures/expected/issue92.nu b/tests/fixtures/expected/issue92.nu new file mode 100644 index 0000000..cd82a46 --- /dev/null +++ b/tests/fixtures/expected/issue92.nu @@ -0,0 +1 @@ +def a []: nothing -> nothing { } diff --git a/tests/fixtures/expected/issue93.nu b/tests/fixtures/expected/issue93.nu new file mode 100644 index 0000000..e3cb6e4 --- /dev/null +++ b/tests/fixtures/expected/issue93.nu @@ -0,0 +1,5 @@ +let result = $items | from json | if ($in | default [] | where value == "ERR" | is-empty) { + $in +} else { + null +} diff --git a/tests/fixtures/expected/issue94.nu b/tests/fixtures/expected/issue94.nu new file mode 100644 index 0000000..3235cd2 --- /dev/null +++ b/tests/fixtures/expected/issue94.nu @@ -0,0 +1,4 @@ +let name: string = "hello" +let count: int = 42 +mut results: list = [] +let flag: bool = true diff --git a/tests/fixtures/expected/issue95.nu b/tests/fixtures/expected/issue95.nu new file mode 100644 index 0000000..0f12bec --- /dev/null +++ b/tests/fixtures/expected/issue95.nu @@ -0,0 +1,5 @@ +def do-thing [--check] { $check } +def run [--check] { + let c: bool = $check + do-thing --check=($c) +} diff --git a/tests/fixtures/expected/issue97.nu b/tests/fixtures/expected/issue97.nu new file mode 100644 index 0000000..e749534 --- /dev/null +++ b/tests/fixtures/expected/issue97.nu @@ -0,0 +1,4 @@ +def start_zellij [] { + if ($env.ZELLIJ_AUTO_ATTACH? | default "false") == "true" { zellij attach -c } + if ($env.ZELLIJ_AUTO_EXIT? | default "false") == "true" { exit } +} diff --git a/tests/fixtures/input/issue85.nu b/tests/fixtures/input/issue85.nu new file mode 100644 index 0000000..4d81341 --- /dev/null +++ b/tests/fixtures/input/issue85.nu @@ -0,0 +1 @@ +$env.FOO? | default 'foo' diff --git a/tests/fixtures/input/issue86.nu b/tests/fixtures/input/issue86.nu new file mode 100644 index 0000000..1b88dda --- /dev/null +++ b/tests/fixtures/input/issue86.nu @@ -0,0 +1 @@ +def foo [cls: closure] { } diff --git a/tests/fixtures/input/issue87.nu b/tests/fixtures/input/issue87.nu new file mode 100644 index 0000000..b45d98a --- /dev/null +++ b/tests/fixtures/input/issue87.nu @@ -0,0 +1,6 @@ +def "nu-complete cargo packages" [] { [] } +export extern "cargo build" [ + --package(-p): string@"nu-complete cargo packages" + -h + --help +] diff --git a/tests/fixtures/input/issue92.nu b/tests/fixtures/input/issue92.nu new file mode 100644 index 0000000..cd82a46 --- /dev/null +++ b/tests/fixtures/input/issue92.nu @@ -0,0 +1 @@ +def a []: nothing -> nothing { } diff --git a/tests/fixtures/input/issue93.nu b/tests/fixtures/input/issue93.nu new file mode 100644 index 0000000..e3cb6e4 --- /dev/null +++ b/tests/fixtures/input/issue93.nu @@ -0,0 +1,5 @@ +let result = $items | from json | if ($in | default [] | where value == "ERR" | is-empty) { + $in +} else { + null +} diff --git a/tests/fixtures/input/issue94.nu b/tests/fixtures/input/issue94.nu new file mode 100644 index 0000000..3235cd2 --- /dev/null +++ b/tests/fixtures/input/issue94.nu @@ -0,0 +1,4 @@ +let name: string = "hello" +let count: int = 42 +mut results: list = [] +let flag: bool = true diff --git a/tests/fixtures/input/issue95.nu b/tests/fixtures/input/issue95.nu new file mode 100644 index 0000000..0f12bec --- /dev/null +++ b/tests/fixtures/input/issue95.nu @@ -0,0 +1,5 @@ +def do-thing [--check] { $check } +def run [--check] { + let c: bool = $check + do-thing --check=($c) +} diff --git a/tests/fixtures/input/issue97.nu b/tests/fixtures/input/issue97.nu new file mode 100644 index 0000000..e749534 --- /dev/null +++ b/tests/fixtures/input/issue97.nu @@ -0,0 +1,4 @@ +def start_zellij [] { + if ($env.ZELLIJ_AUTO_ATTACH? | default "false") == "true" { zellij attach -c } + if ($env.ZELLIJ_AUTO_EXIT? | default "false") == "true" { exit } +} diff --git a/tests/ground_truth.rs b/tests/ground_truth.rs index 5e6054f..300ed0b 100644 --- a/tests/ground_truth.rs +++ b/tests/ground_truth.rs @@ -778,3 +778,32 @@ fn idempotency_inline_param_comment_issue77() { let test_binary = get_test_binary(); run_idempotency_test(&test_binary, "inline_param_comment"); } + +macro_rules! issue_fixture_tests { + ($(($fixture:literal, $ground_truth_test:ident, $idempotency_test:ident)),+ $(,)?) => { + $( + #[test] + fn $ground_truth_test() { + let test_binary = get_test_binary(); + run_ground_truth_test(&test_binary, $fixture); + } + + #[test] + fn $idempotency_test() { + let test_binary = get_test_binary(); + run_idempotency_test(&test_binary, $fixture); + } + )+ + }; +} + +issue_fixture_tests!( + ("issue85", issue85_test, idempotency_issue85_test), + ("issue86", issue86_test, idempotency_issue86_test), + ("issue87", issue87_test, idempotency_issue87_test), + ("issue92", issue92_test, idempotency_issue92_test), + ("issue93", issue93_test, idempotency_issue93_test), + ("issue94", issue94_test, idempotency_issue94_test), + ("issue95", issue95_test, idempotency_issue95_test), + ("issue97", issue97_test, idempotency_issue97_test), +);