Fix multiline scalar serialization round-trips by injeniero · Pull Request #414 · yaml/go-yaml

injeniero · 2026-09-07T13:32:11Z

Problem

Marshal can emit multiline scalars that Unmarshal then rejects with did not find expected key. Three defects in the same class are addressed here; all reproduce on a plain Marshal → Unmarshal round-trip, with no explicit style set.

// 1) leading empty line + indented first content line  (issue #76)
yaml.Marshal(map[string]string{"k": "\n  a\n b\n"})

// 2) multiline string inside a block sequence  (issue #399)
yaml.Marshal([]any{"  a\nb\n"})

// 3) multiline scalar whose first content line starts with a tab  (issue #383)
yaml.Marshal(map[string]string{"k": "\tthis\nis\nmultiline"})

Cause and fix

All three live in the block-scalar path:

Missing indentation indicator. writeBlockScalarHints emitted the indicator only when the first byte was a space, so a scalar starting with empty lines then an indented line got none, and the parser inferred the wrong indentation. Fix: skip leading line breaks, then test for a space.
Wrong indicator value. The indicator was always BestIndent, but per the spec it is relative to the parent node; a block sequence item only adds two columns, so BestIndent overstated it. Fix: derive the indicator from the content's indentation relative to the parent, falling back to BestIndent when outside the 1..9 range.
Leading-tab scalars. A multiline value whose first content line begins with a tab was emitted as a literal block, but block indentation is spaces only, so it never parsed back. Fix: such values are emitted double-quoted (shouldUseLiteralStyle returns false; the serializer falls back to double-quoted for multiline values that are not literal-eligible).

The #65 behavior is preserved: a leading blank line followed by unindented content still gets no indicator.

Tests

TestBlockScalarIndentIndicatorRoundTrip and TestTabLeadingScalarRoundTrip cover the round-trips across SetIndent values and nesting.
Added roundtrip/encode/should-literal fixtures in the testdata files.
Existing TestScalarStyleWithTabs expectations updated to the corrected double-quoted output.
Full suite green including the yaml-test-suite (e.g. F6MC).

Closes #76
Closes #399
Closes #383
Also fixes go-yaml/yaml#1071. Supersedes #393, #396, #388.

Copilot

🟡 Changes recommended

writeBlockScalarHints can still emit syntactically invalid indentation indicators when Encoder.SetIndent is set outside the YAML 1..9 indicator range, and this path is directly exercised/expanded by the new indicator logic.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes several block-scalar serialization cases where Marshal could emit YAML that Unmarshal rejects on a round-trip, primarily by correcting when/what indentation indicators are emitted and by avoiding literal block scalars for multiline values that begin with a tab.

Changes:

Adjusts block-scalar hint emission to (a) skip leading empty lines when deciding whether an indentation indicator is required, and (b) compute the indicator relative to the parent indentation.
Changes default multiline scalar style selection to use literal style only when eligible; otherwise emits double-quoted scalars (notably for leading-tab multiline values).
Adds/updates regression tests and testdata fixtures covering these round-trip cases and updated tab-handling expectations.

File summaries

File	Description
`internal/libyaml/emitter.go`	Fixes indentation indicator emission logic for block scalars (skip leading breaks; compute indicator relative to parent).
`internal/libyaml/serializer.go`	Updates default scalar style selection to avoid non-roundtrippable block scalars and emit double-quoted for multiline non-literal-eligible strings.
`internal/libyaml/node.go`	Updates `shouldUseLiteralStyle` to reject multiline values whose first content character is a tab (after leading line breaks).
`yaml_test.go`	Adds new round-trip tests and updates tab-related scalar style expectations.
`testdata/encode.yaml` / `internal/libyaml/testdata/{emitter.yaml,node.yaml}`	Adds regression fixtures for round-trips and literal-style eligibility cases.

Review details

