From 6ff1a9c1af72a9ba96df16febf5f092c2e1aabcc Mon Sep 17 00:00:00 2001 From: Meezaan-ud-Din A Wal-Ikram Date: Tue, 15 Sep 2026 09:52:38 +0400 Subject: [PATCH] fix(document): preserve comments, plain-scalar leaders, comment columns and line endings on round trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects that each rewrote part of a document the author did not write that way, so a load/emit cycle was not byte-identical. All four are in the trivia and formatting paths; none affects the data model. 1. Multi-byte characters were truncated in comments and directives. The scanner advances by whole UTF-8 characters, but the trivia accumulators appended a single byte, so every continuation byte was dropped: "# see §4.3" was collected as "# see \xC24.3". The orphaned lead byte renders as U+FFFD, which made it look like character substitution rather than truncation. currentChar() already existed for exactly this, and its docblock says so - it was used for scalar content but not for trivia. Seven call sites. 2. "?", ":" and "-" were treated as unconditionally reserved plain-scalar leaders. YAML 1.2 ns-plain-first admits them when followed by a plain-safe character; they are indicators only when standing alone or preceding a space. So "--cert-dir=/tmp" and "-single-dash" are valid plain scalars, and quoting them on emit changes the document's text. Values that would reparse as a non-string ("-5") are still quoted by the existing check. 3. Standalone comments were re-indented to their surrounding container. The emitter already honoured CommentNode::getIndent() and the scanner already captured each comment's column, but the parser passed a hardcoded 0. The indent is now nullable rather than zero-defaulted, because column 1 is a real position: a flush-left comment inside an indented block is a deliberate style that "0 means unset" could not express. A programmatically created CommentNode has no recorded column and still follows its container. 4. CRLF documents were rewritten with LF. The scanner normalises CRLF to LF so the rest of the parser sees one line ending, and the emitter already writes whatever YamlStream::getLineEnding() names - but nothing ever called setLineEnding(), so the ending the source used was discarded. Pipeline::parse() already derives setTrailingNewline() from the raw source and now derives the line ending the same way. It is set only when every newline in the source is CRLF. A file with mixed endings has no single ending to preserve, and quietly picking one would rewrite the other. Raw source needed the same treatment: it is captured after normalisation, so a block scalar re-emitted verbatim carried LF into a CRLF document. It now goes through a helper that rewrites newlines to the document's ending, which is a no-op for LF. Three of the four are omissions rather than disagreements: currentChar() was written for exactly this truncation, CommentNode::getIndent() was already honoured by the emitter, and YamlStream::getLineEnding() was already read by it. In each case the mechanism existed and nothing wired it up. Only 2 changes behaviour the code had deliberately chosen. Tests: 32 cases across four files, covering Latin-1, accented Latin, Arabic, CJK, emoji, end-of-line and indented comments; plain and genuinely-indicator leaders; comment columns above, below and matching their siblings; and CRLF with indentation, comments, sequences and block scalars. 21 of the 32 fail without these changes. The existing suite is unchanged at 1022 tests with the same single pre-existing error and failure (ConformanceTest fixtures and a PHPUnit 12 data-provider arity issue), neither related to these changes. --- src/Document/Emitter/Emitter.php | 57 +++++++++++++++--- src/Document/Node/CommentNode.php | 8 +-- src/Document/Parser/Parser.php | 8 +-- src/Document/Parser/Pipeline.php | 13 ++++ src/Document/Parser/Scanner.php | 14 ++--- test/unit/Document/CommentIndentTest.php | 54 +++++++++++++++++ test/unit/Document/LineEndingTest.php | 53 +++++++++++++++++ test/unit/Document/MultibyteCommentTest.php | 59 +++++++++++++++++++ test/unit/Document/PlainScalarLeaderTest.php | 62 ++++++++++++++++++++ 9 files changed, 305 insertions(+), 23 deletions(-) create mode 100644 test/unit/Document/CommentIndentTest.php create mode 100644 test/unit/Document/LineEndingTest.php create mode 100644 test/unit/Document/MultibyteCommentTest.php create mode 100644 test/unit/Document/PlainScalarLeaderTest.php diff --git a/src/Document/Emitter/Emitter.php b/src/Document/Emitter/Emitter.php index 76853db..3fd6981 100644 --- a/src/Document/Emitter/Emitter.php +++ b/src/Document/Emitter/Emitter.php @@ -175,7 +175,7 @@ private function emitScalar(ScalarNode $node): void { $properties = $this->renderValueProperties($node); if ($this->isBlockStyleScalar($node)) { - $raw = $node->getRawSource(); + $raw = $this->rawSource($node); if ($raw !== null) { $this->buffer .= $properties . $raw; } else { @@ -322,7 +322,7 @@ private function emitMapEntry(MapEntry $entry, int $indent): void // Block scalars span multiple lines and end with their own // newline. if ($this->isBlockStyleScalar($value)) { - $rawSource = $value->getRawSource(); + $rawSource = $this->rawSource($value); if ($rawSource !== null) { $this->buffer .= ' ' . $properties . $rawSource; } else { @@ -623,14 +623,42 @@ private function emitSequenceItem(SequenceItem $item, int $indent): void private function emitStandaloneComment(CommentNode $comment, int $indent): void { - // Use the comment's stored indent if it has one; otherwise - // the surrounding container's indent. - $useIndent = $comment->getIndent() > 0 ? $comment->getIndent() : $indent; + // Use the comment's own column when the parser recorded one, so a + // comment comes back where its author put it. null means no column was + // recorded - a comment built programmatically rather than read from a + // document - and those follow the surrounding container. + // + // Nullable rather than `> 0`: column 1 is a real position, and a + // flush-left comment inside an indented block is a deliberate style + // that was previously indistinguishable from "unset". + $useIndent = $comment->getIndent() ?? $indent; $this->buffer .= str_repeat(' ', $useIndent); $this->buffer .= $comment->getText(); $this->buffer .= $this->lineEnding; } + /** + * A node's raw source, in the line ending this document uses. + * + * Raw source is captured after the scanner has normalised CRLF to LF, so a + * multi-line scalar re-emitted verbatim carried LF into a CRLF document - + * a block scalar was the one construct that still broke a Windows file's + * round trip after the stream learned its line ending. + * + * A no-op for LF documents, which is every document until one says + * otherwise. + */ + private function rawSource(ScalarNode $node): ?string + { + $raw = $node->getRawSource(); + + if ($raw === null || $this->lineEnding === "\n") { + return $raw; + } + + return str_replace("\n", $this->lineEnding, $raw); + } + private function emitBlankLines(BlankLineNode $node): void { // count() blank lines = count() newline characters since the @@ -640,7 +668,7 @@ private function emitBlankLines(BlankLineNode $node): void private function scalarOrRaw(ScalarNode $node): string { - $rawSource = $node->getRawSource(); + $rawSource = $this->rawSource($node); if ($rawSource !== null) { return $rawSource; } @@ -817,7 +845,7 @@ private function emitBlockScalarSynthesized(ScalarNode $node, int $parentIndent) private function renderBlockScalar(ScalarNode $node, int $parentIndent): string { - $rawSource = $node->getRawSource(); + $rawSource = $this->rawSource($node); if ($rawSource !== null) { return $rawSource; } @@ -1003,10 +1031,23 @@ private function isPlainSafe(string $value, ?string $tag = null): bool return false; } $first = $value[0]; - $reservedLeaders = ['[', ']', '{', '}', '#', '&', '*', '!', '|', '>', "'", '"', '%', '@', '`', '?', ':', '-', ',', "\t", ' ']; + $reservedLeaders = ['[', ']', '{', '}', '#', '&', '*', '!', '|', '>', "'", '"', '%', '@', '`', ',', "\t", ' ']; if (in_array($first, $reservedLeaders, true)) { return false; } + // "?", ":" and "-" are indicators only when they stand alone or are + // followed by a space. YAML 1.2 ns-plain-first admits them otherwise, + // so "--cert-dir=/tmp" and "-single-dash" are valid plain scalars and + // quoting them rewrites a line the author did not write that way. + if (in_array($first, ['?', ':', '-'], true)) { + if (strlen($value) === 1) { + return false; + } + $second = $value[1]; + if ($second === ' ' || $second === "\t") { + return false; + } + } if (str_contains($value, ': ') || str_contains($value, " #")) { return false; } diff --git a/src/Document/Node/CommentNode.php b/src/Document/Node/CommentNode.php index 210e608..129fea7 100644 --- a/src/Document/Node/CommentNode.php +++ b/src/Document/Node/CommentNode.php @@ -34,7 +34,7 @@ final class CommentNode implements Node use NodeTrait; private string $text; - private int $indent; + private ?int $indent; private string $gap; /** @@ -50,7 +50,7 @@ final class CommentNode implements Node * Empty for synthesized nodes; the emitter falls * back to a sensible default when emitting. */ - public function __construct(string $text = '', int $indent = 0, string $gap = '') + public function __construct(string $text = '', ?int $indent = null, string $gap = '') { $this->text = $text; $this->indent = $indent; @@ -67,12 +67,12 @@ public function setText(string $text): void $this->text = $text; } - public function getIndent(): int + public function getIndent(): ?int { return $this->indent; } - public function setIndent(int $indent): void + public function setIndent(?int $indent): void { $this->indent = $indent; } diff --git a/src/Document/Parser/Parser.php b/src/Document/Parser/Parser.php index a481e2b..64b248b 100644 --- a/src/Document/Parser/Parser.php +++ b/src/Document/Parser/Parser.php @@ -589,7 +589,7 @@ private function migrateTrivia(array $trivia, MapNode|SequenceNode $container): { foreach ($trivia as $t) { if ($t->type === TriviaType::Comment) { - $node = new CommentNode($t->text, 0, $t->gap); + $node = new CommentNode($t->text, max(0, $t->column - 1), $t->gap); $node->setPosition($t->line, $t->column); $container->appendChildInternal($node); continue; @@ -652,7 +652,7 @@ private function drainTriviaIntoDoc(array $trivia, YamlDocument $doc): void private function triviaToNode(TriviaToken $t): CommentNode|BlankLineNode|null { if ($t->type === TriviaType::Comment) { - $node = new CommentNode($t->text, 0, $t->gap); + $node = new CommentNode($t->text, max(0, $t->column - 1), $t->gap); $node->setPosition($t->line, $t->column); return $node; } @@ -885,7 +885,7 @@ private function extractEolCommentFromTrailingTrivia(Token $token): ?CommentNode { foreach ($token->trailingTrivia as $t) { if ($t->type === TriviaType::Comment) { - $node = new CommentNode($t->text, 0, $t->gap); + $node = new CommentNode($t->text, max(0, $t->column - 1), $t->gap); $node->setPosition($t->line, $t->column); return $node; } @@ -955,7 +955,7 @@ private function parseBlockMapEntry(): MapEntry $first->type === TriviaType::Comment && $first->line === $valueToken->line ) { - $eolComment = new CommentNode($first->text, 0, $first->gap); + $eolComment = new CommentNode($first->text, max(0, $first->column - 1), $first->gap); $eolComment->setPosition($first->line, $first->column); // Strip this one trivia entry from the inner token so // it isn't re-emitted as a child of the nested value. diff --git a/src/Document/Parser/Pipeline.php b/src/Document/Parser/Pipeline.php index 168a833..a5155c4 100644 --- a/src/Document/Parser/Pipeline.php +++ b/src/Document/Parser/Pipeline.php @@ -61,6 +61,19 @@ public function parse(string $source): YamlStream if ($source !== '' && str_ends_with($source, "\n")) { $stream->setTrailingNewline(true); } + // Round-trip: the scanner normalises CRLF to LF so the rest of the + // parser sees one line ending, and the emitter already writes whatever + // the stream names - but nothing recorded what the source used, so a + // CRLF document was silently rewritten with LF. + // + // Set only when EVERY newline is CRLF. A file with mixed endings has no + // single ending to preserve, and quietly picking one would rewrite the + // other; LF leaves that document visibly changed rather than subtly, so + // a caller comparing input to output can still tell. + $lf = substr_count($source, "\n"); + if ($lf > 0 && substr_count($source, "\r\n") === $lf) { + $stream->setLineEnding("\r\n"); + } $this->resolver->resolve($stream); return $stream; diff --git a/src/Document/Parser/Scanner.php b/src/Document/Parser/Scanner.php index 6af6880..0b1e2f7 100644 --- a/src/Document/Parser/Scanner.php +++ b/src/Document/Parser/Scanner.php @@ -317,7 +317,7 @@ public function scan(string $source): array $commentColumn = $this->column; $text = ''; while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") { - $text .= $this->source[$this->pos]; + $text .= $this->currentChar(); $this->advance(); } $this->triviaBuffer[] = TriviaToken::comment( @@ -810,7 +810,7 @@ private function scanBlockSequenceItem(array &$tokens): void $commentColumn = $this->column; $text = ''; while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") { - $text .= $this->source[$this->pos]; + $text .= $this->currentChar(); $this->advance(); } $this->triviaBuffer[] = TriviaToken::comment( @@ -1746,7 +1746,7 @@ private function scanBlockMapEntry(array &$tokens): void $commentColumn = $this->column; $text = ''; while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") { - $text .= $this->source[$this->pos]; + $text .= $this->currentChar(); $this->advance(); } $this->triviaBuffer[] = TriviaToken::comment( @@ -1821,7 +1821,7 @@ private function scanBlockMapEntry(array &$tokens): void $commentColumn = $this->column; $text = ''; while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") { - $text .= $this->source[$this->pos]; + $text .= $this->currentChar(); $this->advance(); } $this->triviaBuffer[] = TriviaToken::comment( @@ -2079,7 +2079,7 @@ private function collectTrivia(): bool $startColumn = $this->column; $text = ''; while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") { - $text .= $this->source[$this->pos]; + $text .= $this->currentChar(); $this->advance(); } $this->triviaBuffer[] = TriviaToken::comment( @@ -2193,7 +2193,7 @@ private function consumeDirective(): Token $this->advance(); $value = ''; while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") { - $value .= $this->source[$this->pos]; + $value .= $this->currentChar(); $this->advance(); } if ($this->pos < $this->length && $this->source[$this->pos] === "\n") { @@ -3950,7 +3950,7 @@ private function collectTrailingEolTrivia(): array $startColumn = $this->column; $text = ''; while ($this->pos < $this->length && $this->source[$this->pos] !== "\n") { - $text .= $this->source[$this->pos]; + $text .= $this->currentChar(); $this->advance(); } $trailing[] = TriviaToken::comment($text, $startLine, $startColumn, $gap); diff --git a/test/unit/Document/CommentIndentTest.php b/test/unit/Document/CommentIndentTest.php new file mode 100644 index 0000000..3782228 --- /dev/null +++ b/test/unit/Document/CommentIndentTest.php @@ -0,0 +1,54 @@ + */ + public static function comments(): array + { + return [ + 'matching its sibling' => ["parent:\n # aligned\n key: 1\n"], + 'deeper than its sibling'=> ["parent:\n child:\n # over-indented\n key: 1\n"], + 'shallower' => ["parent:\n # four spaces\n key: 1\n"], + 'flush left in a block' => ["parent:\n# flush left\n key: 1\n"], + 'before a nested block' => ["a:\n b:\n # note\n c: 1\n"], + ]; + } + + #[DataProvider('comments')] + public function testCommentsKeepTheirColumn(string $source): void + { + self::assertSame($source, (new Emitter())->emit((new YamlStringLoader())->load($source))); + } + + public function testAProgrammaticCommentStillFollowsItsContainer(): void + { + // No column was ever recorded for it, so it indents with the block it + // is added to rather than landing at column 1. + $stream = (new YamlStringLoader())->load("parent:\n key: 1\n"); + $parent = $stream->getDocument(0)->root()->entry('parent')->getValue(); + $parent->appendChildInternal(new CommentNode('# added later')); + + self::assertStringContainsString(" # added later", (new Emitter())->emit($stream)); + } +} diff --git a/test/unit/Document/LineEndingTest.php b/test/unit/Document/LineEndingTest.php new file mode 100644 index 0000000..1ed38c7 --- /dev/null +++ b/test/unit/Document/LineEndingTest.php @@ -0,0 +1,53 @@ + */ + public static function documents(): array + { + return [ + 'crlf' => ["# note\r\na:\r\n b: 1\r\n"], + 'lf' => ["# note\na:\n b: 1\n"], + 'crlf with indentation' => ["a:\r\n b:\r\n c: 1\r\n"], + 'crlf with a comment column' => ["a:\r\n # deep\r\n b: 1\r\n"], + 'crlf with a block scalar' => ["a: |\r\n line one\r\n line two\r\n"], + 'crlf with a sequence' => ["a:\r\n - one\r\n - two\r\n"], + ]; + } + + #[DataProvider('documents')] + public function testLineEndingsRoundTrip(string $source): void + { + self::assertSame($source, (new Emitter())->emit((new YamlStringLoader())->load($source))); + } + + public function testTheStreamReportsTheEndingItRead(): void + { + self::assertSame("\r\n", (new YamlStringLoader())->load("a: 1\r\n")->getLineEnding()); + self::assertSame("\n", (new YamlStringLoader())->load("a: 1\n")->getLineEnding()); + } + + public function testAMixedDocumentIsNotGuessedAt(): void + { + // No single ending to preserve. LF leaves it visibly changed rather + // than subtly, so a caller comparing input to output can still tell. + self::assertSame("\n", (new YamlStringLoader())->load("a:\r\n b: 1\n")->getLineEnding()); + } +} diff --git a/test/unit/Document/MultibyteCommentTest.php b/test/unit/Document/MultibyteCommentTest.php new file mode 100644 index 0000000..80071d9 --- /dev/null +++ b/test/unit/Document/MultibyteCommentTest.php @@ -0,0 +1,59 @@ + */ + public static function nonAsciiComments(): array + { + return [ + 'latin-1 supplement' => ["# see §4.3\nkey: value\n"], + 'accented latin' => ["# café rollout\nkey: value\n"], + 'arabic' => ["# مملوك\nkey: value\n"], + 'cjk' => ["# 設定\nkey: value\n"], + 'emoji (4-byte)' => ["# ship it 🚀\nkey: value\n"], + 'eol comment' => ["key: value # ° degrees\n"], + 'indented comment' => ["parent:\n # é\n child: 1\n"], + ]; + } + + #[DataProvider('nonAsciiComments')] + public function testNonAsciiCommentsRoundTripByteIdentically(string $source): void + { + $stream = (new YamlStringLoader())->load($source); + + self::assertSame($source, (new Emitter())->emit($stream)); + } + + public function testTheCommentTextItselfIsIntact(): void + { + $stream = (new YamlStringLoader())->load("# see §4.3 café\nkey: value\n"); + + $texts = []; + foreach ($stream->getDocument(0)->root()->children() as $child) { + if (method_exists($child, 'getText')) { + $texts[] = $child->getText(); + } + } + + self::assertContains('# see §4.3 café', $texts); + } +} diff --git a/test/unit/Document/PlainScalarLeaderTest.php b/test/unit/Document/PlainScalarLeaderTest.php new file mode 100644 index 0000000..39f45c5 --- /dev/null +++ b/test/unit/Document/PlainScalarLeaderTest.php @@ -0,0 +1,62 @@ + */ + public static function plainScalars(): array + { + return [ + 'double dash flag' => ["key: --cert-dir=/tmp\n"], + 'single dash' => ["key: -single-dash\n"], + 'in a sequence' => ["args:\n - --kubelet-insecure-tls\n - --metric-resolution=15s\n"], + 'question leader' => ["key: ?query=1\n"], + 'colon leader' => ["key: ::1\n"], + ]; + } + + #[DataProvider('plainScalars')] + public function testPlainLeadersRoundTripUnquoted(string $source): void + { + self::assertSame($source, (new Emitter())->emit((new YamlStringLoader())->load($source))); + } + + /** @return array */ + public static function stillQuoted(): array + { + return [ + 'bare dash' => ["key: '-'\n"], + 'dash space' => ["key: '- not a sequence'\n"], + 'negative number'=> ["key: '-5'\n"], + 'bare colon' => ["key: ':'\n"], + 'bare question' => ["key: '?'\n"], + ]; + } + + /** + * The relaxation must not go too far: a leader that really is an indicator, + * or a value that would reparse as a non-string, still needs its quotes. + */ + #[DataProvider('stillQuoted')] + public function testGenuineIndicatorsKeepTheirQuotes(string $source): void + { + self::assertSame($source, (new Emitter())->emit((new YamlStringLoader())->load($source))); + } +}