fix: make Unpacker.readArray usable - #25
Conversation
readArray passed its type parameter straight to unpackArray, but the two disagreed about what it means. unpackArray takes the slice type and derives the element type itself; readArray declares `![]T`, treating T as the element type. Both readings failed to compile, so the method could not be instantiated with any argument, and the "completely custom format" example in README.md did not build. Pass the slice type down, keeping the element-type convention that Packer.writeArray and Unpacker.readArrayInto already use. The same mismatch hid two errors in the Array(T) helper, which nothing constructs: msgpackWrite called writeArray with one argument instead of two, and msgpackRead passed the slice type where the element type belongs. refAllDecls does not instantiate generic functions, so none of this was covered. Add tests that actually call readArray, round trip Array(T), and build the README example verbatim.
|
Warning Review limit reachedNext included review available in 53 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change fixes typed array decoding and updates ChangesTyped array support
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Typed MessagePack array reads and Array(T) serialization now use consistent type conventions, restoring the documented custom-format and wrapper round trips without an identified remaining merge risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Fixes #21.
Unpacker.readArraypasses its type parameter straight through tounpackArray, but the two disagree about what that parameter means:unpackArrayexpects the slice type — it derives the element type itself viastd.meta.Childand returnsT.readArraydeclares![]T, i.e. it treatsTas the element type. Both readings fail, so the method cannot be instantiated with any argument:readArray(u32)→ "Expected pointer, optional, array or vector type, found 'u32'"readArray([]u32)→ "pointer type child 'u32' cannot cast into pointer type child '[]u32'"The fix passes the slice type down, keeping the element-type convention that
Packer.writeArrayandUnpacker.readArrayIntoalready use.This broke the README
The "Or you can use a completely custom format" example calls
unpacker.readArray(u32), so it did not compile as written. It is now a test, verbatim.Array(T)
The same mismatch hid two more errors in the
Array(T)helper insrc/array.zig, which nothing in the repo constructs:msgpackWritecalledwriteArraywith one argument where it takes two, andmsgpackReadpassed the slice type where the element type belongs. Both fixed, with a round-trip test so the generic is actually instantiated. The neighbouringStringandBinaryhelpers were already fine.Why CI was green
test { _ = std.testing.refAllDecls(@This()); }does not instantiate generic functions, so nothing ever type-checked these. The three tests added here call the affected paths directly. Reverting the one-line fix makes them fail to compile, so they hold the line.zig build test: 156/156 pass.Summary by CodeRabbit
Bug Fixes
Arraywrappers and raw arrays.Tests