Files reviewed: 7/7 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Marshal could emit multiline scalars that Unmarshal then rejects with "did not find expected key". Three defects in the same class are fixed: 1. Block indentation indicator was emitted only when the first byte was a space, so a scalar starting with empty lines followed by an indented first content line got none. 2. When emitted, the indicator was always BestIndent, but the spec defines it relative to the parent node; a block sequence item only adds two columns, so BestIndent overstated the indentation. 3. A multiline scalar whose first content line begins with a tab was emitted as a literal block, but block indentation is spaces only, so the result did not parse. Such values are now double-quoted. For (1) and (2), skip leading line breaks before testing for a space, and derive the indicator from the content's indentation relative to the parent. The indicator must be a single digit 1..9, so it is clamped to that range and the content indentation is aligned to the clamped value, keeping output valid even when a caller requests a wider indent (e.g. SetIndent(10)). The issue yaml#65 behavior is preserved: a leading blank line followed by unindented content still gets no indicator. Fixes issue yaml#76, issue yaml#399, issue yaml#383, and go-yaml/yaml#1071.

injeniero · 2026-09-07T15:02:08Z

@ingydotnet Pinging you as the last committer on the repo. This PR fixes several critical bugs with the lib we found in real use, like not being able to serialize properly text with new lines.

I tried to address all present bugs and PRs that partially fix related issues in a single one. Please let me know how I can help to get this merged.

Thanks!!

ccoVeille · 2026-09-12T20:34:57Z

+// A multiline scalar whose first content line begins with a tab cannot be
+// expressed as a block scalar (block indentation is spaces only), so it must
+// be emitted double-quoted and still round-trip exactly.
+// See https://github.com/yaml/go-yaml/issues/383
+func TestTabLeadingScalarRoundTrip(t *testing.T) {
+	type doc struct {
+		Text string `yaml:"text"`
+	}
+
+	for _, text := range []string{
+		"\tthis\nis\nmultiline",
+		"\tB\n\tC\n",
+		"\n\tindented by tab\nregular",
+		"first\n\tsecond\n", // tab on a later line: stays literal
+	} {
+		in := doc{Text: text}
+		out, err := yaml.Marshal(&in)
+		assert.NoError(t, err)
+
+		var back doc
+		assert.NoErrorf(t, yaml.Unmarshal(out, &back),
+			"encoded as:\n%s", out)
+		assert.DeepEqualf(t, in, back, "encoded as:\n%s", out)
+	}
+}


Can't we express this in one of the .yaml test file ?

ccoVeille · 2026-09-12T20:37:21Z

+// A block scalar whose first non-empty line is indented needs an explicit
+// indentation indicator, and that indicator is relative to the parent node's
+// indentation, not BestIndent.
+// This covers both the missing-indicator case (a leading empty line followed
+// by indented content) and the wrong-value case (a scalar nested in a block
+// sequence item, which only adds two columns of indentation).
+// See https://github.com/yaml/go-yaml/issues/76
+// and https://github.com/go-yaml/yaml/issues/1071
+func TestBlockScalarIndentIndicatorRoundTrip(t *testing.T) {
+	type params struct {
+		Description string `yaml:"description"`
+	}
+	type spec struct {
+		Parameters []params `yaml:"parameters"`
+	}
+
+	// Includes indents at and beyond the 1..9 indicator range (9, 10) to
+	// cover clamping of the indentation indicator.
+	for _, indent := range []int{0, 1, 2, 3, 4, 8, 9, 10} {
+		for _, description := range []string{
+			"  a\nb",
+			"     a\nb",
+			" a\n      b",
+			"\n  indented\nregular",
+			"\n\n  more indented\nregular",
+		} {
+			in := spec{Parameters: []params{{Description: description}}}
+
+			var buf bytes.Buffer
+			enc := yaml.NewEncoder(&buf)
+			if indent > 0 {
+				enc.SetIndent(indent)
+			}
+			assert.NoError(t, enc.Encode(&in))
+			assert.NoError(t, enc.Close())
+
+			var out spec
+			err := yaml.Unmarshal(buf.Bytes(), &out)
+			assert.NoErrorf(
+				t, err, "indent %d, encoded as:\n%s", indent, buf.String())
+			assert.DeepEqualf(
+				t, in, out, "indent %d, encoded as:\n%s", indent, buf.String())
+		}
+	}
+}


I can see the loop on indentation, but could you try using the .yanl test file to validatw them.

The yaml test suite is intended to be reused on other projects using YAML.

We would appreciate

ccoVeille · 2026-09-12T20:40:57Z

+		case shouldUseLiteralStyle(value):
 			style = LITERAL_SCALAR_STYLE
