diff --git a/docs/reviews/001-coverage-analysis.md b/docs/reviews/001-coverage-analysis.md new file mode 100644 index 0000000..2e27fb3 --- /dev/null +++ b/docs/reviews/001-coverage-analysis.md @@ -0,0 +1,457 @@ +--- +status: open +last_updated: 2026-08-02 +reviewed_artifacts: + - src/lib.rs + - src/error.rs + - src/macros.rs + - src/data_access.rs + - src/validation.rs + - src/schema.rs + - src/offset_map.rs + - src/layout_builder.rs + - src/sequential_reader.rs + - src/tunion.rs + - src/engine.rs + - tests/{engine_integration,error_paths,poc_roundtrip,tunion_dispatch}.rs +tool: cargo-llvm-cov +invocation: cargo llvm-cov --html --output-dir target/coverage +reviewer: coverage pass (post-rebrand sanity check) +--- + +# Coverage Analysis #001 + +## Purpose + +First dedicated code-coverage pass for the alktype crate, run immediately +after the rebranding cleanup (commit `6d61429`). This pass has two goals: + +1. Establish a coverage baseline before any new development begins, so + future passes can measure deltas. +2. Identify the weakly-covered areas that should be tightened up before + publishing — while the surface is still small and the cost of adding + tests is low. + +Headline result: **coverage is in reasonable shape for a crate that was +built task-by-task with per-task tests.** Line coverage is **88.9%** +(4557/5125), function coverage is **82.3%** (433/526), all 285 tests pass, +and the build is clean. The gaps concentrate in three categories: (a) +defensive overflow guards that are structurally hard to reach, (b) an +asymmetry between `is_valid()` (heavily tested) and `validate()` (rarely +called) in the validation layer, and (c) the `error::Display` impl which +is never exercised. None of the gaps indicate a logic defect — the code +under test was reviewed during the rebranding pass and is consistent. + +## Methodology + +- `cargo llvm-cov --json --output-path /tmp/coverage.json` then per-file + line + function breakdown from the JSON export. +- `cargo llvm-cov --text` to extract the exact uncovered source lines per + file, mapped back to the source for attribution. +- Each gap classified by testability: trivial (pure function / Display + impl), easy (existing test patterns extend naturally), medium (new + error-path scaffolding), or hard (overflow guards needing + `usize::MAX`-adjacent inputs). +- No `--all-features` flag — the crate has no feature flags currently + (`[features] default = []` in `Cargo.toml`). + +## Summary Statistics + +| Severity | Count | +|----------|-------| +| Critical | 0 | +| Warning | 0 | +| Suggestion | 8 (S1–S8) | + +No correctness findings — this is a coverage pass, not a logic review. +The logic review was performed as part of the rebranding sanity check +(commit `6d61429` message documents the full sweep). Everything below is +a suggestion ordered by leverage, not severity. + +--- + +## Per-File Coverage + +| File | Lines | Covered | % | Fns | Covered | % | +|------|------:|--------:|--:|----:|--------:|--:| +| src/error.rs | 9 | 0 | 0.0% | 1 | 0 | 0.0% | +| src/macros.rs | 144 | 109 | 75.7% | 19 | 17 | 89.5% | +| src/data_access.rs | 459 | 388 | 84.5% | 62 | 48 | 77.4% | +| src/validation.rs | 433 | 368 | 85.0% | 51 | 45 | 88.2% | +| src/sequential_reader.rs | 1115 | 965 | 86.5% | 82 | 58 | 70.7% | +| src/layout_builder.rs | 1036 | 926 | 89.4% | 95 | 72 | 75.8% | +| src/schema.rs | 464 | 423 | 91.2% | 64 | 56 | 87.5% | +| src/offset_map.rs | 605 | 560 | 92.6% | 63 | 54 | 85.7% | +| src/tunion.rs | 423 | 395 | 93.4% | 49 | 45 | 91.8% | +| src/engine.rs | 437 | 423 | 96.8% | 40 | 38 | 95.0% | +| **TOTAL** | **5125** | **4557** | **88.9%** | **526** | **433** | **82.3%** | + +`engine.rs` and `tunion.rs` are in good shape. The gaps concentrate in +`error.rs` (the `Display` impl), the macro-generated validation code +(`macros.rs`), and the error-path arms across the layout and reader +modules. + +--- + +## Suggestions + +### S1. `AlkTypeError::Display` impl is completely unexercised (error.rs, 0%) + +**File**: `src/error.rs:30–43` + +**Problem**: The `Display` impl for `AlkTypeError` is never called by any +test. All four arms — `Schema`, `Offset`, `Access`, `Validation` — are +uncovered. This is the only file at 0% and the only place where a public +trait implementation has zero coverage. + +The `Display` impl is public API surface: consumers format errors for +logging, error responses, and debugging. A typo or a `field_path`/`reason` +swap in the format strings would not be caught by any test today. + +**Fix**: ~10 lines of unit tests. Construct one error of each variant and +assert `format!("{e}")` contains the expected substrings: + +- `AlkTypeError::Schema("bad")` → `"schema error: bad"` +- `AlkTypeError::Offset { field_path: "f", reason: "r" }` → `"offset + error at f: r"` +- `AlkTypeError::Access { field_path: "f", reason: "r" }` → `"access + error at f: r"` +- `AlkTypeError::Validation(...)` → `"validation error: ..."` (use a + synthetic `ValidationError` or skip the substring check on the inner + message) + +Add a `std::error::Error::source()` call to confirm it returns `None` +(the impl is the blanket `impl std::error::Error for AlkTypeError {}`). + +**Lift**: 9 uncovered lines → 100% for `error.rs`. Trivial effort, no +new infrastructure. + +--- + +### S2. `validate()` vs `is_valid()` asymmetry in the validation layer (validation.rs, macros.rs) + +**Files**: `src/validation.rs` (the `Int64Validator`, `Uint64Validator`, +`StringValidator`, `BytesValidator`, `EnumValidator`, `TimestampValidator` +`validate()` methods); `src/macros.rs` (the macro-generated +`validate()` methods for `Int8/16/32`, `Uint8/16/32`, `Float32/64`, +`Struct/Union/Array/Record/Boolean`) + +**Problem**: The tests heavily exercise `is_valid()` (the bool check) +but rarely call `validate()` (the `Result`-returning method). The +`AlkTypeEngine::validate_json` public API calls `validator.validate()`, +so the `Result` paths are the ones consumers actually hit when they want +error messages — but they're uncovered. + +Specifically uncovered: + +- `Int64Validator::validate` / `Uint64Validator::validate` — the `Ok` + and `Err` arms for both. `is_valid()` is tested; `validate()` is not. +- `StringValidator::validate` — the `maxLength` exceeded arm and the + non-string arm. The `Ok` arm is hit via `is_valid()` but not + `validate()`. +- `BytesValidator::validate` — same shape as StringValidator. +- `EnumValidator::validate` — the whole method (it is a no-op that + returns `Ok(())`, but it should be called at least once). +- `TimestampValidator::validate` — the `Ok` arm (valid RFC 3339) and the + `Err` arm (non-timestamp string). +- Macro-generated `validate()` for all numeric / type-check validators — + the `Ok` arm and the `Err` arm of each. + +The factory error arms are also uncovered: `string_factory`, +`bytes_factory`, `enum_factory`, `timestamp_factory` each have a branch +that rejects a keyword value that isn't `true` (or `true`/object for +string/bytes). Only the `Ok` path of each factory is exercised. + +**Fix**: ~40 lines of unit tests. For each validator, call +`validator.validate(&instance)` on a valid instance (assert `Ok(())`) and +an invalid instance (assert `Err`, check the error message substring). +For the factories, build a validator with a malformed keyword value +(e.g. `{"AlkType:String": 42}`, `{"AlkType:Enum": false}`) and assert +`build_validator` returns `Err(AlkTypeError::Schema(_))`. + +**Lift**: ~65 uncovered lines in `validation.rs`, ~35 in `macros.rs`. +Brings both files above 95% line coverage. Easy — the test patterns +already exist for `is_valid()`, just call the `Result`-returning twin. + +--- + +### S3. Engine `read_field` for String and Struct (engine.rs, ~12 lines) + +**File**: `src/engine.rs:267–282` + +**Problem**: `AlkTypeEngine::read_field` in aligned mode is tested for +`Uint8`, `Uint32`, and `String` (the last only via a string-specific +test). The `AlkTypeKind::String` arm at line 267 and the +`AlkTypeKind::Struct` arm at line 279 are uncovered — the Struct arm +returns `FieldValue::Struct { start, end }` and is the path consumers +use to get a byte range for nested-struct recursion. + +The two `AlkTypeError::Offset` error arms at lines 210–216 (field schema +not found / no `AlkType:*` kind) are also uncovered. These fire when the +`OffsetMap` and the schema tree disagree about a field's existence — a +defensive path, but it's public API surface that should produce a +sensible error. + +**Fix**: ~20 lines of tests, extending the existing +`read_field_aligned_*` tests in `src/engine.rs`: + +- A test with a nested struct field, asserting `read_field` returns + `FieldValue::Struct { start, end }` matching the `OffsetMap` range. +- A test calling `read_field` on a field path that doesn't exist in the + schema tree (but does exist in the offset map by construction — or + skip this if the two are always in sync; the error arm is defensive). +- A `String` read via `read_field` (the existing + `read_field_aligned_reads_string_length_prefixed` test covers this — + verify it is actually hitting line 267 and not just the data_access + call). + +**Lift**: 12 uncovered lines, lifts `engine.rs` from 96.8% to ~100%. +Easy. + +--- + +### S4. `SequentialReader` error paths and uncommon reads (sequential_reader.rs, ~117 lines) + +**File**: `src/sequential_reader.rs` (many locations — see below) + +**Problem**: This is the largest gap area by line count. The uncovered +lines fall into four groups: + +1. **Union error paths** (~30 lines): the `read_union_value` overflow + guards (`variant_start + disc_size overflows`, `after_disc + + variant_size overflows`, `union end ... overflows`), the missing- + properties and missing-discriminator-field schema errors, and the + unknown-discriminator-value access error for field-name unions. These + are defensive guards, but the "unknown discriminator value" path is a + real consumer-facing error that should produce a clear message. + +2. **Array error paths** (~15 lines): the `array schema is not an + object`, `declared fixed count but minItems is absent`, `items schema + has no AlkType:* kind`, `array size × stride overflows`, and `array + end overflows` arms. The `items schema has no AlkType:* kind` arm is + a real validation error a consumer would hit with a malformed schema. + +3. **Discriminator helper arms** (~10 lines): `read_byte_discriminator` + only exercises the `Uint8` arm; the `Uint16` and `Uint32` arms are + uncovered. `discriminator_string_value` only exercises the `String` + arm; `U8`/`U16`/`U32`/`Enum` are uncovered. + +4. **Miscellaneous** (~60 lines): the `schema()` accessor, the + "walked backwards" guards (defensive), the + `resolve_and_walk_variant` `Union`-as-variant arm (nested unions), + the `resolve_variant_schema` `None`-return arms, and several + `panic!("expected ...")` arms in test helpers (these are unreachable + by construction — they're match exhaustiveness guards). + +**Fix**: ~50 lines of tests, split by group: + +- **Group 1**: feed a `read_next` call a buffer with a byte-discriminator + union whose discriminator value isn't in the mapping (the existing + `union_unknown_discriminator_returns_access_error` covers the byte + case; add the field-name case). Add a field-name union whose + `properties` is missing entirely (schema error). +- **Group 2**: an array with `items` set to a schema with no + `AlkType:*` keyword (schema error). An array with `minItems` set but + `maxItems` absent and a count prefix that's too short (access error — + already partly covered by `buffer_too_short_returns_access_error`). +- **Group 3**: a `read_next` with a `Uint16` byte discriminator and a + `Uint32` byte discriminator (extend the existing + `byte_discriminator_union_reads_value` with a parametrized disc type). + A `discriminator_string_value` test with `U8`/`Enum` field + discriminators (extend `field_discriminator_union_reads_value`). +- **Group 4**: a `schema()` accessor one-liner. A nested-union-as- + variant test (a union whose mapping points at another union). The + "walked backwards" guards and `panic!("expected ...")` arms are + unreachable by construction — leave them. + +**Lift**: ~60 uncovered lines (excluding the unreachable guards), lifts +`sequential_reader.rs` from 86.5% to ~92%. Medium effort — the test +patterns exist, but the error-path scaffolding needs a few crafted +schemas and buffers. + +--- + +### S5. `LayoutBuilder` error paths (layout_builder.rs, ~85 lines) + +**File**: `src/layout_builder.rs` (many locations) + +**Problem**: Similar shape to S4 but on the write-side. The uncovered +lines are almost all `AlkTypeError::Offset` construction arms for +defensive guards: `type_size returned None for fixed kind`, `offset + +size overflows`, `prefix + data size overflows`, array/union error +constructions, and the `unreachable!` exhaustiveness guard. Three +`panic!("expected Offset, ...")` arms in test helpers are unreachable by +construction. + +The one error path that's a real consumer-facing validation error is the +"TUnion variant must be AlkType:Struct" arm (lines 528, 591) — a +consumer who points a union mapping at a non-struct variant should get +a clear error. This is uncovered for both byte-offset and field-name +discriminators. + +**Fix**: ~15 lines of tests: + +- A union mapping whose variant is `AlkType:Uint32` (not a struct) — + assert `build` returns `Err(AlkTypeError::Offset { .. })` with a + reason containing "variant must be AlkType:Struct". Test both + byte-offset and field-name discriminator variants. +- The remaining ~70 lines are overflow guards and `unreachable!` — leave + them (same reasoning as S8 below). + +**Lift**: ~6 uncovered lines (the two "variant must be Struct" arms). +The rest is defensive. Low effort for the consumer-facing error; the +guards are S8 territory. + +--- + +### S6. `schema.rs` unused public accessors (schema.rs, ~34 lines) + +**File**: `src/schema.rs` + +**Problem**: Several public API methods are never called by any +internal code or test: + +- `AlkTypeKind::as_str()` — ~15 of the 19 match arms are uncovered (only + `Uint32`, `Float32`, `Float64`, `Timestamp` are hit by existing + tests). The method is public; consumers use it for error messages and + debugging. +- `AlkTypeKind::needs_endian()` — completely uncovered. The method + exists but no internal caller uses it (the engine and data_access + layer infer endianness need from the kind directly). +- `AlkTypeKind::is_composite()` — completely uncovered. Same situation. +- `AlkTypeKind::get_alktype_kind_enum()` — the strict (boolean-form-only) + variant. Tests use `get_alktype_kind_loose_enum` instead. + +**Fix**: ~15 lines of tests: + +- A `as_str()` round-trip test: `for kind in [all 19 variants] { assert + kind.as_str().parse::().unwrap() == kind }`. +- A `needs_endian()` test asserting `true` for the multi-byte kinds and + `false` for the rest. +- An `is_composite()` test asserting `true` for `Struct/Union/Array/ + Record` and `false` for the rest. +- A `get_alktype_kind_enum()` test asserting it returns `None` for the + object-annotation form (mirroring the existing + `get_alktype_kind_ignores_object_keyword` test for the loose variant). + +**Lift**: ~34 uncovered lines, lifts `schema.rs` from 91.2% to ~100%. +Trivial — these are pure functions over the enum. + +**Question**: `needs_endian()` and `is_composite()` are currently unused +internally. If they have no external consumer either (this is a fresh +crate, no consumers yet), consider whether they're dead code that should +be removed, or kept as part of the intended public API. If kept, test +them; if removed, the coverage gap disappears. This is a design +decision, not a coverage decision. + +--- + +### S7. `offset_map.rs` defensive guards and one missing-properties path (offset_map.rs, ~35 lines) + +**File**: `src/offset_map.rs` + +**Problem**: The uncovered lines are almost all `AlkTypeError::Offset` +construction arms for defensive guards: `type_size returned None`, +`element kind has no fixed size`, `array schema is not an object`, +`TArray is missing 'items'`, `could not resolve TArray items schema`, +`TArray element schema has no AlkType:* kind`. One `unreachable!` +exhaustiveness guard and one `round_up`/`align_up` early-return arm. + +The one path that's a real consumer-facing error is the "struct schema +has no 'properties' object" arm — a nested struct missing `properties` +should produce a schema error. This is uncovered. + +**Fix**: ~5 lines: a test with a nested struct whose `properties` is +absent, asserting `OffsetMap::compute` returns +`Err(AlkTypeError::Schema(_))`. + +**Lift**: ~3 uncovered lines (the missing-properties arm). The rest is +defensive (S8). Low effort. + +--- + +### S8. Defensive overflow guards — the long tail (all modules, ~150 lines total) + +**Files**: `src/data_access.rs`, `src/layout_builder.rs`, +`src/sequential_reader.rs`, `src/tunion.rs`, `src/offset_map.rs` + +**Problem**: The single largest category of uncovered lines across the +crate is `checked_add(...).ok_or_else(|| ... overflows usize)` guards. +These appear in `data_access.rs` (the read/write array and string +functions), `layout_builder.rs` (every offset computation), +`sequential_reader.rs` (every offset + size computation in the union/ +array/record readers), `tunion.rs` (the discriminator offset + size +guards), and `offset_map.rs` (the array offset computations). + +They are structurally hard to test because they require +`usize::MAX`-adjacent inputs to trigger the overflow. The code is simple +— a `format!` constructing an error message — and the guards are +correct-by-construction (they wrap a `checked_add` that returns `None` +on overflow). + +**Fix**: Two options, in increasing fidelity: + +1. **Skip them** — these are defensive guards over trivial format- + string construction. They're low-risk gaps. Document in this review + that they're intentionally left, and move on. This is the + recommended path for v1. +2. **Targeted overflow tests** — for each `checked_add` site, construct + an input where `offset + size` would overflow `usize` (e.g. `offset = + usize::MAX - 1`, `size = 2`). This requires crafted buffers or + `var_sizes` entries near `usize::MAX`. ~20 tests, ~150 lines, covers + the full long tail. High effort, low value — the code is a format + string. + +**Lift**: up to ~150 lines if option 2 is taken. Option 1 is +recommended — the effort/value ratio is poor and the guards are +correct-by-construction. + +--- + +## Recommended Order + +1. **S1** (error.rs Display) — trivial, 9 lines, the only 0% file. + Do this first; it's a 5-minute fix. +2. **S2** (validate() paths) — easy, ~100 lines across validation.rs and + macros.rs. Largest single leverage: lifts two files above 95% and + exercises the public `validate_json` error-return path. +3. **S6** (schema.rs accessors) — trivial, ~15 lines, lifts schema.rs to + ~100%. Also resolve the "keep or remove `needs_endian`/ + `is_composite`" design question. +4. **S3** (engine String/Struct read) — easy, ~20 lines, lifts engine.rs + to ~100%. +5. **S4** (sequential reader error paths) — medium, ~50 lines for ~60 + lifted lines. The discriminator-helper arms and the unknown-value + error paths are the high-value subset; the overflow guards are S8. +6. **S5** (layout_builder "variant must be Struct") — low effort, ~6 + lines for the consumer-facing error. The rest is S8. +7. **S7** (offset_map missing-properties) — low effort, ~3 lines. The + rest is S8. +8. **S8** (overflow guards) — recommended to skip for v1. Document as + intentionally deferred; revisit if the crate ever handles untrusted + offsets from untrusted input. + +After S1–S7 (skipping S8), estimated coverage: **~95% lines, ~92% +functions**. The remaining ~5% is the overflow-guard long tail. + +--- + +## Notes + +- All line numbers refer to the tree at commit `6d61429` (the rebranding + cleanup commit). The coverage was measured on that tree. +- No critical or warning findings: this pass confirms the gaps are + testing-only. The underlying logic was reviewed during the rebranding + sweep (the full source was read; all `AlkType:*` keyword strings, the + `AlkTypeKind` enum, the parsers, the validators, and the public API + are consistent). +- The crate has no feature flags (`[features] default = []`), so + `--all-features` is not needed. If feature flags are added later + (e.g. `no_std` per OQ-002), this pass should be re-run with + `--all-features` to catch the cfg-gated paths. +- The `panic!("expected ...")` arms in test helpers across + `sequential_reader.rs`, `tunion.rs`, `layout_builder.rs`, and + `data_access.rs` are unreachable by construction (they're match + exhaustiveness guards on `AlkTypeError` variants in `assert!` + matchers). They show as uncovered but are not test gaps — they're + defensive code in test helpers. \ No newline at end of file