Resolve review #005: refine 0.3.0 plan + ADR-011/012
Resolve all 11 findings from the 0.3.0 plan review (#005) in one docs-only pass. No source changes; the crate still builds/tests at v0.2.0. The one substantive decision change is M3 (per user direction: ship ValidationPlan in 0.3.0, no more hedging); the rest are spec corrections or pre-implementation refinements to types that do not yet exist on main. - H1: refine ADR-011 CompositePlan::Union to carry shared: Option<Box<ReadPlan>> (field-disc shared fields) and variants: Vec<(String, CompositePlan)> (drop VariantPlan/ VariantKind). Plan phase 1 implements the refined shape. - H2: plan phase 2 specifies ReadPlan stores schema: Arc<Value> (not &Value), avoiding the self-referential struct ADR-011 rejects. Verified serde_json::Value: Hash + Eq holds with preserve_order, so phase 6 derives are not blocked. - M1: nested-union support falls out of the H1 shape refinement (a variant can be CompositePlan::Union) — option (a) from the review, no behavioral drop vs 0.2.0, no Semver regression row. - M2: plan phase 5 adds an explicit first sub-step to derive Hash on Endian and VariableEncoding in src/schema.rs (additive, semver-safe prerequisite the original plan omitted). - M3: reverse the ValidationPlan deferral. ADR-012's "Deferring ValidationPlan" becomes "ValidationPlan — in scope for 0.3.0"; new ADR-012 §3 commits the decision (compiled form, no per-buffer BastDoc walk, Hash + Eq + fingerprint()) and defers only the concrete shape to a follow-on design session + the plan's new phase 7. Plan gains phase 7 (ValidationPlan); old phase 7 (bump) renumbered to phase 8. ADR-011's Out-of-scope and Scope Boundaries bullets updated to point at ADR-012 §3. The deferral black hole this review's methodology flagged is closed: the work is committed with a concrete reactivation trigger, not hedged into an unplanned future. - L1: plan phase 2 corrects the dummy_field_for/ty_source removal claim — only packed-side call sites go away; the helpers stay for the aligned materialize_leaf_at path. - L2: plan phase 2 states the packed-vs-aligned materialize_typeref_packed split (packed gets a new plan-walking function; the existing function stays for aligned). - L3: plan phase 5 adds a Scope Boundary note — aligned materialize's BastDoc structure walk is the permanent 0.3.0 design; an AlignedPlan is out of scope, tracked as an OQ. - N1: fix "back-comat" -> "back-compat" typo. - N2: plan phase 1 verification adds the read_plan_is_send_sync static-bound assertion test ADR-011 requires. - N3: Semver Contract table notes the Result drop on SequentialReader::new (Result<Self, AlkTypeError> -> Self) alongside the argument-type change. Also: ADR-012 title -> "Plan Fingerprinting, ValidationPlan, and Closing the Deferred M1 Sites in 0.3.0"; §3 (Fingerprinting OffsetMap) renumbered to §4; README ADR table updated; review #005 gets a Resolution section recording how each finding was closed. Verification (docs-only change, v0.2.0 unchanged): cargo test --release ok (310 crate + 86 integration + 2 doctests) cargo clippy --all-targets -- -D warnings ok cargo doc --no-deps ok
This commit is contained in:
1 parent
0e7921a02a
commit
e461f01c97
5 files changed
+696
-165
No files matched your search
@@ -46,7 +46,7 @@ format definition; the engine is generic.
|
||||
| [009](decisions/009-builder-api.md) | Builder API for Schema Construction | Fluent Rust API producing `serde_json::Value`; covers BAST kinds + standard JSON Schema; resolves OQ-003. *Output format amended to BAST / standard JSON Schema by ADR-BAST.* |
|
||||
| [010](decisions/010-generalized-validation-validate-bytes.md) | Generalized Validation — `validate_bytes` on `AlkTypeEngine` | Single-call binary-buffer validation; materialize `Value` from bytes, then validate. *Validation step amended to the BAST-native validator by ADR-VAL-SPLIT.* |
|
||||
| [011](decisions/011-compiled-read-plan-for-packed-mode.md) | Compiled Read Plan for Packed Mode | `ReadPlan` — the packed read-side compiled form, symmetric to `OffsetMap` (aligned) and `PackedLayout` (packed write). Closes review #004's 400x read-path gap; retires ADR-007's "re-parse on demand" framing. *Accepted.* |
|
||||
| [012](decisions/012-plan-fingerprinting-and-m1-closure.md) | Plan Fingerprinting and Closing the Deferred M1 Sites in 0.3.0 | `ReadPlan`/`OffsetMap` `Hash + Eq` + `fingerprint()`; owned `BastDoc` (lifetime removal); `OffsetMap` carries `LeafMeta` to close the aligned-side M1 sites. Bundles with ADR-011 into one 0.3.0 breaking release. *Proposed.* |
|
||||
| [012](decisions/012-plan-fingerprinting-and-m1-closure.md) | Plan Fingerprinting, ValidationPlan, and Closing the Deferred M1 Sites in 0.3.0 | `ReadPlan`/`OffsetMap`/`ValidationPlan` `Hash + Eq` + `fingerprint()`; owned `BastDoc` (lifetime removal); `OffsetMap` carries `LeafMeta` to close the aligned-side M1 sites; `ValidationPlan` retires the interpretive `bast_validation` walk (review #005 M3 reversed the original deferral). Bundles with ADR-011 into one 0.3.0 breaking release. *Proposed.* |
|
||||
|
||||
## Relevant Open Questions
|
||||
|
||||
|
||||
@@ -8,6 +8,21 @@ POC on branch `readplan-poc` confirmed the `ReadPlan` shape covers
|
||||
every `BastType` arm in the current read loop before implementation
|
||||
began (see "POC coverage" at the end).
|
||||
|
||||
**Refinements on ADR-012 acceptance (2026-08-20, review #005):** the
|
||||
`CompositePlan::Union` shape was refined to carry `shared:
|
||||
Option<Box<ReadPlan>>` (field-disc union shared fields, resolving POC
|
||||
Finding 1 / review #005 H1) and to drop `VariantPlan`/`VariantKind`
|
||||
in favor of `variants: Vec<(String, CompositePlan)>` (resolving review
|
||||
#005 M1 — nested unions now work by `CompositePlan` recursion,
|
||||
restoring the 0.2.0 capability the POC rejected). The "BastDoc
|
||||
unchanged" scope statement stands as the ADR-011-only view; ADR-012
|
||||
§2a subsequently makes `BastDoc` owned. The `ValidationPlan` this
|
||||
ADR's "Out of scope" originally deferred indefinitely is now in
|
||||
0.3.0 via ADR-012 §3 (review #005 M3 reversed the deferral). These
|
||||
are pre-implementation refinements to types that do not yet exist on
|
||||
`main`; the ADR-011 decision (a compiled `ReadPlan` for packed reads)
|
||||
is unchanged.
|
||||
|
||||
## Context
|
||||
|
||||
Review #004 (`docs/reviews/004-performance-review.md`) measured the
|
||||
@@ -145,7 +160,8 @@ pub enum CompositePlan {
|
||||
Struct(ReadPlan),
|
||||
Union {
|
||||
disc: DiscriminatorPlan,
|
||||
variants: Vec<(String, VariantPlan)>,
|
||||
shared: Option<Box<ReadPlan>>,
|
||||
variants: Vec<(String, CompositePlan)>,
|
||||
},
|
||||
Array {
|
||||
element: Box<CompositePlan>,
|
||||
@@ -165,12 +181,30 @@ pub enum DiscriminatorPlan {
|
||||
|
||||
Nested structs share the `ReadPlan` shape (a struct field's `body` is
|
||||
`CompositePlan::Struct(ReadPlan)`). Union variants are pre-resolved:
|
||||
each `(key, VariantPlan)` entry carries the variant's `ReadPlan`, so
|
||||
dispatch is a flat lookup + recurse — no `resolve_typeref_as_def` at
|
||||
read time. Array element strides are precomputed (`element_stride = 0`
|
||||
each `(key, CompositePlan)` entry carries the variant's compiled body,
|
||||
so dispatch is a flat lookup + recurse — no `resolve_typeref_as_def` at
|
||||
read time. A variant may itself be `CompositePlan::Union { ... }`, so
|
||||
**nested unions** (a union variant that is itself a union, which the
|
||||
0.2.0 reader supports via `resolve_and_walk_variant`'s `Union` arm) are
|
||||
covered by ordinary recursion; no separate `VariantKind` enum is
|
||||
needed. Array element strides are precomputed (`element_stride = 0`
|
||||
signals variable-length elements, same convention as today's
|
||||
`FieldValue::Array`).
|
||||
|
||||
`Union.shared` carries the union's declared `fields` (the discriminator
|
||||
field + any shared fields) for the field-name-discriminator case —
|
||||
`DiscriminatorPlan::Field.field_index` indexes into `shared`, and the
|
||||
read loop walks `shared` first, then looks up and walks the selected
|
||||
variant's `CompositePlan` starting after the shared fields. The
|
||||
byte-offset-discriminator case has no shared fields (`shared: None`):
|
||||
the discriminator byte is read at `disc.offset` and the variant starts
|
||||
immediately after the discriminator size. The POC's `plan_read_union`
|
||||
`Field` arm stub (Finding 1) is replaced by this `shared` sub-plan;
|
||||
there is no separate `VariantPlan`/`VariantKind` type in the production
|
||||
shape — the POC's `VariantPlan { kind, plan }` wrapper is dropped in
|
||||
favor of recursing on `CompositePlan` directly, which is what makes
|
||||
nested-union support fall out for free.
|
||||
|
||||
### Construction
|
||||
|
||||
```rust
|
||||
@@ -215,10 +249,16 @@ plan being immutable owned data, but the implementation should add a
|
||||
- `materialize_aligned` is unchanged (already takes `&OffsetMap`, a
|
||||
compiled form).
|
||||
- `ReadPlan` is a new public type, re-exported from `lib.rs`.
|
||||
- `BastDoc` and the `Bast*` types are **unchanged** — they remain the
|
||||
validation-side typed tree, borrowed, as today. This is a smaller
|
||||
breakage than review #004's Option A (which changed `BastDoc<'a>` →
|
||||
`BastDoc` and every `Bast*` signature).
|
||||
- `BastDoc` and the `Bast*` types are **unchanged by this ADR** — they
|
||||
remain the validation-side typed tree, borrowed, as today. This is a
|
||||
smaller breakage than review #004's Option A (which changed
|
||||
`BastDoc<'a>` → `BastDoc` and every `Bast*` signature).
|
||||
**Note (added on ADR-012 acceptance):** ADR-012 §2a subsequently
|
||||
makes `BastDoc` owned, riding the same 0.3.0 bump. That is an
|
||||
ADR-012 change, not an ADR-011 change; ADR-011's scope statement
|
||||
stands as the ADR-011-only view. With ADR-012 §3 (ValidationPlan,
|
||||
now in 0.3.0), `bast_validation` will also stop being the permanent
|
||||
home of the `BastDoc` walk — see ADR-012.
|
||||
|
||||
The crate is pre-1.0 with two in-house downstream consumers
|
||||
(`alktty`, `alkcall`), both of which will be updated with the bump.
|
||||
@@ -248,12 +288,15 @@ The crate is pre-1.0 with two in-house downstream consumers
|
||||
- **`bast_validation`** — the BAST-native value-domain validator walks
|
||||
`BastDoc` to check constraints (`maxLength`, enum string values,
|
||||
union variant keys, integer ranges). These are value-domain checks,
|
||||
not byte-position walks; they don't benefit from a read plan and
|
||||
not byte-position walks; they don't benefit from a *read* plan and
|
||||
would require a separate "validation plan" with a different shape.
|
||||
Validation is not a per-chunk hot loop (AGENTS.md: validation is
|
||||
opt-in per operation). A future `ValidationPlan` is a two-way door
|
||||
if a bench motivates it; for now `bast_validation` keeps walking
|
||||
`BastDoc`, which remains the validation-side typed tree.
|
||||
**Not in scope for ADR-011** — but no longer deferred indefinitely:
|
||||
ADR-012 §3 brings a `ValidationPlan` into 0.3.0. The "not a hot
|
||||
loop" framing this paragraph originally relied on was re-evaluated
|
||||
and rejected (see ADR-012 §3): read+validate on untrusted streams
|
||||
makes validation hot in the same sense review #004 measured for
|
||||
the read path. For 0.3.0 as accepted by ADR-011 alone,
|
||||
`bast_validation` keeps walking `BastDoc`; ADR-012 §3 closes that.
|
||||
- **`LayoutBuilder` / `PackedLayout`** — the packed write-side already
|
||||
has a compiled form (`PackedLayout`). `LayoutBuilder::build`
|
||||
(`src/layout_builder.rs:190`) re-parses `BastDoc::new` per `build()`
|
||||
@@ -363,19 +406,25 @@ The crate is pre-1.0 with two in-house downstream consumers
|
||||
## Scope Boundaries (What This Is Not)
|
||||
|
||||
- **Not a `BastDoc` replacement.** `BastDoc` stays as the
|
||||
validation-side typed tree, borrowed from `&Value`, unchanged. The
|
||||
`Bast*` types and their signatures are not touched. Validation
|
||||
(`bast_validation`), aligned one-shot reads/writes
|
||||
(`engine.rs:334,467`), and `LayoutBuilder::build` continue to walk
|
||||
`BastDoc` until they get their own compiled forms (additive, later).
|
||||
validation-side typed tree within ADR-011's scope (borrowed from
|
||||
`&Value`, unchanged). The `Bast*` types and their signatures are not
|
||||
touched by ADR-011. Validation (`bast_validation`), aligned one-shot
|
||||
reads/writes (`engine.rs:334,467`), and `LayoutBuilder::build`
|
||||
continue to walk `BastDoc` within ADR-011's scope. **ADR-012
|
||||
subsequently revises two of these:** `BastDoc` becomes owned (§2a)
|
||||
and `bast_validation` adopts a `ValidationPlan` (§3), both riding
|
||||
the same 0.3.0 bump. ADR-011's scope statement is the ADR-011-only
|
||||
view and is not re-litigated here.
|
||||
- **Not a flat lookup table.** Packed positions are data-dependent;
|
||||
the plan is a read program (instructions to walk), not a `(path,
|
||||
offset)` table. This is inherent to packed sequential layout
|
||||
(ADR-002), not a limitation of this design.
|
||||
- **Not a validation plan.** `bast_validation`'s value-domain checks
|
||||
(maxLength, enum values, union variant keys, integer ranges) are a
|
||||
different concern and a different shape. They stay on `BastDoc`. A
|
||||
future `ValidationPlan` is a two-way door.
|
||||
different concern and a different shape from `ReadPlan`. They are
|
||||
out of ADR-011's scope; ADR-012 §3 adds a `ValidationPlan` in 0.3.0
|
||||
rather than leaving validation on an interpretive `BastDoc` walk
|
||||
indefinitely.
|
||||
- **Not the review's Option A or Option B.** It is the "compiled form"
|
||||
path the review pointed at but did not name: Option A (make `BastDoc`
|
||||
own its data) kills the re-parse but leaves the read loop as a
|
||||
@@ -448,13 +497,13 @@ closes L2. L1 falls out at step 2. The aligned-side M1 paths
|
||||
`validate_bytes` (packed) consumes the `ReadPlan` via
|
||||
`materialize_packed`
|
||||
|
||||
## Future capabilities (partly in 0.3.0 via ADR-012)
|
||||
## Future capabilities (in 0.3.0 via ADR-012)
|
||||
|
||||
The deterministic-compile property of `ReadPlan` is a prerequisite for
|
||||
several capabilities. ADR-012 ("Plan Fingerprinting and Closing the
|
||||
Deferred M1 Sites in 0.3.0") picks up the first two items below into
|
||||
the 0.3.0 release so they ship with this ADR's breaking changes in one
|
||||
round of downstream churn, not two:
|
||||
several capabilities. ADR-012 ("Plan Fingerprinting, ValidationPlan,
|
||||
and Closing the Deferred M1 Sites in 0.3.0") picks up all three items
|
||||
below into the 0.3.0 release so they ship with this ADR's breaking
|
||||
changes in one round of downstream churn, not two or three:
|
||||
|
||||
- **Fingerprinting the plan** (`#[derive(Hash)]` + a `fingerprint()`
|
||||
method) for cross-run caching of compiled plans, disk-cached plans,
|
||||
@@ -466,9 +515,12 @@ round of downstream churn, not two:
|
||||
candidate listed here as "not a new type — extend the existing
|
||||
compiled forms (`PackedLayout`/`OffsetMap`) and cache the parse."
|
||||
- A `ValidationPlan` that follows the same compile-once-walk-many
|
||||
pattern for `bast_validation`. → **Deferred (ADR-012).** Different
|
||||
shape (value-domain, not byte-position); not a hot loop; separate
|
||||
ADR if a bench motivates it.
|
||||
pattern for `bast_validation`. → **In 0.3.0 (ADR-012 §3).** Different
|
||||
shape (value-domain, not byte-position) but the same class of
|
||||
per-buffer re-walk cost on the read+validate-on-untrusted-input
|
||||
common case. ADR-012 owns the shape decision and the implementation
|
||||
plan scopes it. (Originally deferred by ADR-012 as "not a hot loop";
|
||||
review #005 M3 reversed the deferral — see ADR-012 §3.)
|
||||
|
||||
None of the in-0.3.0 items justify this ADR; the 400x read-path gap
|
||||
does. They are listed here as forward references and to record that
|
||||
@@ -487,7 +539,15 @@ two spots where a plan arm could subtly miss a case:
|
||||
`DiscriminatorPlan::Byte { offset, disc_type }` and
|
||||
`DiscriminatorPlan::Field { name, field_index }`. The field-name case
|
||||
pre-resolves the discriminator field's `ReadKind` so dispatch reads
|
||||
it from the plan, not from a re-parsed `BastField`.
|
||||
it from the plan, not from a re-parsed `BastField`. The
|
||||
field-disc union's declared `fields` (discriminator + any shared
|
||||
fields) are carried as a sub-`ReadPlan` on `CompositePlan::Union`'s
|
||||
`shared` field (a refinement of the POC shape, which stubbed the
|
||||
`Field` arm — Finding 1); the production read loop walks `shared`
|
||||
first, then the selected variant's `CompositePlan`. Nested-union
|
||||
variants (a variant that is itself a union) are covered by ordinary
|
||||
`CompositePlan` recursion; the POC rejected them, the 0.2.0 reader
|
||||
accepts them, and the production shape restores parity.
|
||||
- **Array variable-element-stride (`element_stride = 0`).** Covered:
|
||||
`CompositePlan::Array { element, count, element_stride }` preserves
|
||||
the `0`-signals-variable convention, and the read loop walks
|
||||
|
||||
@@ -1,11 +1,16 @@
|
||||
# ADR-012: Plan Fingerprinting and Closing the Deferred M1 Sites in 0.3.0
|
||||
# ADR-012: Plan Fingerprinting, ValidationPlan, and Closing the Deferred M1 Sites in 0.3.0
|
||||
|
||||
## Status
|
||||
|
||||
Proposed. Bundles two pieces of work into the 0.3.0 release so the
|
||||
crate ships one round of breaking changes, not two. Companion to
|
||||
[ADR-011](011-compiled-read-plan-for-packed-mode.md) (the `ReadPlan`)
|
||||
and the [0.3.0 implementation plan](../../plans/030-compiled-forms.md).
|
||||
Proposed. Bundles three pieces of work into the 0.3.0 release so the
|
||||
crate ships one round of breaking changes, not two (or three). The
|
||||
three pieces: (a) fingerprinting `ReadPlan`/`OffsetMap`, (b) closing
|
||||
the deferred M1 sites via an owned `BastDoc` + `OffsetMap` `LeafMeta`,
|
||||
and (c) a `ValidationPlan` that retires the interpretive
|
||||
`bast_validation` walk (added by reversing the original "defer
|
||||
`ValidationPlan`" decision — see "ValidationPlan — in scope for
|
||||
0.3.0" below). Companion to [ADR-011](011-compiled-read-plan-for-packed-mode.md)
|
||||
(the `ReadPlan`) and the [0.3.0 implementation plan](../../plans/030-compiled-forms.md).
|
||||
|
||||
## Context
|
||||
|
||||
@@ -15,21 +20,26 @@ and deferred two things to "future capabilities":
|
||||
1. **Fingerprinting the plan** for cross-run caching, disk-cached
|
||||
compiled plans, and `alkcall` hub/spoke schema-version handshakes.
|
||||
2. **A `WritePlan` and/or `ValidationPlan`** following the same
|
||||
compile-once-walk-many pattern for the deferred M1 sites.
|
||||
compile-once-walk-many pattern for the deferred M1 sites and the
|
||||
validation walk.
|
||||
|
||||
ADR-011 also explicitly deferred the aligned-side M1 sites
|
||||
(`engine.rs:334,467` `read_field`/`write_field`;
|
||||
`layout_builder.rs:190` `LayoutBuilder::build`) as "a deliberate
|
||||
reversible bet that an aligned-mode hot loop won't emerge."
|
||||
|
||||
This ADR retires both deferrals in one release. The reasoning is
|
||||
This ADR retires the deferrals in one release. The reasoning is
|
||||
timing: 0.3.0 is already a breaking bump (ADR-011 changes
|
||||
`SequentialReader::new` and `materialize_packed` signatures), and the
|
||||
crate has no real downstream consumers yet (only `alktty`/`alkcall`,
|
||||
both in-house). Doing both pieces now costs one round of downstream
|
||||
churn instead of two, and the fingerprinting work cuts across both
|
||||
`ReadPlan` and `OffsetMap` — splitting would create a cross-release
|
||||
dependency that's cleaner in one release.
|
||||
both in-house). Doing all three pieces now costs one round of
|
||||
downstream churn instead of two or three, and the fingerprinting work
|
||||
cuts across both `ReadPlan` and `OffsetMap` — splitting would create
|
||||
a cross-release dependency that's cleaner in one release. The
|
||||
`ValidationPlan` inclusion follows the same logic applied to the
|
||||
validation walk: deferring it would create a *second* breaking change
|
||||
to `validate_bytes`/`bast_validation` after 0.3.0, which is exactly
|
||||
the round of downstream churn this release exists to retire.
|
||||
|
||||
### Reframing "WritePlan"
|
||||
|
||||
@@ -61,16 +71,49 @@ fixes are "cache the parse" and "extend the compiled form with leaf
|
||||
metadata," not "add a third compiled form." This serves the
|
||||
minimal-public-API-changes goal better than a literal `WritePlan`.
|
||||
|
||||
### Deferring `ValidationPlan`
|
||||
### `ValidationPlan` — in scope for 0.3.0 (no longer deferred)
|
||||
|
||||
The BAST-native validator (`bast_validation`) walks `BastDoc` to
|
||||
check value-domain constraints (enum value sets, integer ranges,
|
||||
`maxLength` caps, union variant keys). This is a different shape from
|
||||
`ReadPlan`/`OffsetMap` (value-domain, not byte-position) and is not a
|
||||
hot loop — validation is opt-in per operation per AGENTS.md. A future
|
||||
`ValidationPlan` is a two-way door with its own ADR if a bench
|
||||
motivates it; for 0.3.0 it is scope creep that would delay the
|
||||
read-path fix. Explicitly deferred.
|
||||
`maxLength` caps, union variant keys). This is a different shape
|
||||
from `ReadPlan`/`OffsetMap` (value-domain, not byte-position), and
|
||||
the earlier framing deferred it as "not a hot loop — validation is
|
||||
opt-in per operation per AGENTS.md."
|
||||
|
||||
**That deferral is reversed.** The "not a hot loop" dismissal
|
||||
under-counted the common case: **read + validate together on
|
||||
untrusted input.** The downstream `alkcall` consumer accepts schemas
|
||||
from arbitrary internet peers in a hub/spoke topology (AGENTS.md §3);
|
||||
the common operation on an incoming frame is "read it, then validate
|
||||
it before acting." `validate_bytes` (ADR-010) is therefore called
|
||||
once per incoming buffer, and each call re-walks `BastDoc` for
|
||||
validation even after ADR-011 makes the *read* half plan-fast. That
|
||||
is the same class of per-buffer interpretive cost review #004 measured
|
||||
for the read path (400x per chunk), on a different code path, on the
|
||||
operation the untrusted-input discipline actually requires.
|
||||
|
||||
The cost-of-inaction framing that the original deferral relied on was
|
||||
also wrong: a `ValidationPlan` introduced *after* 0.3.0 would be a
|
||||
breaking change to `validate_bytes`'s contract and to the
|
||||
`bast_validation` public surface, forcing rework of `alktty`/`alkcall`
|
||||
— the exact downstream-churn this release is supposed to retire, not
|
||||
create a second round of. Shipping it in 0.3.0 pays the cost once,
|
||||
alongside the other breaking changes, while there are zero real
|
||||
consumers. The cost of action now is a static, known quantity; the
|
||||
cost of action later is the same work plus a second round of
|
||||
downstream churn plus the risk of the interpretive path being the
|
||||
one that gets used in the meantime on untrusted bytes.
|
||||
|
||||
**Decision: a `ValidationPlan` ships in 0.3.0 as §3 below.** The
|
||||
shape is a compile-once-walk-many compiled form over the BAST
|
||||
document's value-domain constraints, symmetric to `ReadPlan` (packed
|
||||
read-side) and `OffsetMap` (aligned R/W). The concrete shape,
|
||||
construction, and `validate_bytes` integration are scoped in the
|
||||
0.3.0 implementation plan (a dedicated phase) and detailed in a
|
||||
follow-on design session before implementation; this ADR commits the
|
||||
*decision* (in 0.3.0, not deferred) and the *scope* (a compiled
|
||||
validation form that retires the interpretive `BastDoc` walk in
|
||||
`bast_validation`), so the deferral black hole is closed.
|
||||
|
||||
## Decision
|
||||
|
||||
@@ -79,7 +122,11 @@ read-path fix. Explicitly deferred.
|
||||
Add `#[derive(Hash, Eq)]` (alongside the existing `Debug, Clone, PartialEq`)
|
||||
to `ReadPlan` and `OffsetMap`, plus their public sub-types
|
||||
(`FieldPlan`, `CompositePlan`, `ReadKind`, `DiscriminatorPlan`,
|
||||
`VariantPlan`, `ByteRange`, and the new `LeafMeta` — see §2).
|
||||
`ByteRange`, and the new `LeafMeta` — see §2). `VariantPlan`/
|
||||
`VariantKind` are not in the production `ReadPlan` shape (ADR-011
|
||||
was refined on acceptance to drop them — see ADR-011 status), so
|
||||
they are not derived. `ValidationPlan` (§3) gets `Hash + Eq` + its
|
||||
own `fingerprint()` as part of its public surface.
|
||||
|
||||
**`by_name` representation change.** `ReadPlan.by_name` is currently
|
||||
`HashMap<String, usize>`. `HashMap` iteration order is non-deterministic
|
||||
@@ -224,7 +271,76 @@ This is additive on the *capability* (the map now answers questions it
|
||||
previously couldn't) but breaking on the *signature* (`get`'s return
|
||||
type changes). Rides the 0.3.0 bump.
|
||||
|
||||
### 3. Fingerprinting `OffsetMap` (bundled with §2b)
|
||||
### 3. `ValidationPlan` — compile-once validation form
|
||||
|
||||
`bast_validation` currently walks `BastDoc` interpretively on every
|
||||
`validate_bytes` call to check value-domain constraints (enum value
|
||||
sets, integer ranges, `maxLength` caps, union variant keys). After
|
||||
ADR-011, the *read* half of `validate_bytes` (packed) is plan-fast;
|
||||
the *validation* half is still an interpretive `BastDoc` walk per
|
||||
buffer. On the `alkcall` hub/spoke topology, `validate_bytes` is the
|
||||
gate between "bytes arrived from an untrusted peer" and "act on the
|
||||
decoded frame," so it runs once per incoming buffer and validation is
|
||||
hot in the same sense review #004 measured for the read path.
|
||||
|
||||
**Decision: a `ValidationPlan` is a compiled form over the BAST
|
||||
document's value-domain constraints, built once at `compile` time
|
||||
(symmetric to `ReadPlan`/`OffsetMap`) and walked by
|
||||
`bast_validation`/`validate_bytes` without re-touching `BastDoc`.**
|
||||
|
||||
The shape, construction, and `validate_bytes` integration are scoped
|
||||
in the 0.3.0 implementation plan as a dedicated phase and detailed in
|
||||
a follow-on design session before implementation begins. The
|
||||
properties this ADR commits to (so the plan and any implementing agent
|
||||
have a fixed contract):
|
||||
|
||||
- **Compile-once-walk-many.** `ValidationPlan::compile` walks `BastDoc`
|
||||
once; `validate_bytes` (both modes) walks the `ValidationPlan` per
|
||||
buffer, never `BastDoc`. This is the same pattern as `ReadPlan` and
|
||||
`OffsetMap`; it is the structural reason the per-buffer
|
||||
interpretive cost goes away.
|
||||
- **Value-domain, not byte-position.** The plan carries constraint
|
||||
descriptors (enum allowed-sets, integer range bounds, `maxLength`
|
||||
caps, union variant keys, and any other value-domain checks
|
||||
`bast_validation` performs today), keyed for dispatch against the
|
||||
materialized `Value` tree, not byte offsets. The shape is therefore
|
||||
different from `ReadPlan`/`OffsetMap`; the *pattern* (compiled form,
|
||||
immutable, shared via `Arc`) is the same.
|
||||
- **No new `BastDoc` walk in the hot path.** After this ADR, the only
|
||||
consumers that walk `BastDoc` interpretively are the one-shot
|
||||
`compile` paths (`ReadPlan::compile`, `OffsetMap::compute`,
|
||||
`ValidationPlan::compile`, `LayoutBuilder::new`). The per-buffer
|
||||
paths (`sequential_reader`, `materialize_packed`,
|
||||
`materialize_aligned`, `validate_bytes`) all walk compiled forms.
|
||||
This is the end state ADR-011 pointed at; this ADR closes it.
|
||||
- **Semver.** `ValidationPlan` is a new public type, re-exported from
|
||||
`lib.rs`. `validate_bytes`'s *signature* is unchanged (still
|
||||
`(buffer) -> Result<(), AlkTypeError>`); the change is internal
|
||||
(walks the plan instead of `BastDoc`). If the `ValidationPlan`
|
||||
design surfaces a need to change `validate_bytes`'s signature, that
|
||||
rides the 0.3.0 bump and is recorded in the plan's Semver Contract
|
||||
table when the shape is scoped. `bast_validation`'s public surface
|
||||
(`build_validator`, `validate_value`) is reviewed at shape-scope
|
||||
time; additive changes ride the bump, removals/renames are avoided
|
||||
unless the shape work shows they're necessary.
|
||||
- **Fingerprinting.** `ValidationPlan` is `Hash + Eq` with a
|
||||
`fingerprint()` method, same as `ReadPlan`/`OffsetMap` (§1/§4), so
|
||||
the downstream uses (cross-run cache, `alkcall` handshake,
|
||||
schema-version diagnostics) extend to the validation form without
|
||||
new API. The fingerprint contract generalizes: two validation plans
|
||||
with equal hashes accept/reject identical `(bytes)` identically.
|
||||
|
||||
**What this ADR does *not* decide** (left to the follow-on shape
|
||||
session + plan phase): the concrete `ValidationPlan` struct/enum
|
||||
shape, how `maxLength`/range/enum/union-key constraints are
|
||||
represented, whether `bast_validation`'s `validate_value` is retired
|
||||
or kept as a convenience wrapper over the plan, and whether the
|
||||
`AlkTypeKind`-driven dispatch in `bast_validation` collapses into the
|
||||
plan or stays a thin match over plan-carried descriptors. These are
|
||||
shape questions, not decision questions; the decision (in 0.3.0,
|
||||
compiled form, no per-buffer `BastDoc` walk) is fixed here.
|
||||
|
||||
### 4. Fingerprinting `OffsetMap` (bundled with §2b)
|
||||
|
||||
Since `OffsetMap` is getting new fields (`LeafMeta`) in §2b, its
|
||||
`#[derive(Hash, Eq)]` (from §1) covers the new fields automatically.
|
||||
@@ -237,7 +353,7 @@ aligned reads/writes over identical bytes.
|
||||
### In scope
|
||||
|
||||
- `ReadPlan: Hash + Eq` + `fingerprint()` method (§1).
|
||||
- `OffsetMap: Hash + Eq` + `fingerprint()` method (§1, §3).
|
||||
- `OffsetMap: Hash + Eq` + `fingerprint()` method (§1, §4).
|
||||
- `BastDoc<'a>` → `BastDoc` (owned) refactor, scoped to the consumers
|
||||
that currently hold `doc_value: Value` and re-parse: `LayoutBuilder`,
|
||||
`bast_validation`, `materialize_aligned`, `OffsetMap::compute`
|
||||
@@ -249,17 +365,26 @@ aligned reads/writes over identical bytes.
|
||||
`BastDoc::new` + `lookup_leaf_field` (§2b).
|
||||
- `LeafMeta` new public type (§2b).
|
||||
- `BTreeMap` for `ReadPlan.by_name` (§1).
|
||||
- `ValidationPlan` new public type + `compile` + `Hash + Eq` +
|
||||
`fingerprint()` (§3). `validate_bytes` (both modes) walks the
|
||||
`ValidationPlan` instead of re-walking `BastDoc`. `bast_validation`
|
||||
adopts the plan; the public `validate_value`/`build_validator`
|
||||
surface is reviewed at shape-scope time and rides the bump only if
|
||||
the shape work shows a signature change is necessary.
|
||||
- `ValidationPlan: Hash + Eq` + `fingerprint()` method (§3, §1) — the
|
||||
fingerprint contract extends to the validation form.
|
||||
|
||||
### Out of scope
|
||||
|
||||
- `ValidationPlan` — different shape, not a hot loop, deferred (see
|
||||
"Deferring `ValidationPlan`" above).
|
||||
- Disk-cache or handshake *implementations* — the fingerprint
|
||||
*contract* and method are in scope; the downstream uses (cache
|
||||
format, wire protocol) are the consumers' problem, not this ADR's.
|
||||
- Aligned-mode `validate_bytes` — already uses `OffsetMap` + the
|
||||
BAST-native validator; the `BastDoc` ownership change touches it
|
||||
but no new compiled form is needed.
|
||||
*contract* and method are in scope (§1, §3); the downstream uses
|
||||
(cache format, wire protocol) are the consumers' problem, not this
|
||||
ADR's.
|
||||
- The concrete `ValidationPlan` struct/enum shape and constraint
|
||||
representation — scoped in the 0.3.0 implementation plan's
|
||||
dedicated phase and a follow-on design session (see §3 "What this
|
||||
ADR does *not* decide"). The decision (in 0.3.0, compiled form, no
|
||||
per-buffer `BastDoc` walk) is fixed; the shape is not.
|
||||
- Cross-version fingerprint stability — the fingerprint is stable
|
||||
within a crate version but may change across versions (a new
|
||||
`AlkTypeKind` variant, for example, changes the hash). Cross-version
|
||||
@@ -271,21 +396,32 @@ aligned reads/writes over identical bytes.
|
||||
|
||||
### Positive
|
||||
|
||||
- **One breaking release, not two.** ADR-011's `ReadPlan` + this
|
||||
ADR's `BastDoc`-owned + `OffsetMap` extension ship together. The
|
||||
two in-house downstream consumers (`alktty`, `alkcall`) update once.
|
||||
- **One breaking release, not two (or three).** ADR-011's `ReadPlan`
|
||||
+ this ADR's `BastDoc`-owned + `OffsetMap` extension +
|
||||
`ValidationPlan` all ship together. The two in-house downstream
|
||||
consumers (`alktty`, `alkcall`) update once.
|
||||
- **Closes all deferred M1 sites.** `LayoutBuilder::build`
|
||||
(`layout_builder.rs:190`), `read_field` (`engine.rs:334`),
|
||||
`write_field` (`engine.rs:467`) all stop re-parsing. The packed-side
|
||||
`validate_bytes` (`engine.rs:284`) was already closed by ADR-011;
|
||||
this ADR closes the aligned-side equivalent.
|
||||
- **Retires the interpretive validation walk.** `validate_bytes` on
|
||||
untrusted streams (the `alkcall` common case) stops re-walking
|
||||
`BastDoc` per buffer. This is the latent perf cliff review #005 M3
|
||||
flagged: the read half was plan-fast after ADR-011, the validation
|
||||
half was not. Closing it here — while there are zero real consumers
|
||||
and one breaking bump already paying the downstream-churn cost —
|
||||
avoids a second breaking change to `validate_bytes`/`bast_validation`
|
||||
after 0.3.0.
|
||||
- **Fingerprinting enables downstream uses.** Cross-run plan caching,
|
||||
`alkcall` schema handshake, and schema-version diagnostics all
|
||||
become possible without further API work.
|
||||
become possible without further API work — across `ReadPlan`,
|
||||
`OffsetMap`, and `ValidationPlan`.
|
||||
- **`BastDoc` owned is a net simplification.** One typed-tree type,
|
||||
owned, used by all non-`ReadPlan` consumers. No more
|
||||
owned, used by all `*::compile` paths. No more
|
||||
lifetime-entanglement workarounds. The "re-parse on demand" framing
|
||||
from ADR-007 is fully retired across both read and write paths.
|
||||
from ADR-007 is fully retired across read, write, and validation
|
||||
paths.
|
||||
- **`OffsetMap` extension is additive capability.** The map now
|
||||
answers `kind`/`encoding`/`endian` questions it previously couldn't,
|
||||
enabling future aligned-side tools without re-walking `BastDoc`.
|
||||
@@ -295,7 +431,8 @@ aligned reads/writes over identical bytes.
|
||||
- **Breaking public-API changes (0.2.0 → 0.3.0).** `BastDoc<'a>` →
|
||||
`BastDoc` (owned) changes every `Bast*` signature that took `&'a`.
|
||||
`OffsetMap::get` return type changes. `LeafMeta` is new public.
|
||||
`ReadPlan` is new public (from ADR-011). All ride the bump.
|
||||
`ReadPlan` is new public (from ADR-011). `ValidationPlan` is new
|
||||
public (§3). All ride the bump.
|
||||
- **`BastDoc` ownership refactor is broad.** Touches `bast.rs` (every
|
||||
typed node: `&'a str` → `String`/`Arc<str>`, `&'a Value` →
|
||||
`Value`/`Arc<Value>`) and every consumer (`layout_builder`,
|
||||
@@ -305,6 +442,13 @@ aligned reads/writes over identical bytes.
|
||||
anyway. The refactor is mechanical (lifetime removal, not logic
|
||||
rewrites); the POC on `readplan-poc` confirmed the read path is
|
||||
unaffected.
|
||||
- **`ValidationPlan` shape work is not yet scoped.** §3 fixes the
|
||||
decision (in 0.3.0, compiled form) but defers the concrete shape to
|
||||
a follow-on design session + a dedicated plan phase. This is a
|
||||
tracked, owned deferral with a concrete reactivation trigger (the
|
||||
shape session before phase 7), not a black-hole hedge: the
|
||||
implementation phase is committed in the plan, so the work cannot
|
||||
slip past 0.3.0 without reopening this ADR.
|
||||
- **Fingerprint cross-version stability is not guaranteed.** A future
|
||||
`AlkTypeKind` variant changes the hash. Documented as a within-
|
||||
version contract. Consumers that need cross-version stability
|
||||
@@ -318,7 +462,9 @@ aligned reads/writes over identical bytes.
|
||||
`PackedLayout`; the aligned R/W compiled form is `OffsetMap`. The
|
||||
M1 fixes are "cache the parse" (§2a) and "extend the compiled form
|
||||
with leaf metadata" (§2b), not "add a third compiled form."
|
||||
- **Not a `ValidationPlan`.** Deferred — different shape, not hot.
|
||||
- **Not a `ValidationPlan` deferral.** `ValidationPlan` is in scope
|
||||
(§3). The shape is to be scoped in a follow-on session; the
|
||||
decision to ship in 0.3.0 is fixed.
|
||||
- **Not cross-version fingerprint stability.** Within-version only.
|
||||
- **Not a disk-cache or wire-protocol spec.** The fingerprint contract
|
||||
and method are in scope; the downstream uses are the consumers'
|
||||
@@ -332,27 +478,34 @@ for the step-by-step execution order. The high-level grouping:
|
||||
1. **`ReadPlan` (ADR-011 steps 1–5)** — the packed read-path fix. Closes
|
||||
H1 + packed-side M1 + L1 + L2.
|
||||
2. **`BastDoc` owned (§2a)** — the typed-tree ownership refactor. Prerequisite
|
||||
for the `LayoutBuilder` M1 fix.
|
||||
for the `LayoutBuilder` M1 fix and for `ValidationPlan::compile`.
|
||||
3. **`LayoutBuilder` M1 fix (§2a)** — cache the owned `BastDoc` at `new()`.
|
||||
4. **`OffsetMap` extension (§2b)** — carry `LeafMeta`; close the
|
||||
aligned-side `read_field`/`write_field` M1.
|
||||
5. **Fingerprinting (§1, §3)** — `Hash + Eq` + `fingerprint()` on
|
||||
`ReadPlan` and `OffsetMap`. Rides on top of the above.
|
||||
6. **Public API bump (0.2.0 → 0.3.0)** — `lib.rs` re-exports, version
|
||||
5. **`ValidationPlan` (§3)** — compiled validation form; `validate_bytes`
|
||||
walks the plan instead of `BastDoc`. Requires the owned `BastDoc`
|
||||
from step 2 for `ValidationPlan::compile`.
|
||||
6. **Fingerprinting (§1, §4)** — `Hash + Eq` + `fingerprint()` on
|
||||
`ReadPlan`, `OffsetMap`, and `ValidationPlan`. Rides on top of the
|
||||
above.
|
||||
7. **Public API bump (0.2.0 → 0.3.0)** — `lib.rs` re-exports, version
|
||||
bump, update `alktty`/`alkcall`.
|
||||
7. **Verification block** — full suite + wasm + bench.
|
||||
8. **Verification block** — full suite + wasm + bench.
|
||||
|
||||
## References
|
||||
|
||||
- [ADR-011](011-compiled-read-plan-for-packed-mode.md) — the
|
||||
`ReadPlan` (packed read-side compiled form). This ADR extends the
|
||||
0.3.0 release with fingerprinting and the deferred M1 fixes.
|
||||
0.3.0 release with fingerprinting, the deferred M1 fixes, and the
|
||||
`ValidationPlan`.
|
||||
- [Review #004](../../reviews/004-performance-review.md) — the
|
||||
performance finding (H1, M1, L1, L2). ADR-011 closed H1 + packed
|
||||
M1 + L1 + L2; this ADR closes the aligned-side M1.
|
||||
- [Review #005](../../reviews/005-plan-review-030.md) — the 0.3.0 plan
|
||||
review whose M3 finding reversed the `ValidationPlan` deferral.
|
||||
- [ADR-007](007-packed-mode-read-factory.md) — the "re-parse on
|
||||
demand" framing, retired across both read and write paths by
|
||||
ADR-011 + this ADR.
|
||||
demand" framing, retired across read, write, and validation paths
|
||||
by ADR-011 + this ADR.
|
||||
- [ADR-002](002-two-layout-modes-packed-vs-aligned.md) — the two
|
||||
layout modes; `OffsetMap` is the aligned R/W compiled form extended
|
||||
here with `LeafMeta`.
|
||||
|
||||
@@ -1,16 +1,18 @@
|
||||
---
|
||||
status: in-progress
|
||||
created: 2026-08-19
|
||||
last_updated: 2026-08-19
|
||||
last_updated: 2026-08-20
|
||||
adr: ADR-011, ADR-012
|
||||
---
|
||||
|
||||
# 0.3.0 — Compiled Forms: ReadPlan, Owned BastDoc, OffsetMap LeafMeta, Fingerprinting
|
||||
# 0.3.0 — Compiled Forms: ReadPlan, Owned BastDoc, OffsetMap LeafMeta, ValidationPlan, Fingerprinting
|
||||
|
||||
This is the execution plan for the 0.3.0 release: the compiled-form
|
||||
rollup that closes review #004's 400x read-path gap (ADR-011) *and*
|
||||
the deferred M1 sites (ADR-012) *and* adds plan fingerprinting
|
||||
(ADR-012) in one breaking bump. It is the **entry point** an
|
||||
the deferred M1 sites (ADR-012) *and* retires the interpretive
|
||||
validation walk via a `ValidationPlan` (ADR-012 §3, reversing the
|
||||
original deferral per review #005 M3) *and* adds plan fingerprinting
|
||||
(ADR-012 §1/§4) in one breaking bump. It is the **entry point** an
|
||||
implementing agent reads first.
|
||||
|
||||
Companion documents:
|
||||
@@ -18,8 +20,8 @@ Companion documents:
|
||||
- [ADR-011](../architecture/decisions/011-compiled-read-plan-for-packed-mode.md)
|
||||
— the `ReadPlan` decision (packed read-side compiled form).
|
||||
- [ADR-012](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md)
|
||||
— fingerprinting + owned `BastDoc` + `OffsetMap` `LeafMeta` (this
|
||||
release's two other pieces).
|
||||
— fingerprinting + owned `BastDoc` + `OffsetMap` `LeafMeta` +
|
||||
`ValidationPlan` (this release's other three pieces).
|
||||
- [Review #004](../reviews/004-performance-review.md) — the
|
||||
performance finding being closed.
|
||||
- [POC findings](../../poc/readplan/FINDINGS.md) (branch `readplan-poc`)
|
||||
@@ -33,7 +35,7 @@ phases in order; within a phase, steps are ordered by dependency.
|
||||
|
||||
## Phases vs sessions
|
||||
|
||||
This plan is deliberately larger than one session's work. The seven
|
||||
This plan is deliberately larger than one session's work. The eight
|
||||
phases are the session boundaries — each phase is a coherent unit
|
||||
that leaves the tree building and tests green, so any one session
|
||||
can pick up a phase without needing context from the previous one.
|
||||
@@ -61,10 +63,13 @@ re-exports.
|
||||
| `ByteRange` | **Unchanged** | Still `Copy + PartialEq + Eq + Hash`. |
|
||||
| `LeafMeta` | **New public type** | `{ kind: AlkTypeKind, encoding: VariableEncoding, endian: Endian }`, re-exported from `lib.rs`. `Copy + PartialEq + Eq + Hash`. |
|
||||
| `ReadPlan` | **New public type** | From ADR-011. Re-exported from `lib.rs`. `Debug + Clone + PartialEq + Eq + Hash`. |
|
||||
| `SequentialReader` | **Breaking (constructor)** | `SequentialReader::new(&Value, &str)` → `SequentialReader::new(Arc<ReadPlan>)`. Public methods (`read_next`/`read_field`/`reset`/`position`/`endian`/`schema`) unchanged. `schema()` returns `&Value` retained on the plan. |
|
||||
| `SequentialReader` | **Breaking (constructor + return type)** | `SequentialReader::new(&Value, &str) -> Result<Self, AlkTypeError>` → `SequentialReader::new(Arc<ReadPlan>) -> Self` (infallible — just stores the `Arc`; the `BastDoc` parse moved to `ReadPlan::compile`). Public methods (`read_next`/`read_field`/`reset`/`position`/`endian`/`schema`) unchanged. `schema()` returns `&Value` retained on the plan (see phase 2 — the plan stores `Arc<Value>`, not `&Value`, to avoid the self-referential struct ADR-011 rejects). `engine.rs`'s `.ok()` on the old `Result` correspondingly goes away. |
|
||||
| `FieldValue` | **Unchanged** | — |
|
||||
| `materialize_packed` | **Breaking (signature)** | `materialize_packed(&BastDoc<'_>, &[u8])` → `materialize_packed(&ReadPlan, &[u8])`. |
|
||||
| `materialize_aligned` | **Breaking (signature)** | `materialize_aligned(&BastDoc<'_>, &[u8], &OffsetMap)` → `materialize_aligned(&BastDoc, &[u8], &OffsetMap)` (owned `BastDoc`, no lifetime). |
|
||||
| `ValidationPlan` | **New public type** | From ADR-012 §3. Re-exported from `lib.rs`. `Debug + Clone + PartialEq + Eq + Hash`. `compile(&BastDoc, &str) -> Result<Self, AlkTypeError>`, `fingerprint() -> u64`. Shape scoped in phase 7 (and a preceding design session); the contract is fixed in ADR-012 §3. |
|
||||
| `AlkTypeEngine::validate_bytes` | **Unchanged (signature)** | Still `(buffer) -> Result<(), AlkTypeError>`. Internally walks `&ValidationPlan` (both modes) instead of re-walking `BastDoc` for value-domain checks. The `materialize` half is unchanged from the ADR-011/§2b work (packed: `materialize_packed(&self.plan, ...)`; aligned: `materialize_aligned(&self.doc, ..., &self.offset_map)`). |
|
||||
| `bast_validation` (`build_validator`, `validate_value`) | **Additive (reviewed at phase 7)** | `validate_value` is expected to become a thin wrapper over `ValidationPlan` (or be retired if the shape work shows it's redundant). Additive changes ride the bump; renames/removals are avoided unless phase 7's shape work shows they're necessary. The BAST meta-schema validator (`build_validator`/`BAST_META_SCHEMA`) used at `compile` time is unaffected. |
|
||||
| `LayoutBuilder`, `PackedLayout`, `FieldPosition` | **Unchanged (signature)** | `LayoutBuilder::new`/`build` signatures unchanged. Internally caches the owned `BastDoc` instead of re-parsing. |
|
||||
| `AlkTypeKind`, `Endian`, `VariableEncoding` | **Unchanged** | — |
|
||||
| `AlkTypeError` | **Unchanged** | — |
|
||||
@@ -74,12 +79,13 @@ re-exports.
|
||||
|
||||
**Net breaking surface:** `BastDoc` and all `Bast*` types (lifetime
|
||||
removal), `OffsetMap::get` (return type), `SequentialReader::new`
|
||||
(constructor), `materialize_packed`/`materialize_aligned` (signatures).
|
||||
**Net additive:** `ReadPlan`, `LeafMeta`, `fingerprint()` methods,
|
||||
`Hash + Eq` derives on `ReadPlan`/`OffsetMap`. **Net unchanged:** the
|
||||
builder, `AlkTypeEngine` accessors (signatures), `FieldValue`,
|
||||
`AlkTypeKind`, `AlkTypeError`, `data_access`, the `Schema`/`Definitions`
|
||||
builders.
|
||||
(constructor + `Result` drop), `materialize_packed`/`materialize_aligned`
|
||||
(signatures). **Net additive:** `ReadPlan`, `LeafMeta`, `ValidationPlan`,
|
||||
`fingerprint()` methods, `Hash + Eq` derives on `ReadPlan`/`OffsetMap`/
|
||||
`ValidationPlan`. **Net unchanged:** the builder, `AlkTypeEngine`
|
||||
accessors (signatures), `FieldValue`, `AlkTypeKind`, `AlkTypeError`,
|
||||
`data_access`, the `Schema`/`Definitions` builders, `validate_bytes`
|
||||
(signature).
|
||||
|
||||
### Decisions deferred to their implementation phases
|
||||
|
||||
@@ -102,7 +108,7 @@ builders.
|
||||
returns `element_stride: 0` for fixed-size struct arrays
|
||||
(`sequential_reader.rs:567`). The `ReadPlan` correctly computes the
|
||||
stride. Decision: preserve the existing `0` behavior in `ReadPlan` for
|
||||
back-comat with `SequentialReader`'s consumer contract, *or* fix it
|
||||
back-compat with `SequentialReader`'s consumer contract, *or* fix it
|
||||
and document the behavioral change. Lean: fix it — the `0` is a latent
|
||||
bug, the stride is behaviorally observable, and we're bumping. The
|
||||
plan step calls this out explicitly. Decided in phase 2.
|
||||
@@ -110,23 +116,31 @@ builders.
|
||||
## Phase 1 — `ReadPlan` type + `compile` (ADR-011 step 1)
|
||||
|
||||
**Goal:** Add the `ReadPlan`/`FieldPlan`/`CompositePlan`/`ReadKind`/
|
||||
`DiscriminatorPlan`/`VariantPlan` types and `ReadPlan::compile(&Value,
|
||||
&str) -> Result<Self, AlkTypeError>`. Pure addition; no existing code
|
||||
touched. This is the foundation — later phases wire it into the engine
|
||||
and reader.
|
||||
`DiscriminatorPlan` types and `ReadPlan::compile(&Value, &str) ->
|
||||
Result<Self, AlkTypeError>`. Pure addition; no existing code touched.
|
||||
This is the foundation — later phases wire it into the engine and
|
||||
reader. (The POC's `VariantPlan`/`VariantKind` types are **dropped**
|
||||
in the production shape — see the union-shape note below.)
|
||||
|
||||
**ADR reference:** [ADR-011 §The `ReadPlan` shape](../architecture/decisions/011-compiled-read-plan-for-packed-mode.md#the-readplan-shape),
|
||||
[ADR-011 §Construction](../architecture/decisions/011-compiled-read-plan-for-packed-mode.md#construction).
|
||||
The `CompositePlan::Union` shape in ADR-011 was refined (vs the
|
||||
accepted-at-POC shape) to carry the field-disc union's shared fields
|
||||
and to drop `VariantPlan`/`VariantKind`; this phase implements the
|
||||
refined shape.
|
||||
|
||||
**POC reference:** `poc/readplan/src/lib.rs` (branch `readplan-poc`) is
|
||||
the reference scaffold. The production version lives in `src/` and
|
||||
adds doc comments, clippy cleanliness, and the field-name-discriminator
|
||||
union read shape the POC stubbed (see POC Finding 1).
|
||||
**POC reference:** `poc/readplan/src/lib.rs` (branch `readplan-poc`)
|
||||
is the reference scaffold. The production version lives in `src/` and
|
||||
adds doc comments, clippy cleanliness, the field-name-discriminator
|
||||
union read shape the POC stubbed (POC Finding 1), and nested-union
|
||||
variant support the POC rejected but 0.2.0 accepts (POC "What this
|
||||
POC does not cover" → nested unions). Both are resolved by the
|
||||
refined `CompositePlan::Union` shape — see below.
|
||||
|
||||
**Files:** New `src/read_plan.rs`. Update `src/lib.rs` to add
|
||||
`pub mod read_plan;` and re-export `ReadPlan` (and the plan sub-types
|
||||
that are part of the public surface — `ReadKind`, `CompositePlan`, etc.
|
||||
if the ADR's public-API section calls for them; the ADR lists
|
||||
that are part of the public surface — `ReadKind`, `CompositePlan`,
|
||||
etc. if the ADR's public-API section calls for them; the ADR lists
|
||||
`ReadPlan` as the public type, sub-types can stay `pub` in-module if
|
||||
consumers don't need to name them).
|
||||
|
||||
@@ -140,16 +154,32 @@ consumers don't need to name them).
|
||||
- `by_name: BTreeMap<String, usize>` (not `HashMap` — ADR-012 §1
|
||||
requires `Hash` on `ReadPlan`, and `HashMap` blocks derive). The
|
||||
POC used `HashMap`; swap to `BTreeMap`.
|
||||
- **Field-name-discriminator union read shape (POC Finding 1):** the
|
||||
POC stubbed `plan_read_union`'s `Field` arm. The production
|
||||
`compile_union` must produce a plan that the read loop can walk for
|
||||
the field-disc case. The shape: `CompositePlan::Union` carries the
|
||||
union's declared `fields` as a sub-`ReadPlan` (the discriminator field
|
||||
+ any shared fields), and the variant plans are laid out *after* the
|
||||
shared fields. The read loop reads the discriminator field from the
|
||||
sub-plan, looks up the variant, and walks the variant plan. This is
|
||||
the one piece the POC deliberately left as a TODO — the production
|
||||
version must implement it. See `poc/readplan/FINDINGS.md` Finding 1.
|
||||
- **Union shape (ADR-011 refined — resolves POC Finding 1 and the
|
||||
nested-union gap):** `CompositePlan::Union { disc, shared,
|
||||
variants: Vec<(String, CompositePlan)> }`.
|
||||
- `shared: Option<Box<ReadPlan>>` — the union's declared `fields`
|
||||
(the discriminator field + any shared fields) for the
|
||||
field-name-discriminator case. `DiscriminatorPlan::Field.field_index`
|
||||
indexes into `shared`. The read loop walks `shared` first, then
|
||||
looks up the selected variant and walks its `CompositePlan`
|
||||
starting after the shared fields. The byte-offset-discriminator
|
||||
case sets `shared: None` (no shared fields; the variant starts
|
||||
immediately after the discriminator size). This replaces the
|
||||
POC's stubbed `plan_read_union` `Field` arm.
|
||||
- `variants: Vec<(String, CompositePlan)>` — **not** the POC's
|
||||
`Vec<(String, VariantPlan)>`. Dropping `VariantPlan`/`VariantKind`
|
||||
means a variant's body is just a `CompositePlan`, so **nested
|
||||
unions** (a variant that is itself a union, which the 0.2.0
|
||||
reader supports via `resolve_and_walk_variant`'s `Union` arm at
|
||||
`sequential_reader.rs:800`) work by ordinary `CompositePlan`
|
||||
recursion — a variant can be `CompositePlan::Union { ... }`. No
|
||||
separate `VariantKind::Union` arm, no behavioral drop vs 0.2.0,
|
||||
no Semver Contract entry for a capability regression. The POC's
|
||||
`VariantPlan { kind, plan }` wrapper is not carried forward.
|
||||
- `compile_union` must reject a variant that is neither a struct
|
||||
nor a union with `AlkTypeError::Schema` (mirroring
|
||||
`resolve_and_walk_variant`'s `other => Err(...)` arm), so the
|
||||
eager-resolution path keeps the untrusted-input discipline.
|
||||
- Do *not* wire `ReadPlan` into `SequentialReader` or `materialize` yet
|
||||
— that's phase 2. Phase 1 is the type + `compile` only, unit-tested
|
||||
against the same BAST fixtures `bast.rs` uses (the existing `bast.rs`
|
||||
@@ -157,10 +187,18 @@ consumers don't need to name them).
|
||||
|
||||
**Verification:** `cargo test --release` (new unit tests for `compile`
|
||||
covering every `BastType` arm — port the `cov_*` tests from
|
||||
`poc/readplan/tests/coverage.rs`). `cargo clippy --all-targets -- -D
|
||||
`poc/readplan/tests/coverage.rs`, **plus** a nested-union-variant
|
||||
test asserting `compile_union` produces `CompositePlan::Union` whose
|
||||
variant body is itself `CompositePlan::Union`, restoring the 0.2.0
|
||||
capability the POC rejected). `cargo clippy --all-targets -- -D
|
||||
warnings`. `cargo doc --no-deps` (new public type). `cargo build
|
||||
--target wasm32-unknown-unknown --release` (`read_plan.rs` is
|
||||
wasm-relevant).
|
||||
wasm-relevant). **Add a `fn read_plan_is_send_sync()` assertion
|
||||
test** (a `const _: fn() = || { fn assert_send_sync<T: Send + Sync>()
|
||||
{}; assert_send_sync::<ReadPlan>(); };` static-bound assertion, as
|
||||
ADR-011 §"Engine integration" requires) to lock in `ReadPlan: Send +
|
||||
Sync` so a future change can't break it silently — mirror the POC's
|
||||
`readplan_is_send_sync` test.
|
||||
|
||||
---
|
||||
|
||||
@@ -182,20 +220,46 @@ packed mode, `sequential_reader()` hands out `Arc::clone`, packed
|
||||
`validate_bytes` calls `materialize_packed(&self.plan, ...)`).
|
||||
|
||||
**Implementation notes:**
|
||||
- `SequentialReader::new(&Value, &str)` →
|
||||
`SequentialReader::new(Arc<ReadPlan>)`. The reader stores
|
||||
`plan: Arc<ReadPlan>`, `field_index: usize`, `position: usize`.
|
||||
`endian()` reads `self.plan.endian()`. `schema()` returns a `&Value`
|
||||
retained on the plan (the plan stores the `&Value` it was compiled
|
||||
from — see ADR-011 §Engine integration; the `&Value` outlives the
|
||||
plan because the engine owns both).
|
||||
- `SequentialReader::new(&Value, &str) -> Result<Self, AlkTypeError>` →
|
||||
`SequentialReader::new(Arc<ReadPlan>) -> Self` (infallible — the
|
||||
fallible `BastDoc` parse moved to `ReadPlan::compile` in phase 1;
|
||||
`new` just stores the `Arc`). The reader stores `plan: Arc<ReadPlan>`,
|
||||
`field_index: usize`, `position: usize`. `endian()` reads
|
||||
`self.plan.endian()`.
|
||||
- **`schema()` ownership (resolves review #005 H2):** `schema()`
|
||||
returns `&Value` retained on the plan, but the plan stores an
|
||||
**`Arc<Value>`**, not a `&Value`. ADR-011 §"Root cause" rejects the
|
||||
self-referential struct pattern (a `ReadPlan` storing `&Value`
|
||||
borrowing from the engine's `bast_doc: Value` would make the engine
|
||||
self-referential — exactly the construction ADR-007 worked around
|
||||
and ADR-011's `Arc<ReadPlan>` was meant to retire). The fix:
|
||||
`ReadPlan` carries `schema: Arc<Value>`; `ReadPlan::compile` clones
|
||||
the input `&Value` into `Arc<Value>` once; `schema()` returns
|
||||
`&self.schema`. The engine stores `bast_doc: Arc<Value>` internally
|
||||
(one allocation, shared via refcount between the engine and all
|
||||
plans it builds) — this is an internal change, not a public
|
||||
signature change (`compile` still takes `&Value`). `Arc<Value>`
|
||||
implements `Hash + Eq` (`serde_json::Value: Hash + Eq` as of the
|
||||
pinned `serde_json` with `preserve_order`; `Map::hash` sorts keys
|
||||
for determinism), so phase 6's `#[derive(Hash)]` on `ReadPlan` is
|
||||
not blocked by carrying the schema. **Note:** if a future
|
||||
`serde_json` version regresses `Value: Hash`, phase 6 would need
|
||||
`ReadPlan`'s hash to exclude the `schema` field; that is a phase-6
|
||||
concern, not a phase-2 blocker.
|
||||
- `read_field_at`/`read_field_value`/`read_typeref_value`/
|
||||
`walk_struct_size`/`read_union_value`/`read_array_value`/
|
||||
`read_record_value` are rewritten to take plan nodes
|
||||
(`&FieldPlan`/`&CompositePlan`/`&ReadKind`) instead of
|
||||
`&BastField`/`&BastType`/`&BastDoc`. Port `plan_read_field_at`/
|
||||
`plan_walk_struct_size`/etc. from `poc/readplan/src/lib.rs` — they're
|
||||
the reference implementations.
|
||||
the reference implementations. The `read_union_value` rewrite
|
||||
handles both discriminator kinds via the refined `CompositePlan::Union`
|
||||
shape from phase 1: byte-disc reads the discriminator at
|
||||
`disc.offset` then walks the variant (no `shared`); field-disc walks
|
||||
`shared` first, reads the discriminator field at
|
||||
`disc.field_index` within `shared`, looks up the variant, and walks
|
||||
it starting after the shared fields. Nested unions (variant body is
|
||||
itself `CompositePlan::Union`) recurse naturally — no special arm.
|
||||
- **Struct-array stride (deferred decision 4):** the POC computes the
|
||||
true fixed-struct stride; the existing reader returns `0`. The
|
||||
production `ReadPlan::compile_array` should compute the true stride
|
||||
@@ -204,21 +268,47 @@ packed mode, `sequential_reader()` hands out `Arc::clone`, packed
|
||||
now the true stride for fixed-size struct elements, not `0`. This is
|
||||
a breaking behavioral change; rides the bump. Update the
|
||||
`eq_array_ref_element`-style test to assert the new stride.
|
||||
- `materialize_packed(&BastDoc<'_>, &[u8])` →
|
||||
`materialize_packed(&ReadPlan, &[u8])`. The materializer walks the
|
||||
plan instead of `BastDoc`. The `dummy_field_for`/`ty_source` helpers
|
||||
in `materialize.rs` are removed (the plan carries everything).
|
||||
- **`materialize_packed` rewrite + packed/aligned split (resolves
|
||||
review #005 L1 and L2):** `materialize_packed(&BastDoc<'_>, &[u8])`
|
||||
→ `materialize_packed(&ReadPlan, &[u8])`. The materializer walks the
|
||||
plan instead of `BastDoc`. **Scoped removal of helpers:** only the
|
||||
*packed-side* call sites of `dummy_field_for`/`ty_source`
|
||||
(`materialize.rs:249, 316, 351, 391` — the packed
|
||||
`materialize_*_packed` paths) go away when packed-materialize moves
|
||||
to the plan. The helpers themselves **stay**, because
|
||||
`materialize_leaf_at` (`materialize.rs:631`, which calls
|
||||
`dummy_field_for`) is on the **aligned** path — it's called by
|
||||
`materialize_struct_aligned` (`:475`), `materialize_array_aligned`
|
||||
(`:544`), `materialize_variable_aligned` (`:613`). Aligned
|
||||
`materialize` keeps walking `BastDoc` through 0.3.0 (see the Scope
|
||||
Boundary note in phase 5), so `dummy_field_for`/`ty_source` must
|
||||
stay. **`materialize_typeref_packed` split:** this function is
|
||||
currently shared by both packed and aligned paths (aligned's
|
||||
`materialize_leaf_at` calls it to read leaves, and aligned's record
|
||||
path at `:498-506` calls it directly). After phase 2,
|
||||
packed-materialize gets a new plan-walking function;
|
||||
`materialize_typeref_packed` stays for aligned's
|
||||
`materialize_leaf_at` and the aligned record path (renamed or not —
|
||||
implementer's choice; the function is private). This is two
|
||||
mode-specific paths — the existing design — not a "parallel walker"
|
||||
in the maintenance-tax sense ADR-011 §"Negative" cautions against
|
||||
(that caution is about packed read-side `SequentialReader` +
|
||||
`materialize_packed` sharing one plan, which this preserves).
|
||||
- `AlkTypeEngine::compile` (packed branch): build `Arc<ReadPlan>` via
|
||||
`ReadPlan::compile(bast_doc, root_name)`, store it in `Layout::Packed`.
|
||||
`sequential_reader()` returns
|
||||
`Some(SequentialReader::new(Arc::clone(&self.plan)))`.
|
||||
`validate_bytes` (packed) calls
|
||||
`materialize_packed(&self.plan, buffer)` then `bast_validation` on
|
||||
the result (validation still uses `BastDoc` until/unless a future
|
||||
`ValidationPlan` ADR; for 0.3.0 `validate_bytes` reconstructs a
|
||||
`BastDoc` for the validator only — the *read* path uses the plan,
|
||||
the *validation* path uses `BastDoc`. This is acceptable: validation
|
||||
is not the hot loop H1 traces).
|
||||
`materialize_packed(&self.plan, buffer)` then runs validation on the
|
||||
materialized `Value`. **Validation path through phase 2:** until
|
||||
phase 7 (ValidationPlan), `validate_bytes` reconstructs a `BastDoc`
|
||||
for the validator only — the *read* path uses the plan, the
|
||||
*validation* path uses `BastDoc`. This is a temporary bridge: phase 7
|
||||
replaces it with a `ValidationPlan` walk (ADR-012 §3), retiring the
|
||||
per-call `BastDoc` reconstruction. The bridge is acceptable for
|
||||
phases 2–6 because the ValidationPlan work is committed in this
|
||||
release (not deferred), so the bridge has a known removal point in
|
||||
phase 7.
|
||||
|
||||
**Verification:** `cargo test --release` — the existing
|
||||
`sequential_reader.rs` and `materialize.rs` tests drive `read_next`/
|
||||
@@ -354,9 +444,22 @@ Closes the last two M1 sites (`engine.rs:334,467`).
|
||||
(re-export `LeafMeta`).
|
||||
|
||||
**Implementation notes:**
|
||||
- **`Hash` on `Endian`/`VariableEncoding` (resolves review #005 M2 —
|
||||
do this first, it's a prerequisite):** `src/schema.rs:205` (`Endian`)
|
||||
and `:212` (`VariableEncoding`) currently derive only `Debug, Clone,
|
||||
Copy, PartialEq, Eq` — no `Hash`. `LeafMeta` (below) requires all
|
||||
its fields to be `Hash` for `#[derive(Hash)]`, and phase 6's
|
||||
`#[derive(Hash)]` on `ReadPlan`/`OffsetMap` requires `FieldPlan`'s
|
||||
`endian: Endian` + `encoding: VariableEncoding` to be `Hash`. Add
|
||||
`Hash` to both derives in `src/schema.rs`. Both are fieldless enums
|
||||
already at `Eq + PartialEq`, so this is additive and semver-safe —
|
||||
no behavioral change. Trivial, but it's an unstated prerequisite
|
||||
the original plan omitted.
|
||||
- New public type `LeafMeta { kind: AlkTypeKind, encoding:
|
||||
VariableEncoding, endian: Endian }`. `Copy + PartialEq + Eq + Hash`
|
||||
(all fields are `Copy + Hash`).
|
||||
(all fields are `Copy + Hash` once the sub-step above is done —
|
||||
`AlkTypeKind` already derives `Hash`; `Endian`/`VariableEncoding`
|
||||
get it from the sub-step above).
|
||||
- `OffsetMap` storage: `fields: Vec<(String, ByteRange, LeafMeta)>`
|
||||
(was `Vec<(String, ByteRange)>`). The `compute` walk already resolves
|
||||
each leaf's type; add the `LeafMeta` extraction at the point where
|
||||
@@ -383,6 +486,23 @@ Closes the last two M1 sites (`engine.rs:334,467`).
|
||||
leaves; they're walked recursively). The `BastDoc` argument to
|
||||
`materialize_aligned` is now owned (phase 3) — no signature change
|
||||
beyond the lifetime drop.
|
||||
- **Scope Boundary — aligned `materialize`'s `BastDoc` structure walk
|
||||
(resolves review #005 L3):** `materialize_struct_aligned`
|
||||
(`materialize.rs:451-521`) walks `BastDoc` to traverse
|
||||
struct/array/record *structure*, using `OffsetMap` only for leaf
|
||||
byte positions. This is the **permanent design for 0.3.0**, not a
|
||||
deferral: after phase 3 the walk is over owned data (no re-parse,
|
||||
not O(N²)), and aligned `validate_bytes` is one structure walk per
|
||||
call (not per-field), so there is no perf driver analogous to review
|
||||
#004's packed per-chunk gap. ADR-011 §"Out of scope" is half-true
|
||||
here (aligned materialize takes `&OffsetMap` *and* `&BastDoc`) —
|
||||
this note owns the decision: aligned materialize keeps walking owned
|
||||
`BastDoc` for structure through 0.3.0. An `AlignedPlan` that
|
||||
compiles the structure walk is **not** in scope; if a future bench
|
||||
shows an aligned-mode hot loop, it gets its own ADR (tracked as an
|
||||
open question, not a silent gap). Phase 7's `ValidationPlan` does
|
||||
not change this — validation is value-domain, orthogonal to the
|
||||
aligned structure walk.
|
||||
|
||||
**Verification:** `cargo test --release` — existing `offset_map.rs`
|
||||
and `engine.rs` `read_field`/`write_field` tests pass (they go through
|
||||
@@ -392,14 +512,16 @@ wasm32-unknown-unknown --release`.
|
||||
|
||||
---
|
||||
|
||||
## Phase 6 — Fingerprinting (ADR-012 §1, §3)
|
||||
## Phase 6 — Fingerprinting `ReadPlan`/`OffsetMap` (ADR-012 §1, §4)
|
||||
|
||||
**Goal:** Add `Hash + Eq` derives + `fingerprint() -> u64` to `ReadPlan`
|
||||
and `OffsetMap`. Enables cross-run caching, `alkcall` schema handshake,
|
||||
schema-version diagnostics.
|
||||
schema-version diagnostics. (`ValidationPlan` gets the same treatment
|
||||
in phase 7, where it's built — it carries its own `Hash + Eq` +
|
||||
`fingerprint()` as part of its public surface.)
|
||||
|
||||
**ADR reference:** [ADR-012 §1](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md#1-fingerprinting--readplan-hash--eq-offsetmap-hash--eq),
|
||||
[ADR-012 §3](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md#3-fingerprinting-offsetmap-bundled-with-2b).
|
||||
[ADR-012 §4](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md#4-fingerprinting-offsetmap-bundled-with-2b).
|
||||
|
||||
**Files:** `src/read_plan.rs` (derives + `fingerprint`), `src/offset_map.rs`
|
||||
(derives + `fingerprint`), `src/lib.rs` (no new re-exports — `Hash`/`Eq`
|
||||
@@ -409,9 +531,18 @@ are trait derives, `fingerprint` is an inherent method).
|
||||
- `ReadPlan` already uses `BTreeMap` for `by_name` (phase 1), so
|
||||
`#[derive(Hash, Eq)]` works. Add it alongside the existing
|
||||
`Debug, Clone, PartialEq`. Same for `FieldPlan`, `CompositePlan`,
|
||||
`ReadKind`, `DiscriminatorPlan`, `VariantPlan`, `VariantKind`.
|
||||
`ReadKind`, `DiscriminatorPlan`. (The POC's `VariantPlan`/`VariantKind`
|
||||
are not in the production shape — phase 1 dropped them — so they are
|
||||
not derived here.) `ReadPlan` also carries `schema: Arc<Value>` from
|
||||
phase 2; `Arc<Value>: Hash + Eq` because `serde_json::Value: Hash +
|
||||
Eq` (with `preserve_order`, `Map::hash` sorts keys deterministically),
|
||||
so the `schema` field does not block the derive. If a future
|
||||
`serde_json` version regresses `Value: Hash`, exclude `schema` from
|
||||
the derived `Hash` via a manual `impl Hash for ReadPlan` that hashes
|
||||
every field except `schema` — phase-6 concern, not a blocker.
|
||||
- `OffsetMap` already carries `LeafMeta` (phase 5), and `LeafMeta` is
|
||||
`Copy + Hash + Eq`. Add `#[derive(Hash, Eq)]` to `OffsetMap`,
|
||||
`Copy + Hash + Eq` (phase 5 added `Hash` to `Endian`/`VariableEncoding`).
|
||||
Add `#[derive(Hash, Eq)]` to `OffsetMap`,
|
||||
`OffsetEntry`, `ByteRange` (already `Eq + Hash`), `LeafMeta`.
|
||||
- **Fingerprint hasher (deferred decision 3):** `DefaultHasher` (std,
|
||||
no new dep). The fingerprint isn't hot; cross-version stability is a
|
||||
@@ -435,7 +566,99 @@ are trait derives, `fingerprint` is an inherent method).
|
||||
|
||||
---
|
||||
|
||||
## Phase 7 — Public API bump, docs, verification (ADR-011 step 6, ADR-012)
|
||||
## Phase 7 — `ValidationPlan` (ADR-012 §3)
|
||||
|
||||
**Goal:** Retire the interpretive `bast_validation` walk. Introduce a
|
||||
`ValidationPlan` — a compile-once-walk-many compiled form over the
|
||||
BAST document's value-domain constraints — built once at `compile`
|
||||
time and walked by `validate_bytes` (both modes) per buffer instead
|
||||
of re-walking `BastDoc`. Closes the latent perf cliff review #005 M3
|
||||
flagged: after ADR-011 the *read* half of `validate_bytes` is
|
||||
plan-fast, but the *validation* half still re-walks `BastDoc` per
|
||||
buffer, which is hot on the `alkcall` read+validate-on-untrusted-stream
|
||||
common case.
|
||||
|
||||
**ADR reference:** [ADR-012 §3](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md#3-validationplan--compile-once-validation-form).
|
||||
|
||||
**Predecessor for this phase:** a **design session** to scope the
|
||||
concrete `ValidationPlan` shape (constraint representation,
|
||||
`compile`/walk structure, `bast_validation` public-surface review)
|
||||
**before** implementation begins. ADR-012 §3 fixes the decision (in
|
||||
0.3.0, compiled form, no per-buffer `BastDoc` walk, `Hash + Eq` +
|
||||
`fingerprint`) and lists what is *not* decided (the struct/enum
|
||||
shape, the constraint descriptors, whether `validate_value` is
|
||||
retired or kept as a wrapper). This phase implements whatever the
|
||||
design session scopes; the contract below holds regardless of shape.
|
||||
|
||||
**Files:** New `src/validation_plan.rs` (the `ValidationPlan` type,
|
||||
`compile`, walk entry points). `src/bast_validation.rs` (adopt the
|
||||
plan; `validate_value` either becomes a thin wrapper over the plan
|
||||
or is retired per the design session's call). `src/engine.rs`
|
||||
(`compile` builds `Arc<ValidationPlan>` in both modes, stores it;
|
||||
`validate_bytes` walks `&self.validation_plan` instead of
|
||||
reconstructing a `BastDoc` for the validator — this removes the
|
||||
temporary bridge from phase 2). `src/lib.rs` (re-export
|
||||
`ValidationPlan`).
|
||||
|
||||
**Implementation contract (fixed by ADR-012 §3, independent of
|
||||
shape):**
|
||||
- **Compile-once-walk-many.** `ValidationPlan::compile` walks the
|
||||
owned `BastDoc` once (phase 3 made it owned); `validate_bytes`
|
||||
walks the `ValidationPlan` per buffer, never `BastDoc`. The only
|
||||
consumers that walk `BastDoc` interpretively after this phase are
|
||||
the one-shot `*::compile` paths (`ReadPlan::compile`,
|
||||
`OffsetMap::compute`, `ValidationPlan::compile`,
|
||||
`LayoutBuilder::new`).
|
||||
- **Value-domain, not byte-position.** The plan carries constraint
|
||||
descriptors (enum allowed-sets, integer range bounds, `maxLength`
|
||||
caps, union variant keys, and any other value-domain checks
|
||||
`bast_validation` performs today), keyed for dispatch against the
|
||||
materialized `Value` tree. The shape is different from
|
||||
`ReadPlan`/`OffsetMap`; the pattern (compiled form, immutable,
|
||||
shared via `Arc`) is the same.
|
||||
- **`Send + Sync`.** `ValidationPlan: Send + Sync` (immutable owned
|
||||
data, no interior mutability) so `Arc<ValidationPlan>` shares from
|
||||
the `Send + Sync` engine. Add a `static` bound assertion test
|
||||
mirroring phase 1's `read_plan_is_send_sync`.
|
||||
- **`Hash + Eq` + `fingerprint()`.** `ValidationPlan` derives
|
||||
`Debug, Clone, PartialEq, Eq, Hash` and has
|
||||
`fingerprint() -> u64` (same `DefaultHasher` implementation as
|
||||
phase 6). The fingerprint contract generalizes: two validation
|
||||
plans with equal hashes accept/reject identical `(bytes)`
|
||||
identically. Add a fingerprint contract test (compile the same
|
||||
schema twice, assert `plan1 == plan2` and `plan1.fingerprint() ==
|
||||
plan2.fingerprint()`; change one constraint, assert fingerprints
|
||||
differ).
|
||||
- **Untrusted-input discipline.** `compile` surfaces malformed
|
||||
schemas as `AlkTypeError::Schema` (AGENTS.md §3); overflow-safe
|
||||
arithmetic (AGENTS.md §4). No `unsafe`, no `async`, no new deps,
|
||||
wasm-clean (AGENTS.md §5–§11).
|
||||
|
||||
**What this phase does *not* include (shape-dependent, scoped by the
|
||||
design session):** the concrete `ValidationPlan` struct/enum, the
|
||||
constraint-descriptor representation, the `bast_validation`
|
||||
public-surface decision (`validate_value` retire-vs-wrapper), and any
|
||||
`AlkTypeError::Validation` variant changes. These are shape questions
|
||||
the design session resolves; they are *not* a re-opening of the
|
||||
"ship in 0.3.0" decision, which is fixed in ADR-012 §3.
|
||||
|
||||
**Verification:** `cargo test --release` — the existing
|
||||
`bast_validation.rs` and `engine.rs` `validate_bytes` tests are the
|
||||
primary validation (they drive validation through the public API and
|
||||
must pass unchanged, confirming behavioral parity with the
|
||||
interpretive walk). New unit tests for `ValidationPlan::compile`
|
||||
covering every constraint kind. New `Send + Sync` assertion test.
|
||||
New fingerprint contract tests. `cargo clippy --all-targets -- -D
|
||||
warnings`. `cargo doc --no-deps` (new public type). `cargo build
|
||||
--target wasm32-unknown-unknown --release` (`validation_plan.rs` is
|
||||
wasm-relevant). **Re-run the alktty `wire_vs_bast` bench** and, if
|
||||
the design session scopes one, a `validate_bytes`-on-untrusted-stream
|
||||
bench alongside `wire_vs_bast` to confirm the validation half of
|
||||
`validate_bytes` no longer dominates per-buffer.
|
||||
|
||||
---
|
||||
|
||||
## Phase 8 — Public API bump, docs, verification (ADR-011 step 6, ADR-012)
|
||||
|
||||
**Goal:** Flip the version to 0.3.0, update `lib.rs` re-exports, update
|
||||
the architecture docs (ADR-007 "Cost" rewrite, ADR-011/012 status flip
|
||||
@@ -446,12 +669,14 @@ consumers, run the full verification block.
|
||||
[ADR-012](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md).
|
||||
|
||||
**Files:** `Cargo.toml` (version 0.2.0 → 0.3.0), `src/lib.rs`
|
||||
(re-export `ReadPlan`, `LeafMeta`, `OffsetEntry`), `docs/architecture/`
|
||||
(README ADR table, ADR-007 "Cost" section, ADR-011/012 status),
|
||||
`docs/architecture/validation.md` / `layout-engine.md` (mention
|
||||
`ReadPlan`/`LeafMeta` where relevant), in-house downstream repos
|
||||
(`alktty`, `alkcall` — update call sites for `OffsetMap::get`,
|
||||
`SequentialReader::new`, `materialize_packed`, `BastDoc` owned).
|
||||
(re-export `ReadPlan`, `LeafMeta`, `OffsetEntry`, `ValidationPlan`),
|
||||
`docs/architecture/` (README ADR table, ADR-007 "Cost" section,
|
||||
ADR-011/012 status), `docs/architecture/validation.md` /
|
||||
`layout-engine.md` (mention `ReadPlan`/`LeafMeta`/`ValidationPlan`
|
||||
where relevant), in-house downstream repos (`alktty`, `alkcall` —
|
||||
update call sites for `OffsetMap::get`, `SequentialReader::new`,
|
||||
`materialize_packed`, `BastDoc` owned, `validate_bytes` internal
|
||||
change if any signature change surfaced in phase 7's shape work).
|
||||
|
||||
**Implementation notes:**
|
||||
- **ADR-007 "Cost" section (L2 from review #004):** rewrite the
|
||||
@@ -460,13 +685,14 @@ consumers, run the full verification block.
|
||||
"Accepted." This is the last loose end from review #004.
|
||||
- **`src/engine.rs:112-115` doc comment (L2):** rewrite the "re-parse
|
||||
the typed tree on demand" comment to describe the compiled-form
|
||||
architecture (ReadPlan for packed reads, OffsetMap+LeafMeta for
|
||||
aligned, owned BastDoc for the builder/validator).
|
||||
- **`lib.rs` re-exports:** add `ReadPlan`, `LeafMeta`, `OffsetEntry`.
|
||||
`BastDoc` and `Bast*` stay re-exported (signatures changed in phase 3,
|
||||
names unchanged). `materialize_packed`/`materialize_aligned` stay
|
||||
re-exported (signatures changed). `SequentialReader` stays
|
||||
re-exported (`new` signature changed).
|
||||
architecture (`ReadPlan` for packed reads, `OffsetMap`+`LeafMeta`
|
||||
for aligned, `ValidationPlan` for validation, owned `BastDoc` for
|
||||
the builder and the `*::compile` paths).
|
||||
- **`lib.rs` re-exports:** add `ReadPlan`, `LeafMeta`, `OffsetEntry`,
|
||||
`ValidationPlan`. `BastDoc` and `Bast*` stay re-exported (signatures
|
||||
changed in phase 3, names unchanged). `materialize_packed`/
|
||||
`materialize_aligned` stay re-exported (signatures changed).
|
||||
`SequentialReader` stays re-exported (`new` signature changed).
|
||||
- **Downstream updates:** `alktty`'s bench (`benches/wire_vs_bast.rs`)
|
||||
updates `SequentialReader::new` call + any `OffsetMap::get` usage.
|
||||
`alkcall` updates similarly. Both are in-house path dev-deps; the
|
||||
@@ -502,36 +728,52 @@ variant dispatch cost.
|
||||
## Cross-phase invariants
|
||||
|
||||
- **The tree builds and tests pass at every phase boundary.** No phase
|
||||
leaves the crate in a non-compiling state. Phase 1 (add `ReadPlan`)
|
||||
and phase 6 (add derives) are pure additions. Phases 2–5 are rewrites
|
||||
that must leave tests green.
|
||||
leaves the crate in a non-compiling state. Phases 1 (add `ReadPlan`),
|
||||
6 (add `Hash`/`Eq` derives to `ReadPlan`/`OffsetMap`), and 7 (add
|
||||
`ValidationPlan`) are pure additions; phases 2–5 are rewrites that
|
||||
must leave tests green; phase 8 is the bump/docs.
|
||||
- **The POC on `readplan-poc` is the reference scaffold for phases 1–2.**
|
||||
It is *not* merged to main; it stays on the branch as the derisking
|
||||
record, like `bast-validator-poc`. If a phase 1–2 implementation
|
||||
question arises about the plan shape, consult the POC.
|
||||
question arises about the plan shape, consult the POC. (The POC's
|
||||
`VariantPlan`/`VariantKind` and its nested-union rejection are
|
||||
**not** carried forward — phase 1's refined `CompositePlan::Union`
|
||||
shape supersedes both; see phase 1.)
|
||||
- **Review #004 is the closure target.** H1 → phase 2; packed M1 →
|
||||
phase 2; aligned M1 → phases 4–5; L1 → phase 2 (falls out); L2 →
|
||||
phase 7 (doc rewrite). The review's status flips to "closed" in the
|
||||
phase 7 commit.
|
||||
phase 8 (doc rewrite). The review's status flips to "closed" in the
|
||||
phase 8 commit.
|
||||
- **Review #005 is the closure target for the plan-spec issues.** H1
|
||||
→ phase 1 (refined union shape in ADR-011 + plan); H2 → phase 2
|
||||
(`Arc<Value>` on the plan); M1 → phase 1 (nested unions via
|
||||
`CompositePlan` recursion, no behavioral drop); M2 → phase 5 (`Hash`
|
||||
on `Endian`/`VariableEncoding`); M3 → ADR-012 §3 + phase 7
|
||||
(`ValidationPlan` shipped in 0.3.0, deferral reversed); L1/L2/L3 →
|
||||
phase 2 / phase 5 Scope Boundary; N1 → typo; N2 → phase 1
|
||||
`Send + Sync` assertion test; N3 → Semver Contract table row.
|
||||
- **No `unsafe`, no `async`, no new deps, no feature flags** (AGENTS.md
|
||||
§5–§11). The owned-`BastDoc` refactor uses `String`/`Value`, not
|
||||
`unsafe` self-referential tricks. `DefaultHasher` is std. Wasm-clean
|
||||
throughout.
|
||||
throughout. `ValidationPlan` follows the same constraints.
|
||||
- **`preserve_order` stays load-bearing** (AGENTS.md §8). The owned-
|
||||
`BastDoc` refactor must not sort schema object keys anywhere; field
|
||||
order in the `Value` still determines byte order in packed mode and
|
||||
iteration order in both modes.
|
||||
iteration order in both modes. `ValidationPlan::compile` inherits
|
||||
this — value-domain checks that depend on field ordering (e.g. union
|
||||
discriminator field lookup) respect `preserve_order`.
|
||||
|
||||
## What this plan is *not*
|
||||
|
||||
- **Not a `ValidationPlan`.** Deferred per ADR-012 — different shape,
|
||||
not a hot loop.
|
||||
- **Not a disk-cache or wire-protocol spec.** The fingerprint contract
|
||||
and method are in scope (phase 6); downstream uses are the consumers'
|
||||
concern.
|
||||
and method are in scope (phase 6 for `ReadPlan`/`OffsetMap`, phase 7
|
||||
for `ValidationPlan`); downstream uses are the consumers' concern.
|
||||
- **Not cross-version fingerprint stability.** Within-version only
|
||||
(ADR-012). The fingerprint may change across versions if a new
|
||||
`AlkTypeKind` variant is added; consumers cache within a version.
|
||||
- **Not a perf bench.** The bench lives in alktty; this plan re-runs it
|
||||
at phase 2 and phase 7 to confirm the gap closes. The plan itself
|
||||
only asserts correctness/coverage.
|
||||
at phase 2 and phase 8 to confirm the gap closes. The plan itself
|
||||
only asserts correctness/coverage.
|
||||
- **Not an `AlignedPlan`.** Aligned `materialize`'s `BastDoc` structure
|
||||
walk is the permanent 0.3.0 design (phase 5 Scope Boundary). An
|
||||
`AlignedPlan` is out of scope; if a future bench motivates one, it
|
||||
gets its own ADR.
|
||||
@@ -1,6 +1,7 @@
|
||||
---
|
||||
status: open
|
||||
last_updated: 2026-08-19
|
||||
last_updated: 2026-08-20
|
||||
resolved_findings: 2026-08-20 (all 11 — see "Resolution" at the end)
|
||||
reviewed_artifacts:
|
||||
- docs/plans/030-compiled-forms.md
|
||||
- docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md
|
||||
@@ -580,4 +581,79 @@ non-blocking and can run in parallel.
|
||||
holes) alongside classic planning mistakes. It surfaced M1 and L3
|
||||
that a conventional severity-only review would have missed or
|
||||
under-weighted. Worth retaining as a default scan for future plan
|
||||
reviews.
|
||||
reviews.
|
||||
|
||||
---
|
||||
|
||||
## Resolution (2026-08-20)
|
||||
|
||||
All 11 findings resolved in one docs-only edit pass to ADR-011,
|
||||
ADR-012, and the 0.3.0 plan. No source changed; the crate still
|
||||
builds/tests at v0.2.0. The M3 deferral reversal is the one
|
||||
substantive decision change (per user direction: ship ValidationPlan
|
||||
in 0.3.0, no more hedging); the rest are spec corrections or
|
||||
pre-implementation refinements to types that do not yet exist on
|
||||
`main`.
|
||||
|
||||
- **H1 (union shape):** ADR-011 §"The `ReadPlan` shape" refined —
|
||||
`CompositePlan::Union` now carries `shared: Option<Box<ReadPlan>>`
|
||||
(field-disc shared fields) and `variants: Vec<(String,
|
||||
CompositePlan)>` (dropping `VariantPlan`/`VariantKind`). Plan
|
||||
phase 1 rewritten to implement the refined shape. The shape
|
||||
refinement is pre-implementation (the types don't exist on `main`).
|
||||
- **H2 (`schema()` `&Value`):** plan phase 2 rewritten — `ReadPlan`
|
||||
stores `schema: Arc<Value>` (not `&Value`); `schema()` returns
|
||||
`&self.schema`. Verified `serde_json::Value: Hash + Eq` holds with
|
||||
`preserve_order` (`Map::hash` sorts keys deterministically), so
|
||||
phase 6's `#[derive(Hash)]` on `ReadPlan` is not blocked.
|
||||
- **M1 (nested unions):** resolved as the review's option (a) —
|
||||
nested-union support falls out of the H1 shape refinement (a
|
||||
variant can be `CompositePlan::Union`), so no behavioral drop vs
|
||||
0.2.0 and no Semver Contract entry for a capability regression.
|
||||
Plan phase 1 adds a nested-union-variant test.
|
||||
- **M2 (`Hash` on `Endian`/`VariableEncoding`):** plan phase 5
|
||||
rewritten with an explicit first sub-step to add `Hash` to both
|
||||
derives in `src/schema.rs` (additive, semver-safe). The inaccurate
|
||||
"all fields are `Copy + Hash`" parenthetical on `LeafMeta` is
|
||||
corrected.
|
||||
- **M3 (`ValidationPlan`):** deferral **reversed** per user
|
||||
direction. ADR-012 §"Deferring `ValidationPlan`" rewritten as
|
||||
"ValidationPlan — in scope for 0.3.0"; new ADR-012 §3 commits the
|
||||
decision (compiled form, no per-buffer `BastDoc` walk, `Hash + Eq`
|
||||
+ `fingerprint()`) and lists the shape questions deferred to a
|
||||
follow-on design session + the plan's new phase 7. Plan gains a
|
||||
new phase 7 (ValidationPlan); old phase 7 (bump) renumbered to
|
||||
phase 8. ADR-011's "Out of scope" `bast_validation` bullet and
|
||||
"Scope Boundaries" `Not a validation plan` bullet updated to point
|
||||
at ADR-012 §3. Plan's "What this plan is *not*" first bullet
|
||||
removed. The deferral-black-hole pattern this review's methodology
|
||||
flagged is closed: the work is committed in the plan with a
|
||||
concrete reactivation trigger (the shape session before phase 7),
|
||||
not hedged into an unplanned future.
|
||||
- **L1 (`dummy_field_for`/`ty_source`):** plan phase 2 rewritten —
|
||||
only the packed-side call sites go away; the helpers stay for the
|
||||
aligned `materialize_leaf_at` path.
|
||||
- **L2 (`materialize_typeref_packed` split):** plan phase 2
|
||||
rewritten — packed-materialize gets a new plan-walking function;
|
||||
`materialize_typeref_packed` stays for aligned's
|
||||
`materialize_leaf_at` and the aligned record path.
|
||||
- **L3 (aligned-materialize `BastDoc` structure walk):** plan phase 5
|
||||
gains a Scope Boundary note — the walk is the permanent 0.3.0
|
||||
design; an `AlignedPlan` is out of scope, tracked as an open
|
||||
question if a future bench motivates it.
|
||||
- **N1 (typo):** "back-comat" → "back-compat" in deferred decision 4.
|
||||
- **N2 (`Send + Sync` assertion test):** plan phase 1 verification
|
||||
rewritten to add the `read_plan_is_send_sync` static-bound
|
||||
assertion test ADR-011 §"Engine integration" requires.
|
||||
- **N3 (`Result` drop on `SequentialReader::new`):** Semver Contract
|
||||
table row updated to note the constructor return-type change
|
||||
(`Result<Self, AlkTypeError>` → `Self`) alongside the argument-type
|
||||
change.
|
||||
|
||||
The deferral-pattern scan's general signal (healthy deferrals have a
|
||||
concrete reactivation condition + tracking; black holes have neither)
|
||||
is reaffirmed by the M3 reversal: the original "if a bench motivates
|
||||
it" trigger was a black hole because no bench was ever going to be
|
||||
run against a path that didn't exist yet, and the cost of inaction
|
||||
(a second breaking change to `validate_bytes`/`bast_validation` after
|
||||
0.3.0) was hidden by the "not a hot loop" framing.
|
||||
Reference in new issue
Block a user