From 0eba5746247bdd0e8d50e1151090673d06133d77 Mon Sep 17 00:00:00 2001 From: Deepak Nayak Date: Sat, 10 Oct 2026 17:15:10 +0000 Subject: [PATCH] fix(cat): Preserve Ion comments in text output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ion cat` read each input into the Ion data model and re-serialized it. Because comments are not part of the data model, the value-level reader discarded them, so running `ion cat` on a commented document silently stripped every comment. Preserve comments that appear between top-level values when the output is text. The input is read into memory and each value's source byte range is used to locate the trivia regions between top-level values; per the Ion grammar those regions contain only whitespace and comments, so comments are recovered unambiguously (comment markers inside string/clob literals fall inside a value's range and are correctly left untouched) and re-emitted around the pretty-printed values. Add a `--strip-comments` flag to opt back into discarding comments. Binary output is unchanged (binary Ion has no comments). Comments inside a value (e.g. between a struct's fields) are not preserved, as the value itself is re-serialized by the writer. 🤖 Assisted by AI https://github.com/amazon-ion/ion-cli/issues/225 --- README.md | 12 ++++ src/bin/ion/commands/cat.rs | 37 ++++++++++-- src/bin/ion/transcribe.rs | 117 ++++++++++++++++++++++++++++++++++++ tests/cli.rs | 83 +++++++++++++++++++++++++ 4 files changed, 244 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 685a48d1..68088178 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,18 @@ You can use the `--format`/`-f` flag to specify the desired format. The supporte * `lines` - Text Ion that places each value on its own line. * `binary`- Binary Ion +#### Comments + +Ion comments are not part of the Ion data model, but `ion cat` preserves comments that appear +between top-level values when the output is text. Use `--strip-comments` to discard them instead: + +```shell +ion cat --strip-comments my_file.ion +``` + +Comments are always discarded when the output format is `binary` (binary Ion has no comments), and +comments that appear *inside* a value (for example, between a struct's fields) are not preserved. + ### Converting between Ion formats Convert Ion text (or JSON) to Ion binary: diff --git a/src/bin/ion/commands/cat.rs b/src/bin/ion/commands/cat.rs index 4aaef46f..516f4dac 100644 --- a/src/bin/ion/commands/cat.rs +++ b/src/bin/ion/commands/cat.rs @@ -1,9 +1,10 @@ use anyhow::Result; -use clap::{ArgMatches, Command}; +use clap::{Arg, ArgAction, ArgMatches, Command}; use ion_rs::*; +use std::io::Read; -use crate::commands::{CommandIo, IonCliCommand, WithIonCliArgument}; -use crate::transcribe::write_all_as; +use crate::commands::{CommandIo, Format, IonCliCommand, WithIonCliArgument}; +use crate::transcribe::{write_all_as, write_all_as_text_preserving_comments}; pub struct CatCommand; @@ -32,14 +33,40 @@ impl IonCliCommand for CatCommand { .with_format() .with_syntax_highlighting() .with_ion_version() + .arg( + Arg::new("strip-comments") + .long("strip-comments") + .action(ArgAction::SetTrue) + .help( + "Discard Ion comments instead of preserving them. By default, comments \ + in text input are preserved in text output. Comments are always \ + discarded when the output format is binary.", + ), + ) } fn run(&self, _command_path: &mut Vec, args: &ArgMatches) -> Result<()> { + let strip_comments = args + .get_one::("strip-comments") + .copied() + .unwrap_or(false); let transform = None:: Result>; CommandIo::new(args)?.for_each_input(|output, input| { - let mut reader = Reader::new(AnyEncoding, input.into_source())?; let (encoding, format) = (*output.encoding(), *output.format()); - write_all_as(&mut reader, output, encoding, format, transform)?; + // Ion comments exist only in text and are discarded by the value-level reader, so they + // can only be recovered when the output is also text. Preserve them by default in that + // case; binary output and an explicit --strip-comments both use the standard path. + match format { + Format::Text(text_format) if !strip_comments => { + let mut data = Vec::new(); + input.into_source().read_to_end(&mut data)?; + write_all_as_text_preserving_comments(&data, output, encoding, text_format)?; + } + _ => { + let mut reader = Reader::new(AnyEncoding, input.into_source())?; + write_all_as(&mut reader, output, encoding, format, transform)?; + } + } Ok(()) }) } diff --git a/src/bin/ion/transcribe.rs b/src/bin/ion/transcribe.rs index 26d61df7..3d0a0bb6 100644 --- a/src/bin/ion/transcribe.rs +++ b/src/bin/ion/transcribe.rs @@ -47,6 +47,123 @@ pub(crate) fn write_n_as Result>( Ok(written) } +/// Transcribes all values from `data` to `output` in the requested text `encoding`/`format`, +/// preserving any Ion comments that appear between top-level values in the input. +/// +/// Ion comments are not part of the Ion data model, so the value-level reader discards them. +/// To preserve them, this function reads the input into memory and uses each value's source byte +/// range (via the reader's tooling APIs) to locate the "trivia" regions that sit *between* +/// top-level values. Per the Ion grammar, those regions contain only whitespace and comments, so +/// the comments can be recovered unambiguously and re-emitted around the pretty-printed values. +/// +/// Only text output is supported; binary Ion has no notion of comments. Comments that appear +/// *inside* a top-level value (for example, between a container's child values or between an +/// annotation and its value) fall within that value's source range and are not preserved, because +/// the value itself is re-serialized by the writer. +pub(crate) fn write_all_as_text_preserving_comments( + data: &[u8], + output: &mut impl Write, + encoding: IonEncoding, + text_format: TextFormat, +) -> Result { + match encoding { + IonEncoding::Text_1_0 => { + let writer = Writer::new(v1_0::Text.with_format(text_format), output)?; + transcribe_preserving_comments(writer, data) + } + IonEncoding::Text_1_1 => { + let writer = Writer::new(v1_1::Text.with_format(text_format), output)?; + transcribe_preserving_comments(writer, data) + } + unsupported => bail!( + "comment preservation is only supported for text output, but got '{:?}'", + unsupported + ), + } +} + +/// Shared implementation of [`write_all_as_text_preserving_comments`] that is generic over the +/// concrete text writer encoding. +fn transcribe_preserving_comments( + mut writer: Writer, + data: &[u8], +) -> Result { + let mut reader = Reader::new(AnyEncoding, data)?; + // Byte offset into `data` of the first byte that has not yet been accounted for by an emitted + // value or comment. + let mut cursor: usize = 0; + let mut index: usize = 0; + // Comment recovery relies on each value exposing its source byte range. Macro-expanded values + // (Ion 1.1) do not have a single source range; if we encounter one, we stop recovering comments + // to avoid re-scanning already-consumed input, but continue transcribing values faithfully. + let mut tracking_comments = true; + + while let Some(value) = reader.next()? { + if tracking_comments { + match value.expanded().range() { + Some(range) if range.start >= cursor => { + // Flush buffered value bytes so the comments we write next land *after* them. + writer.flush()?; + write_comments_in(writer.output_mut(), &data[cursor..range.start])?; + cursor = range.end; + } + // A range we can't use for ordering (out of order or absent); stop recovering. + _ => tracking_comments = false, + } + } + writer.write(value)?; + index += 1; + } + + if tracking_comments && cursor < data.len() { + // Preserve any trailing comments that follow the last value. + writer.flush()?; + write_comments_in(writer.output_mut(), &data[cursor..])?; + } + + writer.flush()?; + Ok(index) +} + +/// Scans `region` -- a slice of input that contains only whitespace and Ion comments -- and writes +/// each comment to `output`, one per line. Both line (`//`) and block (`/* */`) comments are +/// recognized. Any non-comment bytes (whitespace, and defensively any other content) are skipped. +fn write_comments_in(output: &mut impl Write, region: &[u8]) -> Result<()> { + let mut i = 0; + while i < region.len() { + if region[i] == b'/' && i + 1 < region.len() && region[i + 1] == b'/' { + // Line comment: runs until the next line break or the end of the region. + let start = i; + i += 2; + while i < region.len() && region[i] != b'\n' && region[i] != b'\r' { + i += 1; + } + write_comment_line(output, ®ion[start..i])?; + } else if region[i] == b'/' && i + 1 < region.len() && region[i + 1] == b'*' { + // Block comment: runs until the closing `*/` (or the end of the region if unterminated, + // though a well-formed input will always close it). + let start = i; + i += 2; + while i + 1 < region.len() && !(region[i] == b'*' && region[i + 1] == b'/') { + i += 1; + } + let end = (i + 2).min(region.len()); + i = end; + write_comment_line(output, ®ion[start..end])?; + } else { + i += 1; + } + } + Ok(()) +} + +/// Writes a single comment on its own line. +fn write_comment_line(output: &mut impl Write, comment: &[u8]) -> Result<()> { + output.write_all(comment)?; + output.write_all(b"\n")?; + Ok(()) +} + /// Writes up to `count` values from the `Reader` to the provided `Writer`, /// applying an optional mapping function to each element. fn transcribe_n Result>( diff --git a/tests/cli.rs b/tests/cli.rs index d6059aaf..28ce2999 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -351,3 +351,86 @@ mod code_gen_tests { Ok(()) } } + +/// Tests for `ion cat` comment preservation (GitHub issue #225). +/// +/// Ion comments are not part of the data model, so `ion cat` historically discarded them. These +/// tests verify that comments appearing between top-level values are now preserved by default for +/// text output, that `--strip-comments` restores the old behavior, and that comment-like byte +/// sequences inside string/clob literals are never mistaken for comments. +mod comment_preservation { + use assert_cmd::Command; + use std::time::Duration; + + /// Runs `ion cat` with the given extra args and stdin, returning stdout as a `String`. + fn cat(extra_args: &[&str], stdin: &str) -> String { + let mut cmd = Command::cargo_bin("ion").unwrap(); + cmd.arg("cat").args(extra_args).timeout(Duration::new(5, 0)); + cmd.write_stdin(stdin.to_owned()); + let output = cmd.assert().success().get_output().stdout.clone(); + String::from_utf8(output).expect("ion cat produced non-UTF-8 text output") + } + + #[test] + fn preserves_top_level_line_comment_by_default() { + let out = cat(&[], "// a leading comment\n42\n"); + assert!(out.contains("// a leading comment"), "got: {out:?}"); + assert!(out.contains("42"), "got: {out:?}"); + } + + #[test] + fn preserves_block_comment_between_values() { + let out = cat(&[], "1\n/* block comment */\n2\n"); + assert!(out.contains("/* block comment */"), "got: {out:?}"); + } + + #[test] + fn preserves_trailing_comment_after_last_value() { + let out = cat(&[], "42\n// trailing comment\n"); + assert!(out.contains("// trailing comment"), "got: {out:?}"); + } + + #[test] + fn preserves_comment_when_no_values_present() { + let out = cat(&[], "// only a comment\n"); + assert!(out.contains("// only a comment"), "got: {out:?}"); + } + + #[test] + fn strip_comments_flag_discards_comments() { + let out = cat(&["--strip-comments"], "// a comment\n42\n"); + assert!( + !out.contains("a comment"), + "comment should be stripped, got: {out:?}" + ); + assert!(out.contains("42"), "got: {out:?}"); + } + + #[test] + fn comment_markers_inside_strings_are_not_treated_as_comments() { + // The `//` and `/*` here are string content, not comments. They must appear verbatim + // inside the quoted string and must not cause spurious comment extraction. + let out = cat(&[], "\"not // a comment or /* block */\"\n"); + assert!( + out.contains("not // a comment or /* block */"), + "got: {out:?}" + ); + } + + #[test] + fn binary_output_succeeds_and_drops_comments() { + // Binary Ion has no comments; the command must still succeed and emit valid binary Ion. + let mut cmd = Command::cargo_bin("ion").unwrap(); + cmd.arg("cat") + .args(["-f", "binary"]) + .timeout(Duration::new(5, 0)) + .write_stdin("// a comment\n42\n".to_owned()); + let output = cmd.assert().success().get_output().stdout.clone(); + // Binary Ion 1.0 streams start with the binary version marker 0xE0 0x01 0x00 0xEA. + assert_eq!( + &output[0..4], + &[0xE0, 0x01, 0x00, 0xEA], + "expected a binary IVM" + ); + } +}