Skip to content

fix: use registered ext coders for named non-struct types (#55) - #110

Closed
youdie006 wants to merge 1 commit into
shamaton:mainfrom
youdie006:fix/55-ext-nonstruct-types
Closed

fix: use registered ext coders for named non-struct types (#55)#110
youdie006 wants to merge 1 commit into
shamaton:mainfrom
youdie006:fix/55-ext-nonstruct-types

Conversation

@youdie006

Copy link
Copy Markdown

Repro

Register an ext coder for a named non-struct type (as commonly used for Go "enums"):

type Role uint8 // ext code 0x02
  • msgpack.Marshal(Role(1)) returns 0x01 (plain fixint) instead of the fixext frame d4 02 01.
  • msgpack.Marshal([]Role{1, 2}) returns an array of plain ints (92 01 02) instead of ext frames (92 d4 02 01 d4 02 02).

Because the value is never encoded as an ext, Unmarshal cannot recover it either, so the round-trip is lossy.

Root cause

AddExtCoder / AddExtEncoder accept an ext.Encoder whose Type() may be any reflect.Type, but the ext registry was only consulted in the struct dispatch (calcStruct / writeStruct, and setStruct on decode). The general dispatch (calcSize / create on encode, decode on decode) switches purely on reflect.Kind, so an ext coder registered for a named non-struct type is silently ignored at the top level, inside slices, and as a struct field.

This is a self-inconsistency: time.Time works through the same AddExtCoder API only because its kind is Struct; an equally-valid ext coder for a named int is dropped. The public API accepts any reflect.Type without restriction, so ignoring it is a correctness bug, not a documented limitation.

Fix

Consult the ext registry by rv.Type() at the top of the general dispatch functions, mirroring exactly what the struct path already does:

  • encode: calcSize -> CalcByteSize, create -> WriteToBytes (guarded by rv.IsValid() for omitted-field zero Values).
  • decode: decode mirrors setStruct's ext dispatch, so a fixext frame is restored into a named non-struct target.

When no ext coder matches, control falls through to the existing kind switch, so non-ext types, the struct path, and time.Time are unaffected.

Tests

  • internal/encoding/ext_test.go: asserts a named uint8 ext coder produces the fixext frame at the top level and inside a slice (not plain fixints).
  • ext_nonstruct_test.go: full Marshal -> Unmarshal round-trip (top level and slice) proving the value is restored.

go test ./..., gofmt -l ., and go vet ./... are all clean.

Reported by @Beefster09.


This change was made with AI assistance and reviewed by me before submission.

AddExtCoder/AddExtEncoder accept an ext.Encoder whose Type() may be any
reflect.Type, but the ext registry was only consulted in the struct dispatch
(calcStruct/writeStruct, and setStruct on decode). The general dispatch
(calcSize/create on encode, decode on decode) switches purely on reflect.Kind,
so an ext coder registered for a named non-struct type (e.g. type Role uint8,
the common Go "enum" pattern) was silently ignored at the top level, inside
slices, and as a struct field. time.Time works through the same API only because
its kind is Struct.

Marshal(Role(1)) returned 0x01 (plain fixint) instead of the fixext frame
d4 02 01, and because the value was never encoded as an ext, Unmarshal could not
recover it - the round-trip was lossy.

Consult the ext registry by rv.Type() at the top of the general dispatch
functions, mirroring what the struct path already does (calcSize -> CalcByteSize,
create -> WriteToBytes, guarded by rv.IsValid() for omitted-field zero Values;
decode mirrors setStruct's ext dispatch). When no ext coder matches, control
falls through to the existing kind switch, so non-ext types, the struct path and
time.Time are unaffected.

Fixes shamaton#55.
Copilot AI lite review requested due to automatic review settings August 19, 2026 07:20
@youdie006
youdie006 requested a review from shamaton as a code owner August 19, 2026 07:20
@github-actions github-actions Bot added the type: bug Confirmed or likely defect label Aug 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a correctness gap in the ext-coder dispatch so that ext coders registered for named, non-struct Go types (e.g., enum-style type Role uint8) are honored during general encoding/decoding paths, restoring lossless round-trips.

Changes:

  • Encode: consult the ext encoder registry by rv.Type() before the kind switch in calcSize and create.
  • Decode: consult the ext decoder registry before the kind switch in decode, mirroring the struct-path ext handling.
  • Add tests covering named non-struct ext encoding and end-to-end Marshal/Unmarshal round-trip (top-level + slice).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
internal/encoding/ext_test.go Adds a focused internal test ensuring a named uint8 type uses fixext frames (top-level + slice).
internal/encoding/encoding.go Adds ext-dispatch to the general encode size/write paths (not just the struct path).
internal/decoding/decoding.go Adds ext-dispatch to the general decode path (not just the struct path).
ext_nonstruct_test.go Adds an end-to-end public API test for Marshal/Unmarshal round-trips of named non-struct types.
Suppressed comments (1)

internal/encoding/encoding.go:272

  • Same as calcSize: this ext fast-path runs before the kind-based nil handling. If an ext encoder is registered for a pointer/slice/map type, nil values will be routed to WriteToBytes instead of encoding as nil, which is a behavior change vs the existing switch and may be surprising/unsafe.
	if rv.IsValid() {
		for i := range extCoders {
			if extCoders[i].Type() == rv.Type() {
				return extCoders[i].WriteToBytes(rv, offset, &e.d)
			}

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


// ext types: honor a registered ext decoder for any kind, not just structs
// (mirrors setStruct). Falls through to the kind switch when nothing matches.
if isExt, _, extErr := d.extEndOffset(offset); extErr == nil && isExt {
Comment on lines +66 to +74
// ext types: honor a registered ext encoder for any kind, not just structs
// (mirrors calcStruct). Falls through to the kind switch when nothing matches.
if rv.IsValid() {
for i := range extCoders {
if extCoders[i].Type() == rv.Type() {
return extCoders[i].CalcByteSize(rv)
}
}
}
Comment on lines +66 to +70
// ext types: honor a registered ext encoder for any kind, not just structs
// (mirrors calcStruct). Falls through to the kind switch when nothing matches.
if rv.IsValid() {
for i := range extCoders {
if extCoders[i].Type() == rv.Type() {
@shamaton

shamaton commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Thanks for the report and the fix attempt, @youdie006 — the root-cause analysis (ext registry only consulted in the struct dispatch, not the general calcSize/create/decode switches) is spot on, and the repro is exactly the bug reported in #55.

I re-verified the current revision (ea09ad5) before deciding how to proceed:

  • Round-trip correctness: confirmed working for top-level values, slice elements, and struct fields on the non-stream Marshal/Unmarshal path.
  • Stream API gap: internal/stream/encoding and internal/stream/decoding (used by msgpack.NewEncoder/NewDecoder) don't consult the ext registry outside the struct path either, so the same bug reproduces there. This PR doesn't touch those packages.
  • Performance: calcSize/create now do an unconditional linear scan over every registered ext encoder for every value, regardless of Kind(), which regresses the common non-ext path too:
    • plain int encode: ~31ns/op, 2 allocs (this PR) vs ~16ns/op, 1 alloc (kind-partitioned registry)
    • encoding a []Role of 1024 ext-coded elements: ~14.9µs/op (this PR) vs ~4.6µs/op (kind-partitioned registry), ~3x
    • Decode-side performance is comparable between the two approaches either way (the ext.Decoder interface has no Type(), so a linear scan by wire code is unavoidable regardless of design).

Given the stream-API gap and the encode-side regression, I've finished a fix that partitions the encode registry by Kind() and mirrors the decode fix across both the non-stream and stream APIs. I've opened #117 with credit to you for the report, repro, and root-cause writeup, and I'm closing this one in favor of that. Thanks again for digging into this!

@shamaton shamaton closed this Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug Confirmed or likely defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants