Add DumpError for all dump stack errors by ingydotnet · Pull Request #319 · yaml/go-yaml

ingydotnet · 2026-03-15T17:23:48Z

Introduce DumpError as the unified error type for the dump pipeline, mirroring the LoadError pattern added in PR #311 for the load stack.

Motivation

Before this change the dump stack had no structured error type: errors escaped as a mix of bare panics (raw strings or raw error values), untyped fmt.Errorf strings from failf(), EmitterError, WriterError, and — incorrectly — LoadErrors/LoadError in dumper.go. None of these were introspectable by callers: you couldn't tell which stage failed or extract the underlying cause without string-matching.

What changed

New types and helpers (internal/libyaml/errors.go)

Four new Stage constants covering the dump pipeline: RepresenterStage, SerializerStage, EmitterStage, WriterStage.
DumpError struct: Stage + Message + unexported err for Unwrap(). Error() format: "go-yaml dump error in : ".
NewDumpError(stage, message, cause) public constructor.
failDump(stage, err) — wraps an existing error as DumpError.
failDumpf(stage, fmt, args...) — formats a new DumpError message. Both helpers panic with *YAMLError so handleErr catches them and returns them as ordinary Go errors.

representer.go — 7 error sites converted

was	now
Fail(err) (MarshalYAML)	failDump(RepresenterStage, err)

The three bare panics (type, getStructInfo, inline-map key) previously escaped handleErr; they now become returned *DumpError values. Unused fmt import removed.

serializer.go — 3 error sites + must() rewritten

The three failf() calls in node() are now failDumpf(SerializerStage,…).

must() previously collapsed all emitter/writer errors into a single untyped failf string. It now type-switches on the error:

EmitterError → failDumpf(EmitterStage, e.Message)
WriterError → failDump(WriterStage, unwrapped-cause)
(errors.Unwrap strips the "write error: " prefix that writer.go adds
so that WriterStage in the DumpError already communicates that context)
default → failDumpf(SerializerStage, msg)

dumper.go — 2 fixes

Lines 77-86: the AllDocuments slice-check incorrectly returned a LoadErrors/LoadError (load-stack types). Replaced with a plain DumpError{Stage: RepresenterStage}.
SetIndent: bare panic("yaml: cannot indent…") replaced with failDumpf(SerializerStage, …) for consistent error type. (SetIndent has no handleErr so the panic still propagates, but it is now a *YAMLError wrapping *DumpError instead of a raw string.) Unused errors import removed.

yaml.go — public API additions

Stage doc comment updated to cover load and dump.
Four new Stage constants re-exported (RepresenterStage …WriterStage).
DumpError type alias added to the error-types block.
NewDumpError var added alongside NewLoadError.

Tests

errors_test.go: runDumpErrorTest handler + buildDumpError helper, registered under the "dump-error" key.
testdata/errors.yaml: 5 new dump-error test cases covering all four dump stages and Unwrap behaviour.
yaml_test.go TestMarshalErrors: changed from PanicMatches to ErrorMatches + errors.As checks now that the bare panics are caught. Test struct field renamed panic→stage to reflect the new approach.
yaml_test.go TestEncoderWriteError: updated expected message from "yaml: write error: …" to "go-yaml dump error in writer: …" and added errors.As / Stage assertions.
yaml_test.go TestMarshalerError: fixed argument order in ErrorIs (was assert.ErrorIs(t, errFailing, err); now assert.ErrorIs(t, err, errFailing)) — the old order was accidentally correct only because the returned error was the raw errFailing; now it is a DumpError that wraps errFailing via Unwrap().
node_test.go: two ErrorMatches expectations updated to the new "go-yaml dump error in serializer: …" format.

Copilot

Pull request overview

This PR introduces a structured DumpError type for the YAML dump pipeline (mirroring LoadError), so callers can reliably introspect dump failures by stage and unwrap underlying causes instead of relying on panic values or string matching.

Changes:

Add dump-stage Stage constants plus DumpError, NewDumpError, and dump-specific panic helpers in internal/libyaml.
Convert dump pipeline error sites (representer/serializer/dumper) to consistently produce *DumpError (via handleErr recovery).
Update and extend tests and testdata to assert DumpError formatting, Stage, and Unwrap() behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`internal/libyaml/errors.go`	Adds dump-stage constants, `DumpError`, constructor, and dump-specific panic helpers.
`internal/libyaml/representer.go`	Routes representer failures through `DumpError` instead of bare panics/Fail/failf.
`internal/libyaml/serializer.go`	Routes serializer failures through `DumpError`; maps emitter/writer errors to appropriate dump stages.
`internal/libyaml/dumper.go`	Fixes incorrect load-stack error type usage; normalizes indentation error to `DumpError`.
`yaml.go`	Re-exports dump-stage constants, `DumpError`, and `NewDumpError` in the public API.
`internal/libyaml/errors_test.go`	Adds handler and builder for `dump-error` test cases.
`internal/libyaml/testdata/errors.yaml`	Adds dump-error test cases covering all dump stages and unwrap behavior.
`yaml_test.go`	Updates marshaling/encoder error tests to assert `DumpError` and stage instead of panic matching.
`node_test.go`	Updates expected dump error messages to the new `DumpError` format.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ccoVeille

Some minor remarks, otherwise 👍

Three issues raised in code review on PR #319: 1. failDump double-wrapping (errors.go) If the caller passes an error that is already a *DumpError (e.g. a user's MarshalYAML returns yaml.NewDumpError(...)), the old code created a new DumpError whose Message was the existing error's Error() string, producing nested prefixes like "go-yaml dump error in representer: go-yaml dump error in …" and overwriting the original Stage for errors.As callers. Fix: use errors.As to detect an existing *DumpError and panic with it directly, leaving Stage and Message intact. 2. Nil guard in runDumpErrorTest (errors_test.go) err.Error() on the result of buildDumpError would panic with a nil pointer dereference if buildDumpError ever returned nil. Fix: assert.NotNilf guard immediately after the call. 3. Use errors.As in Serializer.must() (serializer.go) The type switch (case EmitterError / case WriterError) only matches direct concrete types and silently falls through for wrapped errors. errors.As traverses the error chain, making the dispatch correct for any future wrapping. Fix: replace the type switch with two errors.As checks followed by a plain default fallthrough.

Copilot

Pull request overview

This PR unifies dump-pipeline error handling by introducing DumpError (mirroring the existing LoadError pattern) and updating dump stack stages to consistently return introspectable, stage-tagged errors instead of a mix of panics and legacy/untyped errors.

Changes:

Add dump-stage Stage constants plus DumpError, NewDumpError, and dump-specific helpers in internal/libyaml/errors.go, re-exported via yaml.go.
Convert representer/serializer/dumper dump error sites to emit *DumpError (via failDump / failDumpf) with correct stage attribution.
Update and extend tests + golden testdata to validate message format, Stage, and Unwrap() behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`internal/libyaml/errors.go`	Introduces `DumpError`, dump `Stage` constants, and dump failure helpers.
`internal/libyaml/representer.go`	Routes representer failures through `DumpError` instead of bare panics/legacy helpers.
`internal/libyaml/serializer.go`	Routes serializer/emitter/writer failures through `DumpError` with stage-specific handling.
`internal/libyaml/dumper.go`	Fixes incorrect load-stack errors returned from dump code; aligns indent validation with `DumpError`.
`yaml.go`	Re-exports dump stages and `DumpError`/`NewDumpError` in the public API.
`internal/libyaml/errors_test.go`	Adds data-driven handler for `DumpError` formatting + unwrap behavior.
`internal/libyaml/testdata/errors.yaml`	Adds dump-error test cases covering all dump stages and unwrap.
`yaml_test.go`	Updates marshal/encoder error assertions to match `DumpError` behavior and stage introspection.
`node_test.go`	Updates expected marshal error messages to new dump error format.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Three issues raised in code review on PR #319: 1. failDump double-wrapping (errors.go) If the caller passes an error that is already a *DumpError (e.g. a user's MarshalYAML returns yaml.NewDumpError(...)), the old code created a new DumpError whose Message was the existing error's Error() string, producing nested prefixes like "go-yaml dump error in representer: go-yaml dump error in …" and overwriting the original Stage for errors.As callers. Fix: use errors.As to detect an existing *DumpError and panic with it directly, leaving Stage and Message intact. 2. Nil guard in runDumpErrorTest (errors_test.go) err.Error() on the result of buildDumpError would panic with a nil pointer dereference if buildDumpError ever returned nil. Fix: assert.NotNilf guard immediately after the call. 3. Use errors.As in Serializer.must() (serializer.go) The type switch (case EmitterError / case WriterError) only matches direct concrete types and silently falls through for wrapped errors. errors.As traverses the error chain, making the dispatch correct for any future wrapping. Fix: replace the type switch with two errors.As checks followed by a plain default fallthrough.

