diff --git a/CHANGELOG.md b/CHANGELOG.md index feb2a59..4f2aafd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ number says nothing about any other port. See the Releases section of - **`DatasetReader` returns a complete row from a stream that stays open** (#171), as protowire-go does. On a pipe, a socket or a file still being appended to, `Next` could wait for 4 KiB more input, or for the stream to end, before returning a row that had already arrived. The read took a full 4 KiB (`std::istream::read`), and the scan held a token until four more bytes followed it. `Next` now reads what the stream has, blocking only when nothing is buffered. It takes a token as complete as soon as the bytes the lexer looks at past it have arrived. Input that ends reads as before. - **A row's validator error and schema violation carry no row position** (#166), as protowire-go gives them. `DatasetReader::Scan` and `BindRow` put the row's position in front of every error, so a validator's refusal read `2:1: check: id: denied (...)` and a schema violation `2:1: PXF schema violations: ...`. Only a decode error is moved to the row now, and a hook's error from `on_secret_field` is one, so it keeps the row's position and its column. `Scan` also checks the column paths before the schema, as Go does, so a row type that breaks both gets the column's error. - **`DatasetReader::Scan` honours every option but `max_message_size`, and protowire-go is changing to match** (#166). Go's `Scan` ignored `skip_validate`, `skip_post_decode`, `discard_unknown`, `on_secret_field` and `validator`, while cpp's honoured them. The owner chose to change Go (trendvidia/protowire-go#280) and keep cpp's behaviour. Each option now has a test that binds the same row through `Scan` and `BindRow`, with the texts Go gives, and `dataset_reader.h` lists the options `Scan` uses. +- **A comment that no entry follows is kept** (#176), as protowire-go keeps it (protowire-go#279, PR #283). The parser attached a comment only to the entry after it. So a comment at the end of a document was dropped, and one before a block's closing `}` was dropped or carried out of the block onto the next entry. Comments between dataset rows went to the next directive, or were dropped at the end. `Document`, `Block` and `BlockVal` now have `trailing_comments`, and `DatasetRow` has `leading_comments`, which `Parse` and `DatasetReader` both set. `FormatDocument` writes each in place, `CanonicalizeKeyed` keeps them when it rewrites a keyed block, and `Rewriter` copies them. A comment inside a row now comes before the row. Comments inside a list still move after it (protowire-go#282). Streaming a 300,000-row table is about 4% slower, and 9% slower when each row carries a comment, since those comments are now kept. ## [2.0.0] — 2026-11-05 diff --git a/cmd/pxf_diff/main.cc b/cmd/pxf_diff/main.cc index 022886f..8a042f6 100644 --- a/cmd/pxf_diff/main.cc +++ b/cmd/pxf_diff/main.cc @@ -372,6 +372,7 @@ class AstDump { Comments("", x.leading_comments); for (const auto& r : x.rows) { out_ += "row " + Pos(r.pos) + "\n"; + Comments(" ", r.leading_comments); for (const auto& c : r.cells) { if (c.has_value()) { Value(" ", *c); @@ -382,6 +383,7 @@ class AstDump { } } Entries("", doc.entries); + Comments("", doc.trailing_comments, "trailing"); } private: @@ -394,9 +396,11 @@ class AstDump { for (size_t i = 0; i < v.size(); ++i) out += (i ? " " : "") + v[i]; return out; } - void Comments(const std::string& ind, const std::vector& cs) { + void Comments(const std::string& ind, + const std::vector& cs, + const char* label = "comment") { for (const auto& c : cs) { - out_ += ind + "comment " + Pos(c.pos) + " " + Quote(c.text) + "\n"; + out_ += ind + label + " " + Pos(c.pos) + " " + Quote(c.text) + "\n"; } } void Kind(const pxf::StringVal& v, const std::string&) { @@ -430,6 +434,7 @@ class AstDump { void Kind(const pxf::BlockVal& v, const std::string& ind) { out_ += "blockval\n"; Entries(ind + " ", v.entries); + Comments(ind + " ", v.trailing_comments, "trailing"); } void Value(const std::string& ind, const pxf::ValuePtr& v) { auto [start, end] = pxf::ValueSpan(v); @@ -456,6 +461,7 @@ class AstDump { out_ += "block " + Quote(e.name) + " " + Bool(e.name_quoted) + "\n"; Comments(ind, e.leading_comments); Entries(ind + " ", e.entries); + Comments(ind + " ", e.trailing_comments, "trailing"); } void Entries(const std::string& ind, const std::vector& es) { for (const auto& e : es) { diff --git a/include/protowire/pxf/ast.h b/include/protowire/pxf/ast.h index 7d10fba..a26468f 100644 --- a/include/protowire/pxf/ast.h +++ b/include/protowire/pxf/ast.h @@ -180,7 +180,11 @@ struct Block { // else. bool name_quoted = false; std::vector entries; - std::vector leading_comments; + std::vector + leading_comments; // trailing_comments are the comments after the last entry, before the + // closing '}' (all of an empty block's comments), as protowire-go has them + // (#176). + std::vector trailing_comments; }; // TypeEntry is `@type = ""` opening a block bound to // google.protobuf.Any (draft -01 "Any Blocks"). The rest of the block is a @@ -197,7 +201,11 @@ struct TypeEntry { struct BlockVal { Position pos; Position end; // just past the closing '}' - std::vector entries; + std::vector + entries; // trailing_comments are the comments after the last entry, before the + // closing '}' (all of an empty block's comments), as protowire-go has them + // (#176). + std::vector trailing_comments; }; // Directive is a top-of-document `@ *() [{ ... }]` @@ -237,6 +245,10 @@ struct Directive { struct DatasetRow { Position pos; std::vector> cells; + // leading_comments are the comments before this row (after the column + // list or the previous row) and inside it. FormatDocument writes a row + // on one line, so it writes them all before the row (#176). + std::vector leading_comments; }; // DatasetDirective is a `@dataset ( col1, col2, ... ) row*` entry @@ -308,6 +320,9 @@ struct Document { 0; // byte offset where the schema-typed body begins (after all leading directives) std::vector entries; std::vector leading_comments; + // trailing_comments are the comments after the document's last entry, + // directive or dataset row, which precede no entry of their own (#176). + std::vector trailing_comments; }; Position EntryPos(const EntryPtr& e); diff --git a/include/protowire/pxf/dataset_reader.h b/include/protowire/pxf/dataset_reader.h index fc0598a..70bc19d 100644 --- a/include/protowire/pxf/dataset_reader.h +++ b/include/protowire/pxf/dataset_reader.h @@ -140,9 +140,9 @@ class DatasetReader { Status Pull(size_t n); Status Fail(Status st); int MaxBytesLiteral() const; - StatusOr ParseRow(int start, int end, int* end_offset = nullptr) const; + StatusOr ParseRow(int end, int* end_offset = nullptr) const; std::optional TryWholeRow(); - Status RowError(int start) const; + Status RowError() const; RowScan ScanRow(int* start, int* end); RowScan ScanHeader(int* end); bool Complete(const Token& tok, int tok_end) const; diff --git a/src/pxf/dataset_reader.cc b/src/pxf/dataset_reader.cc index c946620..7bc0d92 100644 --- a/src/pxf/dataset_reader.cc +++ b/src/pxf/dataset_reader.cc @@ -240,7 +240,7 @@ Status DatasetReader::Next(DatasetRow* out) { int end = 0; switch (ScanRow(&start, &end)) { case RowScan::kRow: { - auto parsed = ParseRow(start, end); + auto parsed = ParseRow(end); Consume(static_cast(end)); if (!parsed.ok()) return Fail(parsed.status()); *out = std::move(parsed).consume(); @@ -251,7 +251,7 @@ Status DatasetReader::Next(DatasetRow* out) { // stopped the scan: the lexer may reject a token before its end (an // oversized bytes literal), and cut there it would read as // unterminated instead. - return Fail(RowError(start)); + return Fail(RowError()); case RowScan::kEnd: // The directive and what precedes it stay buffered, for Tail. finished_ = true; @@ -266,7 +266,7 @@ Status DatasetReader::Next(DatasetRow* out) { // At the end of the source only spaces and comments may remain; anything // else is a row cut short. if (src_eof_) { - if (start >= 0) return Fail(RowError(start)); + if (start >= 0) return Fail(RowError()); Consume(pending_.size()); finished_ = true; return Status::OK(); @@ -357,14 +357,16 @@ int DatasetReader::MaxBytesLiteral() const { : kMaxBytesLiteralLength; } -// ParseRow parses pending_ from `start` as one row, with the parser Parse -// uses, at its stream position. `end` bounds the bytes it may read. -StatusOr DatasetReader::ParseRow(int start, int end, int* end_offset) const { - return ParseDatasetRow(std::string_view(pending_).substr(start, end - start), +// ParseRow parses the row in pending_[:end] with the parser Parse uses, at +// its stream position. It starts at pending_[0], as protowire-go's parseRow +// does: only newlines and comments precede the row's `(` there, and those +// comments are the row's leading_comments, as Parse files them (#176). +StatusOr DatasetReader::ParseRow(int end, int* end_offset) const { + return ParseDatasetRow(std::string_view(pending_).substr(0, end), static_cast(columns_.size()), opts_.max_nesting_depth > 0 ? opts_.max_nesting_depth : kMaxNestingDepth, MaxBytesLiteral(), - PosAt(static_cast(start)), + base_, end_offset); } @@ -376,22 +378,22 @@ StatusOr DatasetReader::ParseRow(int start, int end, int* end_offset // boundary. Called only between rows. std::optional DatasetReader::TryWholeRow() { int end_offset = 0; - auto row = ParseRow(scan_pos_, static_cast(pending_.size()), &end_offset); + auto row = ParseRow(static_cast(pending_.size()), &end_offset); if (!row.ok()) return std::nullopt; Consume(static_cast(end_offset - base_.offset)); return std::move(row).consume(); } -// RowError is the error Parse reports for the row starting at pending_[start], -// which the scan found cannot close well. Every token before the one that -// stopped the scan is the same as Parse sees, so the row parser fails where -// Parse fails on the whole document. -Status DatasetReader::RowError(int start) const { - auto row = ParseRow(start, static_cast(pending_.size())); +// RowError is the error Parse reports for the row being scanned, which the +// scan found cannot close well. Only newlines and comments precede its `(` in +// the buffer, and every token before the one that stopped the scan is the +// same as Parse sees, so the row parser fails where Parse fails on the whole +// document. +Status DatasetReader::RowError() const { + auto row = ParseRow(static_cast(pending_.size())); if (!row.ok()) return row.status(); // Unreachable: the bytes hold no `)` that closes the row. - return Status::Error("pxf: DatasetReader: row at offset " + std::to_string(start) + - " neither closes nor fails"); + return Status::Error("pxf: DatasetReader: a row neither closes nor fails"); } // ScanRow finds the next row with the lexer Parse uses, resuming at diff --git a/src/pxf/format.cc b/src/pxf/format.cc index 60ef1d7..c03ff1e 100644 --- a/src/pxf/format.cc +++ b/src/pxf/format.cc @@ -145,6 +145,7 @@ class Formatter { } out_ += " {\n"; FormatEntries(b.entries, level + 1); + WriteComments(b.trailing_comments, level + 1); WriteIndent(level); out_ += "}\n"; } @@ -195,6 +196,7 @@ class Formatter { void FormatValueImpl(const BlockVal& v, int level) { out_ += "{\n"; FormatEntries(v.entries, level + 1); + WriteComments(v.trailing_comments, level + 1); WriteIndent(level); out_.push_back('}'); } @@ -295,6 +297,7 @@ void FormatDataset(const DatasetDirective& d, Formatter& f, std::string& out) { } out += ")\n"; for (const auto& row : d.rows) { + f.WriteComments(row.leading_comments, 0); out.push_back('('); for (size_t i = 0; i < row.cells.size(); ++i) { if (i > 0) out += ", "; @@ -356,6 +359,7 @@ std::string FormatDocument(const Document& doc) { } f.WriteComments(doc.leading_comments, 0); f.FormatEntries(doc.entries, 0); + f.WriteComments(doc.trailing_comments, 0); return out; } @@ -384,6 +388,7 @@ std::string FormatDocument(const Document& doc) { out.push_back('\n'); f.FormatEntries(doc.entries, 0); } + f.WriteComments(doc.trailing_comments, 0); return out; } diff --git a/src/pxf/keyed.cc b/src/pxf/keyed.cc index 38004b4..e9db154 100644 --- a/src/pxf/keyed.cc +++ b/src/pxf/keyed.cc @@ -35,10 +35,11 @@ void CanonEntries(std::vector& entries, const pb::Descriptor* desc); // redundant agreeing spelling of an entry's key — from entries. Disagreeing // or non-string assignments are kept (the document is invalid; formatting // must not silently change its meaning). Leading comments of a dropped -// assignment move to the next surviving entry. -void DropKeyAssignments(std::vector& entries, - std::string_view key_name, - std::string_view entry_name) { +// assignment move to the next surviving entry; when none follows, they are +// returned, for the block's trailing comments (#176). +std::vector DropKeyAssignments(std::vector& entries, + std::string_view key_name, + std::string_view entry_name) { std::vector pending; std::vector out; out.reserve(entries.size()); @@ -61,6 +62,13 @@ void DropKeyAssignments(std::vector& entries, out.push_back(std::move(e)); } entries = std::move(out); + return pending; +} + +// PrependComments puts `left` before the comments already in *to. +void PrependComments(std::vector left, std::vector* to) { + left.insert(left.end(), std::make_move_iterator(to->begin()), std::make_move_iterator(to->end())); + *to = std::move(left); } // CanonKeyedEntries normalizes the entries of a keyed block in place: @@ -81,6 +89,7 @@ void CanonKeyedEntries(Block* b, const pb::FieldDescriptor* fd, const pb::FieldD nb->name_quoted = a->key_quoted; nb->entries = std::move(bv->entries); nb->leading_comments = std::move(a->leading_comments); + nb->trailing_comments = std::move(bv->trailing_comments); eb = nb.get(); e = EntryPtr(std::move(nb)); } @@ -88,7 +97,7 @@ void CanonKeyedEntries(Block* b, const pb::FieldDescriptor* fd, const pb::FieldD } if (!eb) continue; // malformed entry; leave untouched if (eb->name_quoted && IsIdentifierSafeEntryName(eb->name)) eb->name_quoted = false; - DropKeyAssignments(eb->entries, key_name, eb->name); + PrependComments(DropKeyAssignments(eb->entries, key_name, eb->name), &eb->trailing_comments); CanonEntries(eb->entries, fd->message_type()); } } @@ -129,7 +138,7 @@ void CanonAnonymousKeyed(EntryPtr& entry, blk->leading_comments = std::move(n->leading_comments); for (size_t i = 0; i < lv->elements.size(); ++i) { auto* bv = GetMut(lv->elements[i]); - DropKeyAssignments(bv->entries, key_name, keys[i]); + std::vector left = DropKeyAssignments(bv->entries, key_name, keys[i]); CanonEntries(bv->entries, fd->message_type()); auto eb = std::make_unique(); eb->pos = bv->pos; @@ -137,6 +146,8 @@ void CanonAnonymousKeyed(EntryPtr& entry, eb->name = keys[i]; eb->name_quoted = !IsIdentifierSafeEntryName(keys[i]); eb->entries = std::move(bv->entries); + eb->trailing_comments = std::move(bv->trailing_comments); + PrependComments(std::move(left), &eb->trailing_comments); blk->entries.push_back(EntryPtr(std::move(eb))); } entry = EntryPtr(std::move(blk)); @@ -178,6 +189,7 @@ void CanonAssignment(EntryPtr& entry, Assignment* n, const pb::FieldDescriptor* blk->name = n->key; blk->entries = std::move(bv->entries); blk->leading_comments = std::move(n->leading_comments); + blk->trailing_comments = std::move(bv->trailing_comments); CanonKeyedEntries(blk.get(), fd, key_fd); entry = EntryPtr(std::move(blk)); } diff --git a/src/pxf/parser.cc b/src/pxf/parser.cc index 2b97efa..3b04c7e 100644 --- a/src/pxf/parser.cc +++ b/src/pxf/parser.cc @@ -49,19 +49,6 @@ class Parser { Advance(); } - // A parser that reads no comments before its first token. - struct DropLeadingCommentsTag {}; - Parser(DropLeadingCommentsTag, - std::string_view input, - int max_depth, - int max_bytes_literal, - Position start) - : lex_(input, start), max_depth_(max_depth), drop_comments_(true) { - lex_.SetMaxBytesLiteral(max_bytes_literal); - Advance(); - drop_comments_ = false; - } - struct TolerantTag {}; Parser(TolerantTag, std::string_view input, int max_depth, int max_bytes_literal) : lex_(input), max_depth_(max_depth), tolerant_(true) { @@ -134,7 +121,9 @@ class Parser { StatusOr ParseValue(int depth); StatusOr ParseList(int depth); StatusOr ParseBlockVal(int depth); - StatusOr> ParseBody(int depth, Position* end); + StatusOr> ParseBody(int depth, + Position* end, + std::vector* trailing); StatusOr ParseDirective(int* end_offset); StatusOr ParseProtoDirective(int* end_offset); StatusOr> ParseRowCell(); @@ -167,9 +156,6 @@ class Parser { int max_depth_ = kMaxNestingDepth; bool for_decoder_ = false; bool tolerant_ = false; - // Advance skips comments without keeping them, for a caller that reads - // none: DatasetReader's row parse, before the row's `(`. - bool drop_comments_ = false; std::vector errs_; Position err_pos_; // Just past the last significant token consumed: where a missing value @@ -183,7 +169,7 @@ void Parser::Advance() { current_ = lex_.Next(); if (current_.kind == TokenKind::kNewline) continue; if (current_.kind == TokenKind::kComment) { - if (!drop_comments_) comments_.push_back(Comment{current_.pos, std::string(current_.value)}); + comments_.push_back(Comment{current_.pos, std::string(current_.value)}); continue; } return; @@ -369,6 +355,8 @@ StatusOr Parser::ParseDocument() { auto entry = std::move(e).consume(); if (entry.has_value()) doc.entries.push_back(std::move(*entry)); } + // The comments after the last entry, directive or row (#176). + doc.trailing_comments = FlushComments(); return doc; } @@ -488,7 +476,7 @@ StatusOr> Parser::ParseEntry(int depth, bool allow_map_e return std::optional(EntryPtr(std::move(b))); } Advance(); // consume { - auto entries = ParseBody(depth + 1, &b->end); + auto entries = ParseBody(depth + 1, &b->end, &b->trailing_comments); if (!entries.ok()) return entries.status(); b->entries = std::move(entries).consume(); return std::optional(EntryPtr(std::move(b))); @@ -689,14 +677,19 @@ StatusOr Parser::ParseBlockVal(int depth) { return ValuePtr(std::move(bv)); } Advance(); // { - auto entries = ParseBody(depth + 1, &bv->end); + auto entries = ParseBody(depth + 1, &bv->end, &bv->trailing_comments); if (!entries.ok()) return entries.status(); bv->entries = std::move(entries).consume(); return ValuePtr(std::move(bv)); } -// ParseBody is entered just inside a `{` and leaves past its `}`. -StatusOr> Parser::ParseBody(int depth, Position* end) { +// ParseBody is entered just inside a `{` and leaves past its `}`. The +// comments after the last entry precede no entry, so no entry flushes them: +// they go to *trailing, taken before the `}` is consumed, since Advance would +// add the comments after it, which belong to what follows the block (#176). +StatusOr> Parser::ParseBody(int depth, + Position* end, + std::vector* trailing) { std::vector out; while (current_.kind != TokenKind::kRBrace && current_.kind != TokenKind::kEOF && !TolerantRowClose()) { @@ -714,6 +707,7 @@ StatusOr> Parser::ParseBody(int depth, Position* end) { auto entry = std::move(e).consume(); if (entry.has_value()) out.push_back(std::move(*entry)); } + *trailing = FlushComments(); if (current_.kind != TokenKind::kRBrace) { if (!tolerant_) { return Err(current_.pos, std::string("expected '}', got ") + TokenKindName(current_.kind)); @@ -926,6 +920,11 @@ StatusOr Parser::ParseTableDirective(int* end_offset) { // offset immediately past the closing `)` to *end_offset. StatusOr Parser::ParseTableRow(int expected, int* end_offset, bool read_past) { Position pos = current_.pos; + // The comments pending at the `(` and those inside the row are the row's + // leading_comments (#176). Left pending, they would pass to the next row or + // directive, or to the document's end. DatasetReader's row parser calls + // this too, so it files them as Parse does. + std::vector leading = FlushComments(); Advance(); // consume ( DatasetRow row; @@ -948,6 +947,13 @@ StatusOr Parser::ParseTableRow(int expected, int* end_offset, bool r std::string("expected ',' or ')' in @dataset row, got ") + DatasetTokenMsg()); } int eo = current_.pos.offset + 1; + // Taken before the `)` is consumed: Advance would add the comments after + // it, which belong to what follows the row. + std::vector inside = FlushComments(); + leading.insert(leading.end(), + std::make_move_iterator(inside.begin()), + std::make_move_iterator(inside.end())); + row.leading_comments = std::move(leading); // Consume the `)`, unless the caller reads nothing after the row: lexing on // would read the comments after it for nothing. if (read_past) Advance(); @@ -1187,7 +1193,7 @@ StatusOr ParseDatasetRow(std::string_view row, int max_bytes_literal, Position start, int* end_offset) { - Parser p(Parser::DropLeadingCommentsTag{}, row, max_depth, max_bytes_literal, start); + Parser p(row, max_depth, max_bytes_literal, start); if (p.current().kind != TokenKind::kLParen) { // Parse reports any token here, a lexical error included, as a // top-level entry beside the dataset. diff --git a/src/pxf/rewriter.cc b/src/pxf/rewriter.cc index 4728d7e..77673bc 100644 --- a/src/pxf/rewriter.cc +++ b/src/pxf/rewriter.cc @@ -88,6 +88,7 @@ EntryPtr CloneEntry(const EntryPtr& e) { n->name_quoted = b->name_quoted; n->entries = CloneEntries(b->entries); n->leading_comments = b->leading_comments; + n->trailing_comments = b->trailing_comments; return n; } return std::make_unique(*As(e)); @@ -115,6 +116,7 @@ ValuePtr CloneValue(const ValuePtr& v) { n->pos = b->pos; n->end = b->end; n->entries = CloneEntries(b->entries); + n->trailing_comments = b->trailing_comments; return n; } return std::visit( diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b609a89..8296dd6 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -38,7 +38,7 @@ if(TARGET protowire_pxf) pxf_null_test pxf_bignum_test pxf_features_test pxf_any_test pxf_fast_test pxf_full_roundtrip_test pxf_escapes_test pxf_annotations_test pxf_format_test pxf_tolerant_test pxf_check_test pxf_directive_test - pxf_schema_test pxf_schema_rules_test pxf_secret_test pxf_element_defaults_test pxf_result_directives_test pxf_dataset_reader_test pxf_dataset_scan_options_test + pxf_schema_test pxf_schema_rules_test pxf_secret_test pxf_element_defaults_test pxf_result_directives_test pxf_dataset_reader_test pxf_dataset_scan_options_test pxf_closing_comments_test pxf_proto_directive_test pxf_map_keys_test pxf_keyed_test pxf_hardening_test pxf_go_parity_test pxf_dataset_writer_test pxf_rewriter_test pxf_wkt_shape_test pxf_nan_test pxf_format_directives_test pxf_duration_block_test diff --git a/test/pxf_closing_comments_test.cc b/test/pxf_closing_comments_test.cc new file mode 100644 index 0000000..e048ac7 --- /dev/null +++ b/test/pxf_closing_comments_test.cc @@ -0,0 +1,202 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 TrendVidia, LLC. +// +// Comments that precede no entry — at the end of a document or of a block, +// and around dataset rows — survive Parse → FormatDocument in place (#176), +// as they do in protowire-go (#279, PR #283). Before, the parser attached a +// comment only to the entry after it, so a comment with none after it was +// dropped, or carried out of its block onto the next entry. The documents +// and expected outputs are protowire-go's (format_comments_test.go). + +#include "protowire/pxf/dataset_reader.h" +#include "protowire/pxf/format.h" +#include "protowire/pxf/parser.h" + +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace { + +namespace pxf = protowire::pxf; + +// FormatRoundTrip parses src and formats it, and requires the output to parse +// and to be a fixed point of formatting. +std::string FormatRoundTrip(const std::string& src) { + auto doc = pxf::Parse(src); + EXPECT_TRUE(doc.ok()) << doc.status().ToString() << "\n" << src; + if (!doc.ok()) return ""; + std::string out = pxf::FormatDocument(*doc); + auto again = pxf::Parse(out); + EXPECT_TRUE(again.ok()) << again.status().ToString() << "\n" << out; + if (!again.ok()) return out; + EXPECT_EQ(out, pxf::FormatDocument(*again)) << "formatting is not a fixed point"; + return out; +} + +std::vector Texts(const std::vector& cs) { + std::vector out; + out.reserve(cs.size()); + for (const auto& c : cs) out.push_back(c.text); + return out; +} + +TEST(ClosingComments, AtTheEnd) { + struct Case { + const char* name; + std::string src; + std::string want; // empty: the source itself + }; + const std::vector cases = { + // protowire-go#279's three documents. + {"after an entry", + "@type a.v1.M\nname = \"n\"\n# end of doc\n", + "@type a.v1.M\n\nname = \"n\"\n# end of doc\n"}, + {"after a directive", "@type a.v1.M\n@flag\n# end of doc\n", ""}, + {"after a row", "@dataset t.v1.T (id)\n(1)\n# end of doc\n", ""}, + {"a run of comments", + "@type a.v1.M\nname = \"n\" # inline\n# a\n// b\n/* c */\n", + "@type a.v1.M\n\nname = \"n\" # inline\n# a\n// b\n/* c */\n"}, + {"after @type alone", "@type a.v1.M\n# only\n", "@type a.v1.M\n\n# only\n"}, + }; + for (const auto& c : cases) { + EXPECT_EQ(FormatRoundTrip(c.src), c.want.empty() ? c.src : c.want) << c.name; + } + + auto doc = pxf::Parse("@type a.v1.M\nname = \"n\"\n# end of doc\n"); + ASSERT_TRUE(doc.ok()) << doc.status().ToString(); + EXPECT_EQ(Texts(doc->trailing_comments), std::vector{"# end of doc"}); + const auto* a = std::get_if>(&doc->entries[0]); + ASSERT_NE(a, nullptr); + EXPECT_TRUE((*a)->leading_comments.empty()); +} + +TEST(ClosingComments, BeforeTheClosingBrace) { + const std::vector> cases = { + // Before, this comment moved out of m, onto y. + {"block then entry", "@type a.v1.M\n\nm {\n k: 1\n # end of m\n}\ny = 2\n"}, + {"last block", "@type a.v1.M\n\nb {\n x = 1\n # end of b\n}\n"}, + {"empty block", "@type a.v1.M\n\nb {\n # empty\n}\n"}, + {"block value", "@type a.v1.M\n\nb = {\n x = 1\n # end of b\n}\n"}, + {"nested", "@type a.v1.M\n\na {\n b {\n x = 1\n # end of b\n }\n # end of a\n}\n"}, + {"after the brace stays after", "@type a.v1.M\n\na {\n x = 1\n}\n# about y\ny = 2\n"}, + {"block cell", "@dataset t.v1.T (id, m)\n(1, {\n x = 1\n # end of m\n})\n"}, + }; + for (const auto& [name, src] : cases) EXPECT_EQ(FormatRoundTrip(src), src) << name; + + auto doc = pxf::Parse("@type a.v1.M\nm {\n k: 1\n # end of m\n}\ny = 2\n"); + ASSERT_TRUE(doc.ok()) << doc.status().ToString(); + const auto* m = std::get_if>(&doc->entries[0]); + ASSERT_NE(m, nullptr); + EXPECT_EQ(Texts((*m)->trailing_comments), std::vector{"# end of m"}); + const auto* y = std::get_if>(&doc->entries[1]); + ASSERT_NE(y, nullptr); + EXPECT_TRUE((*y)->leading_comments.empty()); +} + +TEST(ClosingComments, DatasetRows) { + const std::string src = "@dataset t.v1.T (id)\n# first\n(1)\n# between\n(2)\n# end\n"; + EXPECT_EQ(FormatRoundTrip(src), src); + + // A row is written on one line, so a comment inside it moves before it. + EXPECT_EQ(FormatRoundTrip("@dataset t.v1.T (a, b)\n(1, # one\n 2)\n(3, 4)\n"), + "@dataset t.v1.T (a, b)\n# one\n(1, 2)\n(3, 4)\n"); + + auto doc = pxf::Parse(src); + ASSERT_TRUE(doc.ok()) << doc.status().ToString(); + const auto& rows = doc->datasets[0].rows; + ASSERT_EQ(rows.size(), 2u); + EXPECT_EQ(Texts(rows[0].leading_comments), std::vector{"# first"}); + EXPECT_EQ(Texts(rows[1].leading_comments), std::vector{"# between"}); + EXPECT_EQ(Texts(doc->trailing_comments), std::vector{"# end"}); +} + +// OneByteBuf serves its bytes one per read, so every token can be cut by a +// read's end. +class OneByteBuf : public std::streambuf { + public: + explicit OneByteBuf(std::string data) : data_(std::move(data)) {} + + protected: + int_type underflow() override { + if (gptr() != nullptr && gptr() < egptr()) return traits_type::to_int_type(*gptr()); + if (next_ >= data_.size()) return traits_type::eof(); + char* at = &data_[next_++]; + setg(at, at, at + 1); + return traits_type::to_int_type(*at); + } + + private: + std::string data_; + size_t next_ = 0; +}; + +struct RowComments { + pxf::Position pos; + std::vector comments; +}; + +std::vector StreamRows(std::istream* in) { + std::vector out; + auto tr = pxf::DatasetReader::Create(in); + EXPECT_TRUE(tr.ok()) << tr.status().ToString(); + if (!tr.ok()) return out; + for (;;) { + pxf::DatasetRow row; + auto st = (*tr)->Next(&row); + EXPECT_TRUE(st.ok()) << st.ToString(); + if (!st.ok() || (*tr)->Done()) break; + out.push_back({row.pos, row.leading_comments}); + } + return out; +} + +void ExpectSameComments(const std::vector& got, + const std::vector& want, + const std::string& how) { + ASSERT_EQ(got.size(), want.size()) << how; + for (size_t i = 0; i < want.size(); ++i) { + EXPECT_EQ(got[i].pos.offset, want[i].pos.offset) << how << " row " << i; + ASSERT_EQ(got[i].comments.size(), want[i].leading_comments.size()) << how << " row " << i; + for (size_t j = 0; j < want[i].leading_comments.size(); ++j) { + const auto& g = got[i].comments[j]; + const auto& w = want[i].leading_comments[j]; + EXPECT_EQ(g.text, w.text) << how << " row " << i; + EXPECT_EQ(g.pos.line, w.pos.line) << how << " row " << i; + EXPECT_EQ(g.pos.column, w.pos.column) << how << " row " << i; + EXPECT_EQ(g.pos.offset, w.pos.offset) << how << " row " << i; + } + } +} + +// DatasetReader files a row's comments as Parse does, read whole or a byte +// at a time. +TEST(ClosingComments, DatasetReaderRowComments) { + for (const char* body : {"# first\n(1, 2)\n# between\n/* b */\n(3, 4)\n# end\n", + "(1, # one\n 2)\n(3, 4) # after\n(5, 6)\n", + "(1, 2)\n# about U\n@dataset U (z)\n(9)\n"}) { + const std::string in = std::string("@dataset T (a, b)\n") + body; + auto doc = pxf::Parse(in); + ASSERT_TRUE(doc.ok()) << doc.status().ToString(); + const auto& want = doc->datasets[0].rows; + + std::istringstream whole(in); + ExpectSameComments(StreamRows(&whole), want, std::string("whole: ") + body); + OneByteBuf buf(in); + std::istream bytes(&buf); + ExpectSameComments(StreamRows(&bytes), want, std::string("one byte: ") + body); + } + + std::istringstream src("@dataset T (a, b)\n(1, 2)\n# between\n(3, 4)\n"); + auto rows = StreamRows(&src); + ASSERT_EQ(rows.size(), 2u); + EXPECT_EQ(Texts(rows[1].comments), std::vector{"# between"}); +} + +} // namespace diff --git a/test/pxf_keyed_test.cc b/test/pxf_keyed_test.cc index 6800c69..a6a240f 100644 --- a/test/pxf_keyed_test.cc +++ b/test/pxf_keyed_test.cc @@ -517,4 +517,39 @@ TEST_F(Keyed, DiagnosticsToleratesNullDescriptor) { EXPECT_TRUE(protowire::pxf::KeyedDiagnostics(doc.document, nullptr).empty()); } +// CanonicalizeKeyed keeps a block's closing comments when it rewrites the +// block, and the comments of a redundant key assignment it drops last in a +// block (#176; protowire-go's TestCanonicalizeKeyed_KeepsClosingComments). +TEST_F(Keyed, CanonicalizeKeepsClosingComments) { + struct Case { + const char* name; + std::string src; + std::string want; + }; + const std::vector cases = { + {"dropped last key assignment", + "@type keyed.v1.Node\n\nchildren {\n greeting {\n type = \"Label\"\n # about the " + "key\n id = \"greeting\"\n }\n # end of children\n}\n", + "@type keyed.v1.Node\n\nchildren {\n greeting {\n type = \"Label\"\n # about the " + "key\n }\n # end of children\n}\n"}, + {"assignment form", + "@type keyed.v1.Node\n\nchildren = {\n greeting = {\n type = \"Label\"\n # end of " + "greeting\n }\n # end of children\n}\n", + "@type keyed.v1.Node\n\nchildren {\n greeting {\n type = \"Label\"\n # end of " + "greeting\n }\n # end of children\n}\n"}, + {"anonymous list", + "@type keyed.v1.Node\n\nchildren = [\n {\n type = \"L\"\n # about the key\n id = " + "\"g\"\n },\n {\n id = \"h\"\n # end of h\n }\n]\n", + "@type keyed.v1.Node\n\nchildren {\n g {\n type = \"L\"\n # about the key\n }\n h " + "{\n # end of h\n }\n}\n"}, + }; + for (const auto& c : cases) { + const pb::Descriptor* d = DescOf(c.src); + ASSERT_NE(d, nullptr) << c.name; + const std::string out = Fmt(c.src, d); + EXPECT_EQ(out, c.want) << c.name; + EXPECT_EQ(Fmt(out, d), out) << c.name << ": formatting is not a fixed point"; + } +} + } // namespace diff --git a/tools/godiff/main.go b/tools/godiff/main.go index 11f6fb2..18b0c9b 100644 --- a/tools/godiff/main.go +++ b/tools/godiff/main.go @@ -317,9 +317,15 @@ type astDump struct{ w *bufio.Writer } func pos(p pxf.Position) string { return fmt.Sprintf("%d:%d:%d", p.Line, p.Column, p.Offset) } -func (d astDump) comments(ind string, cs []pxf.Comment) { +func (d astDump) comments(ind string, cs []pxf.Comment) { d.labeled(ind, "comment", cs) } + +// trailing writes the comments that follow a block's or document's last +// entry (TrailingComments, protowire-go#279). +func (d astDump) trailing(ind string, cs []pxf.Comment) { d.labeled(ind, "trailing", cs) } + +func (d astDump) labeled(ind, label string, cs []pxf.Comment) { for _, c := range cs { - fmt.Fprintf(d.w, "%scomment %s %s\n", ind, pos(c.Pos), quote(c.Text)) + fmt.Fprintf(d.w, "%s%s %s %s\n", ind, label, pos(c.Pos), quote(c.Text)) } } @@ -355,6 +361,7 @@ func (d astDump) value(ind string, v pxf.Value) { case *pxf.BlockVal: fmt.Fprintf(d.w, "blockval\n") d.entries(ind+" ", v.Entries) + d.trailing(ind+" ", v.TrailingComments) } } @@ -378,6 +385,7 @@ func (d astDump) entries(ind string, es []pxf.Entry) { fmt.Fprintf(d.w, "block %s %v\n", quote(e.Name), e.NameQuoted) d.comments(ind, e.LeadingComments) d.entries(ind+" ", e.Entries) + d.trailing(ind+" ", e.TrailingComments) } } } @@ -399,6 +407,7 @@ func (d astDump) document(doc *pxf.Document) { d.comments("", x.LeadingComments) for _, r := range x.Rows { fmt.Fprintf(d.w, "row %s\n", pos(r.Pos)) + d.comments(" ", r.LeadingComments) for _, c := range r.Cells { if c == nil { fmt.Fprintf(d.w, " empty\n") @@ -409,6 +418,7 @@ func (d astDump) document(doc *pxf.Document) { } } d.entries("", doc.Entries) + d.trailing("", doc.TrailingComments) } // defaultTable is the same list in cmd/pxf_diff. The literals are a schema's,