Remove support for decoding positional encoding (structs as arrays) from code generator - #37
Conversation
cce
changed the title
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy fallback that allowed “map-structs” (normal msgp structs) to be decoded from array-encoded msgpack values (positional/Go-field-order decoding), making map-struct decoding strictly require a msgpack map. It adds targeted tests to ensure array-encoded inputs are rejected while explicit tuple structs continue to support array encoding.
Changes:
- Removed the generator’s “struct-from-array” fallback path for map-encoded structs (now map-only).
- Updated generated test fixtures (
tests/required_gen.go) to reflect map-only decoding. - Added tests asserting that array-encoded inputs for map-structs are rejected, while
//msgp:tuplestructs still decode arrays.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/required_gen.go | Regenerated unmarshaling code to remove array fallback and enforce map-only decoding for map-structs. |
| tests/array_decode_test.go | Adds coverage ensuring array-encoded values are rejected for map-structs, and tuple structs still round-trip via arrays. |
| gen/unmarshal.go | Removes the array-decoding fallback from the struct unmarshal generator and updates related comments. |
Files not reviewed (1)
- tests/required_gen.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // object, or encoded as a zero value) is a decode error. This runs after the | ||
| // struct body has been decoded, so it applies uniformly to the map, | ||
| // struct-from-array, and tuple decode paths. | ||
| // struct body has been decoded, so it applies uniformly to the map and tuple |
There was a problem hiding this comment.
you can specify //msgp:tuple and have a struct encoded as a positional array. It doesn't look like we've ever used this feature though.
There was a problem hiding this comment.
can/should we remove that as well?
There was a problem hiding this comment.
I feel less concerned about it, as it is an explicit opt-in, and not automatic fallback behavior that is being generated whether you specified array/tuple encoding or not.
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
The encoding path is already gone? So there's essentially no way any reasonably modern hunk of msgp contains anything that needs to be decoded this way?
I guess we need to confirm that no old blocks/bundles contain anything done this way?
Remove unused code path for msgp positional encoding