Copilot

Pull request overview

This PR introduces a unified DumpError type for the dump (marshal/encode) pipeline, analogous to the existing LoadError pattern, so callers can reliably introspect dump failures by stage and unwrap underlying causes.

Changes:

Add dump-stage Stage constants and new DumpError (+ NewDumpError, failDump, failDumpf) in internal/libyaml.
Convert representer/serializer/dumper error sites to emit DumpError consistently (including previously “bare panic” paths).
Update public re-exports and tests to assert DumpError messages, stages, and Unwrap behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`internal/libyaml/errors.go`	Adds `DumpError`, dump `Stage` constants, and `failDump`/`failDumpf` helpers.
`internal/libyaml/representer.go`	Routes representer-stage failures through `DumpError` instead of `Fail`/raw panics.
`internal/libyaml/serializer.go`	Converts serializer failures to `DumpError` and maps emitter/writer errors to the proper dump stages.
`internal/libyaml/dumper.go`	Fixes dump option validation error type and standardizes `SetIndent` failure to `DumpError`.
`yaml.go`	Re-exports dump stages and `DumpError`/`NewDumpError` in the public API.
`internal/libyaml/errors_test.go`	Adds data-driven handler for `DumpError` cases including unwrap checks.
`internal/libyaml/testdata/errors.yaml`	Adds `DumpError` test vectors for representer/serializer/emitter/writer stages.
`yaml_test.go`	Updates marshal/encoder tests to assert `DumpError` instead of panic strings and checks `Stage`.
`node_test.go`	Updates expected marshal error strings to the new dump error format.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Three issues raised in code review on PR #319: 1. failDump double-wrapping (errors.go) If the caller passes an error that is already a *DumpError (e.g. a user's MarshalYAML returns yaml.NewDumpError(...)), the old code created a new DumpError whose Message was the existing error's Error() string, producing nested prefixes like "go-yaml dump error in representer: go-yaml dump error in …" and overwriting the original Stage for errors.As callers. Fix: use errors.As to detect an existing *DumpError and panic with it directly, leaving Stage and Message intact. 2. Nil guard in runDumpErrorTest (errors_test.go) err.Error() on the result of buildDumpError would panic with a nil pointer dereference if buildDumpError ever returned nil. Fix: assert.NotNilf guard immediately after the call. 3. Use errors.As in Serializer.must() (serializer.go) The type switch (case EmitterError / case WriterError) only matches direct concrete types and silently falls through for wrapped errors. errors.As traverses the error chain, making the dispatch correct for any future wrapping. Fix: replace the type switch with two errors.As checks followed by a plain default fallthrough.