+		case strings.Contains(value, "\n"):
+			// Multiline but not literal-eligible (e.g. first content line
+			// starts with a tab): a block scalar would not round-trip, so
+			// emit a double-quoted scalar instead.
+			style = DOUBLE_QUOTED_SCALAR_STYLE
 		case forceQuoting:
 			style = s.quotePreference.ScalarStyle()
 		}


I'm unsure I saw test for the priority of shoulduseLiteral or new line against forceQuoting ?

Do you think you could add tests for this?

ccoVeille · 2026-09-12T20:43:14Z

 		{
 			"\tThis starts with tab\nand is long enough\nfor literal style",
-			"|-\n    \tThis starts with tab\n    and is long enough\n    for literal style\n",
+			"\"\\tThis starts with tab\\nand is long enough\\nfor literal style\"\n",


Here and everwhere. Please use raw string literal for readability

Suggested change

"\"\\tThis starts with tab\\nand is long enough\\nfor literal style\"\n",

`"\tThis starts with tab\nand is long enough\nfor literal style"` + "\n",

ccoVeille · 2026-09-12T20:45:57Z

@silverwind

You had review another PR

#396 (comment)

Could you review this one, and let us know what you think about this one compared to the others?

Thanks

ccoVeille · 2026-09-12T20:49:10Z

+		parent_indent := 0
+		if len(emitter.indents) > 0 && emitter.indents[len(emitter.indents)-1] > 0 {
+			parent_indent = emitter.indents[len(emitter.indents)-1]
+		}


Please use what I had suggested in another PR

#393 (comment)

Suggested change

parent_indent := 0

if len(emitter.indents) > 0 && emitter.indents[len(emitter.indents)-1] > 0 {

parent_indent = emitter.indents[len(emitter.indents)-1]

}

parent_indent := 0

if n := len(emitter.indents); n > 0 && emitter.indents[n-1] > 0 {

parent_indent = emitter.indents[n-1]

}

ccoVeille · 2026-09-12T21:03:17Z

Also, may I know your motivation behind opening a new PR for things that were already in review? I would have appreciated if you dropped your comments on the existing PRs instead.

I'm concerned about the "supersedes #X, #Y, and #Z" framing, particularly since I can see code borrowed from those PRs. If you're building on existing work, could you please use Co-Authored-By in your commit messages to give proper credit?

Additionally, while your code may indeed supersede the others, it would help to have an explicit explanation of why and how your approach is superior. Right now, the reasoning behind the supersession isn't clear from the PR description.

Copilot AI lite review requested due to automatic review settings September 7, 2026 13:32

Copilot started reviewing on behalf of injeniero September 7, 2026 13:32 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Comment thread internal/libyaml/emitter.go

injeniero force-pushed the fix/multiline-scalar-roundtrip branch from 966d48a to 2eddc77 Compare September 7, 2026 14:05

Merge branch 'main' into fix/multiline-scalar-roundtrip

2b29125

ccoVeille reviewed Sep 12, 2026

View reviewed changes

Navigation Menu

Fix multiline scalar serialization round-trips - #414

Fix multiline scalar serialization round-trips#414
injeniero wants to merge 2 commits into
yaml:mainfrom
injeniero:fix/multiline-scalar-roundtrip

injeniero commented Sep 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

injeniero commented Sep 7, 2026

Uh oh!

ccoVeille Sep 12, 2026

Uh oh!

ccoVeille Sep 12, 2026

Uh oh!

ccoVeille Sep 12, 2026

Uh oh!

ccoVeille Sep 12, 2026

Uh oh!

ccoVeille commented Sep 12, 2026

Uh oh!

ccoVeille Sep 12, 2026

Uh oh!

ccoVeille commented Sep 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	"\"\\tThis starts with tab\\nand is long enough\\nfor literal style\"\n",
	`"\tThis starts with tab\nand is long enough\nfor literal style"` + "\n",

Conversation

injeniero commented Sep 7, 2026

Problem

Cause and fix

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

injeniero commented Sep 7, 2026

Uh oh!

ccoVeille Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

ccoVeille Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

ccoVeille Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

ccoVeille Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

ccoVeille commented Sep 12, 2026

Uh oh!

ccoVeille Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

ccoVeille commented Sep 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants