From 75feeda7e1aab040c1a7676088defbd2fb075fa6 Mon Sep 17 00:00:00 2001
From: Darren Schroeder <343840+fdncred@users.noreply.github.com>
Date: Thu, 26 Mar 2026 16:33:39 -0500
Subject: [PATCH] Enhance formatting capabilities for pipelines, comments, and
 `for` loop calls; add new test cases for improved coverage

---
 src/formatting/blocks.rs              | 42 ++++++++++++++++-
 src/formatting/calls.rs               | 68 +++++++++++++++++++++++++++
 src/formatting/collections.rs         | 10 +++-
 src/formatting/comments.rs            | 39 ++++++++++++---
 src/formatting/expressions.rs         | 25 +++++++++-
 src/formatting/mod.rs                 | 15 ++++--
 tests/fixtures/expected/error_make.nu |  5 +-
 tests/fixtures/expected/issue128.nu   |  4 ++
 tests/fixtures/expected/issue129.nu   |  3 ++
 tests/fixtures/expected/issue130.nu   |  2 +
 tests/fixtures/expected/issue131.nu   |  3 ++
 tests/fixtures/expected/issue132.nu   |  3 ++
 tests/fixtures/expected/issue133.nu   |  5 ++
 tests/fixtures/expected/issue134.nu   |  6 +++
 tests/fixtures/input/issue128.nu      |  4 ++
 tests/fixtures/input/issue129.nu      |  3 ++
 tests/fixtures/input/issue130.nu      |  2 +
 tests/fixtures/input/issue131.nu      |  3 ++
 tests/fixtures/input/issue132.nu      |  3 ++
 tests/fixtures/input/issue133.nu      |  5 ++
 tests/fixtures/input/issue134.nu      |  6 +++
 tests/ground_truth.rs                 |  7 +++
 22 files changed, 245 insertions(+), 18 deletions(-)
 create mode 100644 tests/fixtures/expected/issue128.nu
 create mode 100644 tests/fixtures/expected/issue129.nu
 create mode 100644 tests/fixtures/expected/issue130.nu
 create mode 100644 tests/fixtures/expected/issue131.nu
 create mode 100644 tests/fixtures/expected/issue132.nu
 create mode 100644 tests/fixtures/expected/issue133.nu
 create mode 100644 tests/fixtures/expected/issue134.nu
 create mode 100644 tests/fixtures/input/issue128.nu
 create mode 100644 tests/fixtures/input/issue129.nu
 create mode 100644 tests/fixtures/input/issue130.nu
 create mode 100644 tests/fixtures/input/issue131.nu
 create mode 100644 tests/fixtures/input/issue132.nu
 create mode 100644 tests/fixtures/input/issue133.nu
 create mode 100644 tests/fixtures/input/issue134.nu

diff --git a/src/formatting/blocks.rs b/src/formatting/blocks.rs
index 28cd570..3f49cb5 100644
--- a/src/formatting/blocks.rs
+++ b/src/formatting/blocks.rs
@@ -24,11 +24,17 @@ impl<'a> Formatter<'a> {
                 self.write_comments_before(first_elem.expr.span.start);
             }
 
+            // Pipelines inside indented blocks (e.g. def bodies) need
+            // explicit indentation when they start on a fresh line.
+            if self.at_line_start {
+                self.write_indent();
+            }
+
             self.format_pipeline(pipeline);
 
             if let Some(last_elem) = pipeline.elements.last() {
                 let end_pos = self.get_element_end_pos(last_elem);
-                self.write_inline_comment(end_pos);
+                self.write_inline_comment_bounded(end_pos, self.inline_comment_upper_bound);
                 self.last_pos = end_pos;
             }
 
@@ -58,10 +64,34 @@ impl<'a> Formatter<'a> {
     }
 
     /// Format a pipeline (elements joined by `|`).
