Source
PR #144 R1 type-design review (MEDIUM finding, deferred to follow-up).
Problem
PR #133 added unit: BoundsUnit to AllocationFailed (parallel to BoundsExceeded::unit). PR #134 typified context: AllocationContext. After both, the unit is structurally derivable from the context — verified by inventorying every call site:
| AllocationContext variant |
Unit |
| EntryPayloadBytes |
Bytes |
| V10MainIndexBytes |
Bytes |
| V10EncodedEntriesBytes |
Bytes |
| V10FdiBytes |
Bytes |
| InlineCompressionBlocks |
Items |
| EncodedCompressionBlocks |
Items |
| FlatIndexEntries |
Items |
| DedupTracker |
Items |
| ByPathLookup |
Items |
| V10NonEncodedEntries |
Items |
| V10IndexEntries |
Items |
The variant naming convention IS the discriminator: *Bytes ↔ Bytes, everything else ↔ Items. So AllocationFailed::unit is the type-system equivalent of carrying around a copy of self.id in a struct field — at every construction site the caller has to repeat data the type already knows, and at every reconstruction site there's an opportunity for the two to disagree silently.
No test pins the implicit unit == context.unit() invariant. A future call site that passes unit: BoundsUnit::Items with context: V10MainIndexBytes would silently render "items for v10+ index" instead of "bytes for v10+ index" — wrong but accepted.
BoundsExceeded::unit does NOT have this problem because WireField's units are mixed (UncompressedSize is bytes; BlockCount is items; CompressionMethod doesn't even have a sensible unit). Just AllocationContext.
Fix
Two acceptable resolutions:
Recommended (single source of truth):
- Add
pub fn unit(&self) -> BoundsUnit method to AllocationContext with the mapping above.
- Drop the
unit: BoundsUnit field from AllocationFailed.
- Update Display arm to call
context.unit() instead of reading the field.
- Migrate all 11 production call sites + tests to drop the redundant arg.
This is mechanical and produces compile errors at every drift site — no silent failures possible.
Acceptable (paper over the gap):
- Keep
unit as-is.
- Add
AllocationContext::unit() -> BoundsUnit method.
- Add a unit test that walks every variant and asserts
context.unit() == matching unit field passed at the corresponding production site (or just that the method returns the documented value).
- Document in
AllocationFailed's unit field doc that it MUST equal context.unit().
The current "explicit-and-redundant" choice is the worst-of-both-worlds: every call site has to remember the right pairing AND nothing catches it if they don't.
Files
crates/paksmith-core/src/error.rs (variant + Display + new method)
- All 11
AllocationFailed construction sites (5 production files + tests)
Source
PR #144 R1 type-design review (MEDIUM finding, deferred to follow-up).
Problem
PR #133 added
unit: BoundsUnittoAllocationFailed(parallel toBoundsExceeded::unit). PR #134 typifiedcontext: AllocationContext. After both, the unit is structurally derivable from the context — verified by inventorying every call site:The variant naming convention IS the discriminator:
*Bytes↔ Bytes, everything else ↔ Items. SoAllocationFailed::unitis the type-system equivalent of carrying around a copy ofself.idin a struct field — at every construction site the caller has to repeat data the type already knows, and at every reconstruction site there's an opportunity for the two to disagree silently.No test pins the implicit
unit == context.unit()invariant. A future call site that passesunit: BoundsUnit::Itemswithcontext: V10MainIndexByteswould silently render "items for v10+ index" instead of "bytes for v10+ index" — wrong but accepted.BoundsExceeded::unitdoes NOT have this problem becauseWireField's units are mixed (UncompressedSizeis bytes;BlockCountis items;CompressionMethoddoesn't even have a sensible unit). JustAllocationContext.Fix
Two acceptable resolutions:
Recommended (single source of truth):
pub fn unit(&self) -> BoundsUnitmethod toAllocationContextwith the mapping above.unit: BoundsUnitfield fromAllocationFailed.context.unit()instead of reading the field.This is mechanical and produces compile errors at every drift site — no silent failures possible.
Acceptable (paper over the gap):
unitas-is.AllocationContext::unit() -> BoundsUnitmethod.context.unit() == matching unit field passed at the corresponding production site(or just that the method returns the documented value).AllocationFailed'sunitfield doc that it MUST equalcontext.unit().The current "explicit-and-redundant" choice is the worst-of-both-worlds: every call site has to remember the right pairing AND nothing catches it if they don't.
Files
crates/paksmith-core/src/error.rs(variant + Display + new method)AllocationFailedconstruction sites (5 production files + tests)