From b0ed4b87a373f4cc4d885b42609b551e94a028d0 Mon Sep 17 00:00:00 2001 From: Xavier Ruiz Date: Wed, 8 Jul 2026 16:08:20 -0400 Subject: [PATCH] fix: preserve comments in match arms, nested records, and raw strings - match-arm guards no longer hoist or drop neighboring comments - nested records stay multiline instead of collapsing and eating field comments - extract_comments skips r#'...'# raw strings (were eaten, non-idempotent) --- src/formatting/collections.rs | 23 +++++- src/formatting/comments.rs | 77 +++++++++++++++---- src/formatting/mod.rs | 8 ++ ...tch_guard_paren_does_not_hoist_comments.nu | 13 ++++ ...nter_arm_comment_not_hoisted_into_guard.nu | 8 ++ .../nested_record_comments_preserved.nu | 10 +++ ...tring_hash_delimiter_preserves_comments.nu | 17 ++++ ...tch_guard_paren_does_not_hoist_comments.nu | 13 ++++ ...nter_arm_comment_not_hoisted_into_guard.nu | 8 ++ .../input/nested_record_comments_preserved.nu | 10 +++ ...tring_hash_delimiter_preserves_comments.nu | 17 ++++ tests/ground_truth.rs | 20 +++++ 12 files changed, 206 insertions(+), 18 deletions(-) create mode 100644 tests/fixtures/expected/match_guard_paren_does_not_hoist_comments.nu create mode 100644 tests/fixtures/expected/match_inter_arm_comment_not_hoisted_into_guard.nu create mode 100644 tests/fixtures/expected/nested_record_comments_preserved.nu create mode 100644 tests/fixtures/expected/raw_string_hash_delimiter_preserves_comments.nu create mode 100644 tests/fixtures/input/match_guard_paren_does_not_hoist_comments.nu create mode 100644 tests/fixtures/input/match_inter_arm_comment_not_hoisted_into_guard.nu create mode 100644 tests/fixtures/input/nested_record_comments_preserved.nu create mode 100644 tests/fixtures/input/raw_string_hash_delimiter_preserves_comments.nu diff --git a/src/formatting/collections.rs b/src/formatting/collections.rs index ed0f5e6..c61e5d2 100644 --- a/src/formatting/collections.rs +++ b/src/formatting/collections.rs @@ -266,7 +266,18 @@ impl<'a> Formatter<'a> { let preserve_multiline_top_level = source_has_newline && self.indent_level == 0; - if all_simple && items.len() <= 3 && !nested_multiline && !preserve_multiline_top_level { + // A record containing standalone comments must stay multiline, otherwise + // the inline path emits fields without `write_comments_before` and the + // comments are silently dropped. Same `has_comments_in_span` guard that + // `format_list` already applies for the same reason. + let has_comments_in_record = self.has_comments_in_span(span.start, span.end); + + if all_simple + && items.len() <= 3 + && !nested_multiline + && !preserve_multiline_top_level + && !has_comments_in_record + { // Inline format let record_start = self.output.len(); self.write("{"); @@ -461,7 +472,10 @@ impl<'a> Formatter<'a> { self.newline(); self.indent_level += 1; - for ((_pattern, expr), lhs) in matches.iter().zip(rendered_lhs.iter()) { + for ((pattern, expr), lhs) in matches.iter().zip(rendered_lhs.iter()) { + // Emit any standalone comments preceding this arm at the arm + // boundary, so they are neither dropped nor pulled into a guard. + self.write_comments_before(pattern.span.start); self.write_indent(); self.write_bytes(lhs); @@ -582,6 +596,11 @@ impl<'a> Formatter<'a> { fn render_match_arm_lhs(&self, pattern: &MatchPattern, rhs: &Expression) -> Vec { self.probe_format(|probe| { + // Constrain comment emission to this arm's own span. Seeding to the + // pattern start stops a parenthesized guard from vacuuming comments + // that sit *between* the previous arm and this one — those are + // emitted at the arm boundary by `format_match_block` instead. + probe.last_pos = pattern.span.start; probe.format_match_pattern_for_arm(pattern, rhs); if let Some(guard) = &pattern.guard { probe.write(" if "); diff --git a/src/formatting/comments.rs b/src/formatting/comments.rs index a323d90..cb1d1c2 100644 --- a/src/formatting/comments.rs +++ b/src/formatting/comments.rs @@ -14,29 +14,41 @@ use nu_protocol::Span; pub(super) fn extract_comments(source: &[u8]) -> Vec<(Span, Vec)> { let mut comments = Vec::new(); let mut i = 0; - let mut in_string = false; - let mut string_char = b'"'; while i < source.len() { let c = source[i]; - // Track string state to avoid matching # inside strings - if !in_string && (c == b'"' || c == b'\'') { - in_string = true; - string_char = c; - i += 1; - continue; - } - - if in_string { - if c == b'\\' && i + 1 < source.len() { - i += 2; // Skip escaped character + // Raw string: r#'...'# (or r##'...'##, r###'...'###, ...). Skip the whole + // literal so that neither the `#` in its delimiters nor any `#` in its + // body is ever mistaken for a comment. Without this, the `#` in `r#'` + // starts a bogus comment and the closing `'#` opens a phantom string + // that swallows every real comment until the next stray apostrophe. + if c == b'r' { + if let Some(hashes) = raw_string_open_hashes(source, i) { + let body_start = i + 1 + hashes + 1; // 'r' + N*'#' + '\'' + i = find_raw_string_end(source, body_start, hashes).unwrap_or(source.len()); continue; } - if c == string_char { - in_string = false; - } + } + + // Quoted string. Single-quoted strings are raw (no escapes); only + // double-quoted strings process backslash escapes. Consuming each string + // inline (rather than a persistent flag) means an unterminated string can + // never bleed across the rest of the file. + if c == b'"' || c == b'\'' { + let quote = c; i += 1; + while i < source.len() { + let b = source[i]; + if quote == b'"' && b == b'\\' && i + 1 < source.len() { + i += 2; + continue; + } + i += 1; + if b == quote { + break; + } + } continue; } @@ -47,6 +59,7 @@ pub(super) fn extract_comments(source: &[u8]) -> Vec<(Span, Vec)> { i += 1; } comments.push((Span::new(start, i), source[start..i].to_vec())); + continue; } i += 1; @@ -55,6 +68,38 @@ pub(super) fn extract_comments(source: &[u8]) -> Vec<(Span, Vec)> { comments } +/// If `source[i..]` begins a raw-string opener (`r#'`, `r##'`, ...), return the +/// number of `#` hashes; otherwise `None`. Requires `source[i] == b'r'`. +fn raw_string_open_hashes(source: &[u8], i: usize) -> Option { + let mut j = i + 1; + let mut hashes = 0; + while j < source.len() && source[j] == b'#' { + hashes += 1; + j += 1; + } + if hashes >= 1 && source.get(j) == Some(&b'\'') { + Some(hashes) + } else { + None + } +} + +/// Find the byte index just past a raw-string closer (`'` followed by `hashes` +/// `#`), scanning from `body_start`. Returns `None` if unterminated. +fn find_raw_string_end(source: &[u8], body_start: usize, hashes: usize) -> Option { + let mut j = body_start; + while j < source.len() { + if source[j] == b'\'' { + let close = j + 1 + hashes; + if source.len() >= close && source[j + 1..close].iter().all(|&b| b == b'#') { + return Some(close); + } + } + j += 1; + } + None +} + // ───────────────────────────────────────────────────────────────────────────── // Formatter comment methods // ───────────────────────────────────────────────────────────────────────────── diff --git a/src/formatting/mod.rs b/src/formatting/mod.rs index 48bf1f2..738dd19 100644 --- a/src/formatting/mod.rs +++ b/src/formatting/mod.rs @@ -239,6 +239,14 @@ impl<'a> Formatter<'a> { self.config, self.allow_compact_recovered_record_style, ); + // Inherit the parent's comment-emission cursor so a probe never + // re-vacuums comments that precede the current position (they are + // already emitted / owned by the parent). Without this, rendering a + // parenthesized match-arm guard in a probe flushes every earlier + // standalone comment inside the guard's `(` (issue: match-guard + // comment hoist). + probe.last_pos = self.last_pos; + probe.written_comments = self.written_comments.clone(); f(&mut probe); probe.output } diff --git a/tests/fixtures/expected/match_guard_paren_does_not_hoist_comments.nu b/tests/fixtures/expected/match_guard_paren_does_not_hoist_comments.nu new file mode 100644 index 0000000..6687d11 --- /dev/null +++ b/tests/fixtures/expected/match_guard_paren_does_not_hoist_comments.nu @@ -0,0 +1,13 @@ +def first [] { + # orphan comment that must stay put + 1 +} + +# a leading section comment +def z [] { + match $rest { + [] => { '~' } + [$arg] if ($arg | path expand | path type) == 'dir' => { $arg } + _ => { 0 } + } +} diff --git a/tests/fixtures/expected/match_inter_arm_comment_not_hoisted_into_guard.nu b/tests/fixtures/expected/match_inter_arm_comment_not_hoisted_into_guard.nu new file mode 100644 index 0000000..31c4144 --- /dev/null +++ b/tests/fixtures/expected/match_inter_arm_comment_not_hoisted_into_guard.nu @@ -0,0 +1,8 @@ +match $x { + a => 1 + # a comment between arms, before a parenthesized guard + b if ($y | foo) == 'z' => 2 + # before a bare guard + c if $y == 'w' => 3 + _ => 4 +} diff --git a/tests/fixtures/expected/nested_record_comments_preserved.nu b/tests/fixtures/expected/nested_record_comments_preserved.nu new file mode 100644 index 0000000..82ae4c7 --- /dev/null +++ b/tests/fixtures/expected/nested_record_comments_preserved.nu @@ -0,0 +1,10 @@ +$env.config = { + trim: { + # wrapping or truncating + methodology: wrapping + # A strategy used by the 'wrapping' methodology + wrapping_try_keep_words: true + # A suffix used by the 'truncating' methodology + truncating_suffix: "..." + } +} diff --git a/tests/fixtures/expected/raw_string_hash_delimiter_preserves_comments.nu b/tests/fixtures/expected/raw_string_hash_delimiter_preserves_comments.nu new file mode 100644 index 0000000..189e00e --- /dev/null +++ b/tests/fixtures/expected/raw_string_hash_delimiter_preserves_comments.nu @@ -0,0 +1,17 @@ +def mint-token [cache: string] { + let remote = r#' + set -euo pipefail + cache="$1" + # -print -quit: find exits after the first hit + adm=$(find /nix/store -name atticadm -print -quit) + '# + # str trim: drop the trailing newline so it can't land inside the + # secret and break the Authorization header. + $remote | ssh (arca-ssh) bash -s $cache | str trim +} + +# Cache names are validated to [a-z0-9-] — attic's own charset, and it blocks +# shell-metacharacter injection into the `ssh ... bash -s $cache` above. +def declared-caches [] { + 1 +} diff --git a/tests/fixtures/input/match_guard_paren_does_not_hoist_comments.nu b/tests/fixtures/input/match_guard_paren_does_not_hoist_comments.nu new file mode 100644 index 0000000..6687d11 --- /dev/null +++ b/tests/fixtures/input/match_guard_paren_does_not_hoist_comments.nu @@ -0,0 +1,13 @@ +def first [] { + # orphan comment that must stay put + 1 +} + +# a leading section comment +def z [] { + match $rest { + [] => { '~' } + [$arg] if ($arg | path expand | path type) == 'dir' => { $arg } + _ => { 0 } + } +} diff --git a/tests/fixtures/input/match_inter_arm_comment_not_hoisted_into_guard.nu b/tests/fixtures/input/match_inter_arm_comment_not_hoisted_into_guard.nu new file mode 100644 index 0000000..31c4144 --- /dev/null +++ b/tests/fixtures/input/match_inter_arm_comment_not_hoisted_into_guard.nu @@ -0,0 +1,8 @@ +match $x { + a => 1 + # a comment between arms, before a parenthesized guard + b if ($y | foo) == 'z' => 2 + # before a bare guard + c if $y == 'w' => 3 + _ => 4 +} diff --git a/tests/fixtures/input/nested_record_comments_preserved.nu b/tests/fixtures/input/nested_record_comments_preserved.nu new file mode 100644 index 0000000..82ae4c7 --- /dev/null +++ b/tests/fixtures/input/nested_record_comments_preserved.nu @@ -0,0 +1,10 @@ +$env.config = { + trim: { + # wrapping or truncating + methodology: wrapping + # A strategy used by the 'wrapping' methodology + wrapping_try_keep_words: true + # A suffix used by the 'truncating' methodology + truncating_suffix: "..." + } +} diff --git a/tests/fixtures/input/raw_string_hash_delimiter_preserves_comments.nu b/tests/fixtures/input/raw_string_hash_delimiter_preserves_comments.nu new file mode 100644 index 0000000..189e00e --- /dev/null +++ b/tests/fixtures/input/raw_string_hash_delimiter_preserves_comments.nu @@ -0,0 +1,17 @@ +def mint-token [cache: string] { + let remote = r#' + set -euo pipefail + cache="$1" + # -print -quit: find exits after the first hit + adm=$(find /nix/store -name atticadm -print -quit) + '# + # str trim: drop the trailing newline so it can't land inside the + # secret and break the Authorization header. + $remote | ssh (arca-ssh) bash -s $cache | str trim +} + +# Cache names are validated to [a-z0-9-] — attic's own charset, and it blocks +# shell-metacharacter injection into the `ssh ... bash -s $cache` above. +def declared-caches [] { + 1 +} diff --git a/tests/ground_truth.rs b/tests/ground_truth.rs index 1a97570..d5fff0f 100644 --- a/tests/ground_truth.rs +++ b/tests/ground_truth.rs @@ -524,6 +524,26 @@ fixture_tests!( ground_truth_match_guards_preserved_issue139, idempotency_match_guards_preserved_issue139 ), + ( + "match_guard_paren_does_not_hoist_comments", + ground_truth_match_guard_paren_does_not_hoist_comments, + idempotency_match_guard_paren_does_not_hoist_comments + ), + ( + "match_inter_arm_comment_not_hoisted_into_guard", + ground_truth_match_inter_arm_comment_not_hoisted_into_guard, + idempotency_match_inter_arm_comment_not_hoisted_into_guard + ), + ( + "nested_record_comments_preserved", + ground_truth_nested_record_comments_preserved, + idempotency_nested_record_comments_preserved + ), + ( + "raw_string_hash_delimiter_preserves_comments", + ground_truth_raw_string_hash_delimiter_preserves_comments, + idempotency_raw_string_hash_delimiter_preserves_comments + ), ( "catch_block_indentation_and_closing_brace_preserved_issue140", ground_truth_catch_block_indentation_and_closing_brace_preserved_issue140,