+    ///
+    /// Preserves multi-line pipeline layout: if the original source has line
+    /// breaks between pipeline elements, the formatted output keeps each
+    /// stage on its own line with `| ` prefix.
     pub(super) fn format_pipeline(&mut self, pipeline: &nu_protocol::ast::Pipeline) {
+        if pipeline.elements.len() <= 1 {
+            if let Some(element) = pipeline.elements.first() {
+                self.format_pipeline_element(element);
+            }
+            return;
+        }
+
+        // Detect whether the source places pipeline elements on separate lines.
+        let is_multiline = pipeline.elements.windows(2).any(|pair| {
+            let prev_end = self.get_element_end_pos(&pair[0]);
+            let next_start = pair[1].expr.span.start;
+            prev_end < next_start && self.source[prev_end..next_start].contains(&b'\n')
+        });
+
         for (i, element) in pipeline.elements.iter().enumerate() {
             if i > 0 {
-                self.write(" | ");
+                if is_multiline {
+                    self.newline();
+                    self.write_indent();
+                    self.write("| ");
+                } else {
+                    self.write(" | ");
+                }
             }
             self.format_pipeline_element(element);
         }
@@ -133,6 +163,12 @@ impl<'a> Formatter<'a> {
             self.write("{");
         }
 
+        // Reset conditional_context_depth inside block bodies so that
+        // subexpressions nested inside `if` / `try` blocks keep their
+        // explicit parentheses (fixes issue #131).
+        let saved_conditional_depth = self.conditional_context_depth;
+        self.conditional_context_depth = 0;
+
         let is_simple = block.pipelines.len() == 1
             && block.pipelines[0].elements.len() == 1
             && !self.block_has_nested_structures(block)
@@ -164,6 +200,8 @@ impl<'a> Formatter<'a> {
         if with_braces {
             self.write("}");
         }
+
+        self.conditional_context_depth = saved_conditional_depth;
     }
 
     /// Detect whether a braced block looks like a compact record literal
diff --git a/src/formatting/calls.rs b/src/formatting/calls.rs
index 8ba5e3c..496fbb9 100644
--- a/src/formatting/calls.rs
+++ b/src/formatting/calls.rs
@@ -42,6 +42,11 @@ impl<'a> Formatter<'a> {
             return;
         }
 
+        if decl_name == "for" {
+            self.format_for_call(call);
+            return;
+        }
+
         for arg in &call.arguments {
             self.format_call_argument(arg, &cmd_type);
         }
@@ -110,6 +115,69 @@ impl<'a> Formatter<'a> {
         }
     }
 
+    /// Format `for` loop calls, preserving explicit type annotations on the
+    /// loop variable (e.g. `for h: int in [1 2 3] { ... }`).
+    pub(super) fn format_for_call(&mut self, call: &nu_protocol::ast::Call) {
+        // Find the VarDecl positional and the Keyword("in") argument.
+        let var_decl = call.arguments.iter().find_map(|arg| match arg {
+            Argument::Positional(expr) | Argument::Unknown(expr)
+                if matches!(expr.expr, Expr::VarDecl(_)) =>
+            {
+                Some(expr)
+            }
+            _ => None,
+        });
+
+        let keyword_in = call.arguments.iter().find_map(|arg| match arg {
+            Argument::Positional(expr) | Argument::Unknown(expr)
+                if matches!(expr.expr, Expr::Keyword(_)) =>
+            {
+                Some(expr)
+            }
+            _ => None,
+        });
+
+        if let (Some(var_decl), Some(kw_in)) = (var_decl, keyword_in) {
+            self.space();
+            self.format_expression(var_decl);
+
+            // Preserve a type annotation (e.g. `: int`) between the loop
+            // variable and the `in` keyword.
+            if var_decl.span.end < kw_in.span.start {
+                let between = &self.source[var_decl.span.end..kw_in.span.start];
+                let annotation = between.trim_ascii();
+                if annotation.starts_with(b":") {
+                    self.write_bytes(annotation);
+                }
+            }
+
+            self.space();
+            self.format_expression(kw_in);
+        } else {
+            // Fallback — format all arguments normally
+            for arg in &call.arguments {
+                self.format_call_argument(arg, &CommandType::Block);
+            }
+            return;
+        }
+
+        // Format remaining arguments (the body block)
+        for arg in &call.arguments {
+            match arg {
+                Argument::Positional(expr) | Argument::Unknown(expr) => {
+                    if matches!(expr.expr, Expr::VarDecl(_) | Expr::Keyword(_)) {
+                        continue;
+                    }
+                    self.space();
+                    self.format_block_or_expr(expr);
+                }
+                _ => {
+                    self.format_call_argument(arg, &CommandType::Block);
+                }
+            }
+        }
+    }
+
     /// Classify a command name into a [`CommandType`] for formatting purposes.
     pub(super) fn classify_command(name: &str) -> CommandType {
         if DEF_COMMANDS.contains(&name) {
diff --git a/src/formatting/collections.rs b/src/formatting/collections.rs
index ed8e5e2..03a2a20 100644
--- a/src/formatting/collections.rs
+++ b/src/formatting/collections.rs
@@ -113,8 +113,8 @@ impl<'a> Formatter<'a> {
                     | Expr::List(_)
                     | Expr::Closure(_)
                     | Expr::Block(_)
-                    | Expr::Var(_)
-                    | Expr::FullCellPath(_)
+                    | Expr::StringInterpolation(_)
+                    | Expr::Subexpression(_)
             ),
             RecordItem::Spread(_, _) => false,
         });
@@ -153,6 +153,12 @@ impl<'a> Formatter<'a> {
             for item in items {
                 self.write_indent();
                 self.format_record_item(item, preserve_compact);
+                // Capture inline comments on the same line as the record item.
+                let item_end = match item {
+                    RecordItem::Pair(_, v) => v.span.end,
+                    RecordItem::Spread(_, expr) => expr.span.end,
+                };
+                self.write_inline_comment_bounded(item_end, None);
                 self.newline();
             }
             self.indent_level -= 1;
diff --git a/src/formatting/comments.rs b/src/formatting/comments.rs
index e1bfbe1..68c69a6 100644
--- a/src/formatting/comments.rs
+++ b/src/formatting/comments.rs
@@ -75,8 +75,26 @@ impl<'a> Formatter<'a> {
 
         comments_to_write.sort_by_key(|(_, start, _)| *start);
 
-        for (idx, _, content) in comments_to_write {
-            self.written_comments[idx] = true;
+        let mut prev_comment_end: Option<usize> = None;
+        for (idx, start, content) in &comments_to_write {
+            self.written_comments[*idx] = true;
+
+            // Preserve blank lines between comment groups by checking
+            // whether the original source has a blank line between the
+            // previous comment and this one.
+            if let Some(prev_end) = prev_comment_end {
+                let between = &self.source[prev_end..*start];
+                let newline_count = between.iter().filter(|&&b| b == b'\n').count();
+                if newline_count >= 2 {
+                    // There was at least one blank line in the source
+                    // between the two comments — emit one now.
+                    if !self.at_line_start {
+                        self.newline();
+                    }
+                    self.newline();
+                }
+            }
+
             if !self.at_line_start {
                 if let Some(&last) = self.output.last() {
                     if last != b'\n' {
@@ -85,24 +103,33 @@ impl<'a> Formatter<'a> {
                 }
             }
             self.write_indent();
-            self.output.extend(&content);
+            self.output.extend(content);
             self.newline();
+
+            prev_comment_end = Some(start + content.len());
         }
     }
 
-    /// Emit an inline comment (on the same line) that appears after `after_pos`.
-    pub(super) fn write_inline_comment(&mut self, after_pos: usize) {
+    /// Emit an inline comment (on the same line) that appears after `after_pos`,
+    /// optionally bounded by an upper position limit.
+    ///
+    /// When `upper` is `Some(pos)`, comments starting at or after `pos` are
+    /// ignored. This prevents capturing comments that belong outside a
+    /// surrounding delimiter (e.g. after a closing parenthesis).
+    pub(super) fn write_inline_comment_bounded(&mut self, after_pos: usize, upper: Option<usize>) {
         let line_end = self.source[after_pos..]
             .iter()
             .position(|&b| b == b'\n')
             .map_or(self.source.len(), |p| after_pos + p);
 
+        let effective_end = upper.map_or(line_end, |u| u.min(line_end));
+
         let found = self
             .comments
             .iter()
             .enumerate()
             .find(|(i, (span, _))| {
-                !self.written_comments[*i] && span.start >= after_pos && span.start < line_end
+                !self.written_comments[*i] && span.start >= after_pos && span.start < effective_end
             })
             .map(|(i, (span, content))| (i, *span, content.clone()));
 
diff --git a/src/formatting/expressions.rs b/src/formatting/expressions.rs
index 1a75f68..f5b9685 100644
--- a/src/formatting/expressions.rs
+++ b/src/formatting/expressions.rs
@@ -24,7 +24,6 @@ impl<'a> Formatter<'a> {
             | Expr::Binary(_)
             | Expr::Filepath(_, _)
             | Expr::Directory(_, _)
-            | Expr::GlobPattern(_, _)
             | Expr::Var(_)
             | Expr::VarDecl(_)
             | Expr::Operator(_)
@@ -36,6 +35,22 @@ impl<'a> Formatter<'a> {
                 self.write_expr_span(expr);
             }
 
+            // Glob patterns — normalise empty-brace globs like `{ }` to `{}`
+            // (the parser treats `{ }` in unknown-command args as a glob).
+            Expr::GlobPattern(_, _) => {
+                let content = &self.source[expr.span.start..expr.span.end];
+                if content.starts_with(b"{")
+                    && content.ends_with(b"}")
+                    && content[1..content.len() - 1]
+                        .iter()
+                        .all(|b| b.is_ascii_whitespace())
+                {
+                    self.write("{}");
+                } else {
+                    self.write_expr_span(expr);
+                }
+            }
+
             Expr::Signature(sig) => {
                 if self.has_comments_in_span(expr.span.start, expr.span.end) {
                     self.write_expr_span(expr);
@@ -253,6 +268,12 @@ impl<'a> Formatter<'a> {
             }
         }
 
+        // Set inline comment boundary at the closing paren so that comments
+        // appearing after `)` on the same line are not captured inside (issue #133).
+        let saved_bound = self.inline_comment_upper_bound;
+        let closing_paren_pos = span.end.saturating_sub(1);
+        self.inline_comment_upper_bound = Some(closing_paren_pos);
+
         self.write("(");
         let is_simple = !source_has_newline
             && block.pipelines.len() == 1
@@ -269,6 +290,8 @@ impl<'a> Formatter<'a> {
             self.write_indent();
         }
         self.write(")");
+
+        self.inline_comment_upper_bound = saved_bound;
     }
 
     /// Format an expression that could be a block or a regular expression.
diff --git a/src/formatting/mod.rs b/src/formatting/mod.rs
index ec96c67..0f80839 100644
--- a/src/formatting/mod.rs
+++ b/src/formatting/mod.rs
@@ -66,6 +66,9 @@ pub(crate) struct Formatter<'a> {
     pub(crate) preserve_subexpr_parens_depth: usize,
     /// Allow compact inline record style used for repaired malformed records.
     pub(crate) allow_compact_recovered_record_style: bool,
+    /// Optional upper boundary for inline comment capture inside
+    /// delimited contexts (e.g. subexpressions).
+    pub(crate) inline_comment_upper_bound: Option<usize>,
 }
 
 /// Command types for formatting purposes.
@@ -101,6 +104,7 @@ impl<'a> Formatter<'a> {
             conditional_context_depth: 0,
             preserve_subexpr_parens_depth: 0,
             allow_compact_recovered_record_style,
+            inline_comment_upper_bound: None,
         }
     }
 
@@ -151,15 +155,20 @@ impl<'a> Formatter<'a> {
     // Span and source helpers
     // ─────────────────────────────────────────────────────────────────────────
 
-    /// Get the source content for a span.
+    /// Copy the source bytes for a span into a new `Vec`.
+    ///
+    /// Returns owned data so that callers can slice into it while still
+    /// holding `&mut self` for output writes (splitting borrows on `self`
+    /// through a method return is not possible with a `&[u8]` reference).
     pub(crate) fn get_span_content(&self, span: Span) -> Vec<u8> {
         self.source[span.start..span.end].to_vec()
     }
 
     /// Write the original source content for a span.
     pub(crate) fn write_span(&mut self, span: Span) {
-        let content = self.source[span.start..span.end].to_vec();
-        self.write_bytes(&content);
+        self.write_indent();
+        self.output
+            .extend_from_slice(&self.source[span.start..span.end]);
     }
 
     /// Write the original source content for an expression's span.
diff --git a/tests/fixtures/expected/error_make.nu b/tests/fixtures/expected/error_make.nu
index c005227..8b45be3 100644
--- a/tests/fixtures/expected/error_make.nu
+++ b/tests/fixtures/expected/error_make.nu
@@ -6,10 +6,7 @@ error make {
 }
 error make {
     msg: "error"
-    label: {
-        text: "here"
-        span: $span
-    }
+    label: {text: "here", span: $span}
 }
 error make {msg: "test error"}
 error make {
diff --git a/tests/fixtures/expected/issue128.nu b/tests/fixtures/expected/issue128.nu
new file mode 100644
index 0000000..77c2240
--- /dev/null
+++ b/tests/fixtures/expected/issue128.nu
@@ -0,0 +1,4 @@
+# module level doc
+
+# next line comment
+use a.nu
diff --git a/tests/fixtures/expected/issue129.nu b/tests/fixtures/expected/issue129.nu
new file mode 100644
index 0000000..f940244
--- /dev/null
+++ b/tests/fixtures/expected/issue129.nu
@@ -0,0 +1,3 @@
+def create-item [val_a: string, val_b: string] {
+    {field_a: $val_a, field_b: $val_b}
+}
diff --git a/tests/fixtures/expected/issue130.nu b/tests/fixtures/expected/issue130.nu
new file mode 100644
index 0000000..1aacbec
--- /dev/null
+++ b/tests/fixtures/expected/issue130.nu
@@ -0,0 +1,2 @@
+($json_input.tool_response? | default {})
+($json_input.tool_response? | default {})
diff --git a/tests/fixtures/expected/issue131.nu b/tests/fixtures/expected/issue131.nu
new file mode 100644
index 0000000..c3e30eb
--- /dev/null
+++ b/tests/fixtures/expected/issue131.nu
@@ -0,0 +1,3 @@
+if "a" != "b" {
+    return (call true)
+}
diff --git a/tests/fixtures/expected/issue132.nu b/tests/fixtures/expected/issue132.nu
new file mode 100644
index 0000000..c956318
--- /dev/null
+++ b/tests/fixtures/expected/issue132.nu
@@ -0,0 +1,3 @@
+for x: int in [1 2 3] {
+    print $x
+}
diff --git a/tests/fixtures/expected/issue133.nu b/tests/fixtures/expected/issue133.nu
new file mode 100644
index 0000000..8fa1a40
--- /dev/null
+++ b/tests/fixtures/expected/issue133.nu
@@ -0,0 +1,5 @@
+export def colors [] {
+    {
+        warn: (base16 "base0A" "yellow") # warning
+    }
+}
diff --git a/tests/fixtures/expected/issue134.nu b/tests/fixtures/expected/issue134.nu
new file mode 100644
index 0000000..bd1af52
--- /dev/null
+++ b/tests/fixtures/expected/issue134.nu
@@ -0,0 +1,6 @@
+let state = if $is_new {
+    init-state
+    | upsert APP_DRIVER (get-field $item "APP_DRIVER")
+    | upsert APP_STAGE (get-field $item "APP_STAGE")
+    | upsert APP_TOOL (get-field $item "APP_TOOL")
+}
diff --git a/tests/fixtures/input/issue128.nu b/tests/fixtures/input/issue128.nu
new file mode 100644
index 0000000..77c2240
--- /dev/null
+++ b/tests/fixtures/input/issue128.nu
@@ -0,0 +1,4 @@
+# module level doc
+
+# next line comment
+use a.nu
diff --git a/tests/fixtures/input/issue129.nu b/tests/fixtures/input/issue129.nu
new file mode 100644
index 0000000..1b83bba
--- /dev/null
+++ b/tests/fixtures/input/issue129.nu
@@ -0,0 +1,3 @@
+def create-item [val_a: string, val_b: string] {
+    { field_a: $val_a, field_b: $val_b }
+}
diff --git a/tests/fixtures/input/issue130.nu b/tests/fixtures/input/issue130.nu
new file mode 100644
index 0000000..0791ec6
--- /dev/null
+++ b/tests/fixtures/input/issue130.nu
@@ -0,0 +1,2 @@
+($json_input.tool_response? | default {})
+($json_input.tool_response? | default { })
diff --git a/tests/fixtures/input/issue131.nu b/tests/fixtures/input/issue131.nu
new file mode 100644
index 0000000..c3e30eb
--- /dev/null
+++ b/tests/fixtures/input/issue131.nu
@@ -0,0 +1,3 @@
+if "a" != "b" {
+    return (call true)
+}
diff --git a/tests/fixtures/input/issue132.nu b/tests/fixtures/input/issue132.nu
new file mode 100644
index 0000000..c956318
--- /dev/null
+++ b/tests/fixtures/input/issue132.nu
@@ -0,0 +1,3 @@
+for x: int in [1 2 3] {
+    print $x
+}
diff --git a/tests/fixtures/input/issue133.nu b/tests/fixtures/input/issue133.nu
new file mode 100644
index 0000000..8fa1a40
--- /dev/null
+++ b/tests/fixtures/input/issue133.nu
@@ -0,0 +1,5 @@
+export def colors [] {
+    {
+        warn: (base16 "base0A" "yellow") # warning
+    }
+}
diff --git a/tests/fixtures/input/issue134.nu b/tests/fixtures/input/issue134.nu
new file mode 100644
index 0000000..bd1af52
--- /dev/null
+++ b/tests/fixtures/input/issue134.nu
@@ -0,0 +1,6 @@
+let state = if $is_new {
+    init-state
+    | upsert APP_DRIVER (get-field $item "APP_DRIVER")
+    | upsert APP_STAGE (get-field $item "APP_STAGE")
+    | upsert APP_TOOL (get-field $item "APP_TOOL")
+}
diff --git a/tests/ground_truth.rs b/tests/ground_truth.rs
index 37e3d5f..f15970e 100644
--- a/tests/ground_truth.rs
+++ b/tests/ground_truth.rs
@@ -369,4 +369,11 @@ fixture_tests!(
     ("issue120", issue120_test, idempotency_issue120_test),
     ("issue121", issue121_test, idempotency_issue121_test),
     ("issue122", issue122_test, idempotency_issue122_test),
+    ("issue128", issue128_test, idempotency_issue128_test),
+    ("issue129", issue129_test, idempotency_issue129_test),
+    ("issue130", issue130_test, idempotency_issue130_test),
+    ("issue131", issue131_test, idempotency_issue131_test),
+    ("issue132", issue132_test, idempotency_issue132_test),
+    ("issue133", issue133_test, idempotency_issue133_test),
+    ("issue134", issue134_test, idempotency_issue134_test),
 );