Introduce DumpError as the unified error type for the dump pipeline, mirroring the LoadError pattern added in PR #311 for the load stack. ## Motivation Before this change the dump stack had no structured error type: errors escaped as a mix of bare panics (raw strings or raw error values), untyped fmt.Errorf strings from failf(), EmitterError, WriterError, and — incorrectly — LoadErrors/LoadError in dumper.go. None of these were introspectable by callers: you couldn't tell which stage failed or extract the underlying cause without string-matching. ## What changed ### New types and helpers (internal/libyaml/errors.go) * Four new Stage constants covering the dump pipeline: RepresenterStage, SerializerStage, EmitterStage, WriterStage. * DumpError struct: Stage + Message + unexported err for Unwrap(). Error() format: "go-yaml dump error in <stage>: <message>". * NewDumpError(stage, message, cause) public constructor. * failDump(stage, err) — wraps an existing error as DumpError. * failDumpf(stage, fmt, args...) — formats a new DumpError message. Both helpers panic with *YAMLError so handleErr catches them and returns them as ordinary Go errors. ### representer.go — 7 error sites converted | was | now | |-----|-----| | Fail(err) (MarshalYAML) | failDump(RepresenterStage, err) | | Fail(err) (MarshalText) | failDump(RepresenterStage, err) | | panic("cannot represent type: …") | failDumpf(RepresenterStage, …) | | panic(err) (getStructInfo) | failDump(RepresenterStage, err) | | panic(fmt.Sprintf("cannot have key…")) | failDumpf(RepresenterStage, …) | | failf("explicitly tagged !!binary…") | failDumpf(RepresenterStage, …) | | failf("cannot represent invalid UTF-8…") | failDumpf(RepresenterStage, …) | The three bare panics (type, getStructInfo, inline-map key) previously escaped handleErr; they now become returned *DumpError values. Unused fmt import removed. ### serializer.go — 3 error sites + must() rewritten The three failf() calls in node() are now failDumpf(SerializerStage,…). must() previously collapsed all emitter/writer errors into a single untyped failf string. It now type-switches on the error: * EmitterError → failDumpf(EmitterStage, e.Message) * WriterError → failDump(WriterStage, unwrapped-cause) (errors.Unwrap strips the "write error: " prefix that writer.go adds so that WriterStage in the DumpError already communicates that context) * default → failDumpf(SerializerStage, msg) ### dumper.go — 2 fixes * Lines 77-86: the AllDocuments slice-check incorrectly returned a LoadErrors/LoadError (load-stack types). Replaced with a plain DumpError{Stage: RepresenterStage}. * SetIndent: bare panic("yaml: cannot indent…") replaced with failDumpf(SerializerStage, …) for consistent error type. (SetIndent has no handleErr so the panic still propagates, but it is now a *YAMLError wrapping *DumpError instead of a raw string.) Unused errors import removed. ### yaml.go — public API additions * Stage doc comment updated to cover load and dump. * Four new Stage constants re-exported (RepresenterStage …WriterStage). * DumpError type alias added to the error-types block. * NewDumpError var added alongside NewLoadError. ## Tests * errors_test.go: runDumpErrorTest handler + buildDumpError helper, registered under the "dump-error" key. * testdata/errors.yaml: 5 new dump-error test cases covering all four dump stages and Unwrap behaviour. * yaml_test.go TestMarshalErrors: changed from PanicMatches to ErrorMatches + errors.As checks now that the bare panics are caught. Test struct field renamed panic→stage to reflect the new approach. * yaml_test.go TestEncoderWriteError: updated expected message from "yaml: write error: …" to "go-yaml dump error in writer: …" and added errors.As / Stage assertions. * yaml_test.go TestMarshalerError: fixed argument order in ErrorIs (was assert.ErrorIs(t, errFailing, err); now assert.ErrorIs(t, err, errFailing)) — the old order was accidentally correct only because the returned error was the raw errFailing; now it is a DumpError that wraps errFailing via Unwrap(). * node_test.go: two ErrorMatches expectations updated to the new "go-yaml dump error in serializer: …" format.

Three issues raised in code review on PR #319: 1. failDump double-wrapping (errors.go) If the caller passes an error that is already a *DumpError (e.g. a user's MarshalYAML returns yaml.NewDumpError(...)), the old code created a new DumpError whose Message was the existing error's Error() string, producing nested prefixes like "go-yaml dump error in representer: go-yaml dump error in …" and overwriting the original Stage for errors.As callers. Fix: use errors.As to detect an existing *DumpError and panic with it directly, leaving Stage and Message intact. 2. Nil guard in runDumpErrorTest (errors_test.go) err.Error() on the result of buildDumpError would panic with a nil pointer dereference if buildDumpError ever returned nil. Fix: assert.NotNilf guard immediately after the call. 3. Use errors.As in Serializer.must() (serializer.go) The type switch (case EmitterError / case WriterError) only matches direct concrete types and silently falls through for wrapped errors. errors.As traverses the error chain, making the dispatch correct for any future wrapping. Fix: replace the type switch with two errors.As checks followed by a plain default fallthrough.

errors.As in failDump also matched when err merely wrapped a *DumpError, causing the outer wrapper's message and context to be silently dropped. Replace errors.As with a direct type assertion so the pass-through only triggers when err is exactly *DumpError; wrapped DumpErrors are treated as ordinary errors and their full message is preserved in a new DumpError.

1. Replace assert.NotNilf nil guard with t.Fatal so the fatal intent is explicit and obvious to reviewers, regardless of which assert library is in use. 2. Split the stage type assertion so a non-string stage value fails the test instead of silently skipping the Stage check. Previously errorSpec["stage"].(string) returned ok=false for any non-string value, masking a mis-typed test-data field as a no-op.

Use early return pattern and include error type in unknown-error fallback message, per ccoVeille's review.

colinjlacy

Approving. I question the idea of failDump and failDumpf panicking, but I can see that it matches previous behavior

Three issues raised in code review on PR #319: 1. failDump double-wrapping (errors.go) If the caller passes an error that is already a *DumpError (e.g. a user's MarshalYAML returns yaml.NewDumpError(...)), the old code created a new DumpError whose Message was the existing error's Error() string, producing nested prefixes like "go-yaml dump error in representer: go-yaml dump error in …" and overwriting the original Stage for errors.As callers. Fix: use errors.As to detect an existing *DumpError and panic with it directly, leaving Stage and Message intact. 2. Nil guard in runDumpErrorTest (errors_test.go) err.Error() on the result of buildDumpError would panic with a nil pointer dereference if buildDumpError ever returned nil. Fix: assert.NotNilf guard immediately after the call. 3. Use errors.As in Serializer.must() (serializer.go) The type switch (case EmitterError / case WriterError) only matches direct concrete types and silently falls through for wrapped errors. errors.As traverses the error chain, making the dispatch correct for any future wrapping. Fix: replace the type switch with two errors.As checks followed by a plain default fallthrough.

Copilot AI review requested due to automatic review settings March 15, 2026 17:23

Copilot started reviewing on behalf of ingydotnet March 15, 2026 17:24 View session

ingydotnet requested review from a-hilaly, ccoVeille, colinjlacy and Copilot and removed request for Copilot March 15, 2026 17:25

Copilot started reviewing on behalf of ingydotnet March 15, 2026 17:26 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

Comment thread internal/libyaml/errors.go

ccoVeille approved these changes Mar 15, 2026

View reviewed changes

Comment thread internal/libyaml/errors_test.go

Comment thread internal/libyaml/serializer.go Outdated

ingydotnet requested review from ccoVeille and Copilot March 15, 2026 21:07

Copilot started reviewing on behalf of ingydotnet March 15, 2026 21:07 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

ingydotnet force-pushed the dump-error branch from 86f5bf5 to 268f0b7 Compare March 16, 2026 00:59

ingydotnet requested a review from Copilot March 16, 2026 01:00

Copilot started reviewing on behalf of ingydotnet March 16, 2026 01:01 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Comment thread internal/libyaml/errors.go Outdated

ingydotnet force-pushed the dump-error branch from 268f0b7 to e76ceb0 Compare March 16, 2026 01:14

ingydotnet force-pushed the dump-error branch from e76ceb0 to 7f41dde Compare March 16, 2026 01:22

ccoVeille reviewed Mar 16, 2026

View reviewed changes

Comment thread internal/libyaml/errors_test.go Outdated

Comment thread internal/libyaml/errors_test.go Outdated

ingydotnet requested a review from ccoVeille March 16, 2026 14:47

ccoVeille approved these changes Mar 16, 2026

View reviewed changes

Comment thread internal/libyaml/serializer.go Outdated

Comment thread internal/libyaml/serializer.go Outdated

ingydotnet force-pushed the dump-error branch from a6d771c to 78a853e Compare April 12, 2026 14:01

colinjlacy approved these changes Apr 12, 2026

View reviewed changes

ingydotnet force-pushed the dump-error branch from 78a853e to a0b5131 Compare May 1, 2026 15:26

ingydotnet added 5 commits May 1, 2026 13:29

Address must() review comments

4ba5c68

Use early return pattern and include error type in unknown-error fallback message, per ccoVeille's review.

ingydotnet force-pushed the dump-error branch from 2b5aace to 4ba5c68 Compare May 1, 2026 17:29

ingydotnet requested review from ccoVeille and colinjlacy May 1, 2026 17:30

colinjlacy approved these changes May 1, 2026

View reviewed changes

ingydotnet merged commit dee8e44 into main May 1, 2026
24 checks passed

ingydotnet deleted the dump-error branch May 1, 2026 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DumpError for all dump stack errors#319

Add DumpError for all dump stack errors#319
ingydotnet merged 5 commits into
mainfrom
dump-error

ingydotnet commented Mar 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

ccoVeille left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colinjlacy left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ingydotnet commented Mar 15, 2026

Motivation

What changed

New types and helpers (internal/libyaml/errors.go)

representer.go — 7 error sites converted

serializer.go — 3 error sites + must() rewritten

dumper.go — 2 fixes

yaml.go — public API additions

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

ccoVeille left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colinjlacy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants