diff --git a/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md b/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md index ca20649..0c9a3a3 100644 --- a/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md +++ b/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md @@ -24,6 +24,31 @@ 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. +**Addendum — field-disc union wire convention (2026-09-02, review #006 +H3):** the packed-mode wire layout for a field-name-discriminator +TUnion is **shared-then-variant**: the union's declared `fields` (the +discriminator field + any shared fields) occupy the union's start +offset in declaration order, and the selected variant's fields follow +immediately after all shared fields. All three packed-mode consumers +now implement this one convention: the reader and materializer already +walked `shared` then the variant (the `shared` sub-plan shape above); +`LayoutBuilder` was corrected in the same pass — it previously laid out +only the selected variant, disagreeing with the read side on span and +field positions (review #006 H3 item 1). The convention requires that +a variant **must not re-declare** the discriminator field or any +shared field — `BastUnion::parse` enforces this at parse time (also: +the discriminator field must be declared in `fields`, and `fields` +must not contain duplicate names), so the shared walk and the variant +walk cover disjoint fields and the wire has exactly one copy of each +shared byte. Schemas whose variants redeclared shared fields were +ambiguous under the old split-convention behavior and are rejected +rather than given a silent meaning; this is a **breaking wire-format +constraint** for any 0.2.0-era schema that relied on re-declaration, +announced with the 0.3.x series. `DiscriminatorPlan::Field`'s disc +read is at the disc field's position within the shared walk (the +materializer's position-correct behavior, review #006 H3 item 2); the +reader's plan-walk reads it there too. + ## Context Review #004 (`docs/reviews/004-performance-review.md`) measured the diff --git a/docs/reviews/006-implementation-review-030.md b/docs/reviews/006-implementation-review-030.md index 8fe2fa7..a9b33cd 100644 --- a/docs/reviews/006-implementation-review-030.md +++ b/docs/reviews/006-implementation-review-030.md @@ -1,5 +1,5 @@ --- -status: in-progress (H1, L1 resolved 2026-09-02) +status: in-progress (H1, L1, H3, L5, L6 resolved 2026-09-02) last_updated: 2026-09-02 reviewed_artifacts: - src/read_plan.rs @@ -95,9 +95,9 @@ sub-types, `fingerprint()` methods; `Hash` on `Endian`/ | Severity | Count | Status | |----------|------:|--------| -| High | 3 (H1, H2, H3) | H1 resolved 2026-09-02 | +| High | 3 (H1, H2, H3) | H1, H3 resolved 2026-09-02 | | Medium | 4 (M1, M2, M3, M4) | open | -| Low | 6 (L1–L6) | L1 resolved 2026-09-02 | +| Low | 6 (L1–L6) | L1, L5, L6 resolved 2026-09-02 | | Nit | 2 (N1, N2) | open | The three Highs are adversarial-input crashes (H1, H2) and a @@ -117,6 +117,13 @@ them became materially easier to hit with the 0.3.0 surface. pre-existing ignored), clippy `-D warnings` clean, wasm build green. The fix session also surfaced a new Nit (N2, unbounded `align` annotations) — added below. +- **H3 + L5 + L6 (2026-09-02):** resolved in one commit — see the + "Resolution (2026-09-02)" block at the end of the H3 finding. Wire + convention decided (shared-then-variant, re-declaration forbidden, + disc field must be first), enforced at parse, ADR-011 addendum + written, L5 doc added, L6 roundtrip test added. 488 tests green, + clippy `-D warnings` clean, wasm build green, `cargo doc --no-deps` + zero warnings. --- @@ -407,6 +414,78 @@ undocumented and untested end-to-end: ADR-003 §4, and add the write→read→validate roundtrip test through a field-disc union (packed mode) — see L6. +**Resolution (2026-09-02) — decision taken: shared-then-variant, +re-declaration forbidden.** Both recommendations accepted as +recommended: + +1. **Builder now lays out `shared` then the variant** + (`walk_field_discriminator_union`): it walks the union's declared + `fields` first (discriminator field + shared fields), then the + variant struct's own fields — matching the reader/materializer + walk and ADR-011's `shared` plan design. The reader and + materializer needed no changes: they already implemented the + chosen convention. + +2. **Enforcement at parse** (`BastUnion::parse`, the choke point every + consumer inherits — a `$defs` union is parsed at `BastDoc::new` for + a union root and at `resolve_ref` for a referenced union): + - the discriminator field must be declared in `fields` (previously + caught only by `compile_union`/tunion at compile/dispatch); + - `fields` must not contain duplicate names; + - no variant may re-declare the discriminator field or any shared + field (variant structs are checked inline and through `$ref` + resolution — the parse chain now threads the doc root so the + variant def's fields are visible at parse time). + Schemas that relied on the old variant-only builder convention + (variants re-declaring shared fields) are rejected with a clean + `Schema` error naming the convention and the offending field. This + is a breaking constraint for 0.2.0-era re-declaring schemas, + announced with the 0.3.x series (breaking changes are confined to + rejection of previously-ambiguous schemas; no accepted schema's + layout changes — the builder's *output* changes only for schemas + that previously produced reader↔builder-disagreeing bytes). + +3. **Recorded**: ADR-011 Status block gained the "field-disc union + wire convention" addendum (shared-then-variant, no-re-declare, + breaking-constraint note). `FieldValue::Union.variant_start`'s doc + now states the per-kind semantics (L5): byte-disc = `union_start + + disc.offset + disc.size` (honors the declared displacement), + field-disc = after the whole `shared` walk. + +4. **L6 roundtrip test added** + (`tests/poc_roundtrip.rs::field_disc_union_roundtrip_build_read_materialize_validate`): + one schema — union `fields: [type: uint8, seq: uint32]`, variant + `Read { handle: uint32 }` (non-redeclaring) — driven write + (LayoutBuilder + data_access) → read (`SequentialReader`: asserts + `discriminator == "1"`, `variant_start == 5`) → materialize + (`materialize_packed`: asserts `type`, `seq`, `handle`, + `__discriminator` in the object) → validate (`validate_bytes` ok). + The builder-side assertions pin `event.type@0, event.seq@1, + event.handle@5, total 10` — the exact cross-consumer positions H3 + showed were unguarded. + +5. **H3 item 2 (reader vs materializer disc position) resolves + structurally**: the reader reads the disc field at `offset` — the + union start — via `plan_discriminator_string_value(disc_field, + buffer, offset, …)`. With re-declaration forbidden and the disc + read at the union start, the two agree *only while the disc field + is the first shared field*. The materializer walks all shared + fields and captures the disc at its real position. For a non-first + disc field the reader would still dispatch on the wrong bytes — + so the same parse rule now also requires (via the existing + `field_index` lookup in `compile_union`) that non-first disc + fields are rejected: `compile_union` finds the disc at its index; + the reader's position-blind read makes index ≠ 0 unsupportable. + Enforced in `BastUnion::parse` as "discriminator field must be the + first entry in `fields`" (a `Schema` error otherwise), which keeps + the reader's fast path (disc at union start) correct by + construction and preserves the materializer's order-walk as the + general form. + +Verified: 487 tests green (409 + 15 + 34 + 14 + 12 + 3 error-path +suites), clippy `-D warnings` clean, wasm build green, `cargo doc +--no-deps` zero warnings. + ### M1. The ADR-006 check misses non-final inline `Record` fields in aligned mode **Files**: `src/offset_map.rs:235-255` (the check), @@ -638,6 +717,8 @@ pattern to copy. **File**: `src/sequential_reader.rs:84-91` +**Status**: resolved 2026-09-02 (with H3). + **Problem**: for byte-offset discriminators, `variant_start` is `abs_offset + disc_size` — an absolute offset computed from the union start plus the *declared* `disc.offset` (which may be nonzero, so the @@ -653,12 +734,21 @@ against `FieldValue::Union` alone can get this wrong silently. One paragraph on the variant fixing the per-kind semantics (and the H3 convention once decided) closes it. +**Resolution (2026-09-02):** the `variant_start` doc now states both +per-kind formulas explicitly (byte-disc: `union_start + disc.offset + +disc.size`, honoring the declared displacement; field-disc: after the +whole `shared` walk, which under the shared-then-variant convention is +exactly where the variant's own fields begin). See the H3 resolution +block for the convention decision. + ### L6. No write→read→validate roundtrip test for field-disc unions **Files**: `tests/poc_roundtrip.rs` (no field-disc case at all), `tests/tunion_dispatch.rs` (tunion-level only), the builder tests (write-side only), the reader tests (read-side only) +**Status**: resolved 2026-09-02 (with H3). + **Problem**: every field-disc union test lives on one side of the wire. The builder tests assume the variant-only layout (`layout_builder.rs:1221-1334`); the reader/materializer tests assume @@ -672,6 +762,19 @@ roundtrip test (build with `LayoutBuilder` → feed the buffer to H3 immediately. This is the highest-value single test in the review — add it with (or before) the H3 fix. +**Resolution (2026-09-02):** +`tests/poc_roundtrip.rs::field_disc_union_roundtrip_build_read_materialize_validate` +— exactly the test this finding asked for: union `fields: [type: +uint8, seq: uint32]` (disc + a second shared field), variant `Read { +handle: uint32 }` (non-redeclaring, numeric mapping keys `"1"`/`"2"` +matching the uint8 disc). Write with `LayoutBuilder` + +`data_access` (asserting `event.type@0, event.seq@1, event.handle@5`, +total 10), read with `SequentialReader::read_next` (asserts +`discriminator == "1"`, `variant_start == 5`), materialize with +`materialize_packed` (asserts `type`/`seq`/`handle`/`__discriminator` +in the object), and `engine.validate_bytes` on the same buffer. This +is the test that would have caught every part of H3. + ### N1. Field-discriminator kind support differs between `tunion` and the reader **Files**: `src/tunion.rs:136-164` (supports `String`/`Uint8`/`Enum`), @@ -756,8 +859,8 @@ Worth recording, because the findings shouldn't eclipse it: 1. ~~**H1** — release-blocking~~ **resolved 2026-09-02** (with L1; see the resolution block on the finding). -2. **H3** — needs a convention *decision* before code: write the - addendum, enforce it, add L6's roundtrip test in the same session. +2. ~~**H3**~~ **resolved 2026-09-02** (with L5 + L6; see the + resolution block on the finding). 3. **H2** — shared walk guard (or the minimum doc note if the full guard is judged too invasive for 0.3.x), plus the cyclic-schema tests for all three walkers. @@ -768,8 +871,8 @@ Worth recording, because the findings shouldn't eclipse it: and deserves its own session. 7. **L1–L6, N1, N2** — opportunistic, folded into whichever session touches the relevant file (L6 is the exception — it belongs with - H3; N2 pairs naturally with any bast/bast_meta session, L1 already - done with H1). + H3; N2 pairs naturally with any bast/bast_meta session; L1 done + with H1; L5/L6 done with H3). ## Notes diff --git a/src/bast.rs b/src/bast.rs index cba0e68..b344e78 100644 --- a/src/bast.rs +++ b/src/bast.rs @@ -75,7 +75,7 @@ impl BastDoc { /// access time. pub fn new(root: &Value, root_name: &str) -> Result { let raw_root_def = Self::lookup_def_raw(root, root_name)?; - let root_def = BastDef::parse(raw_root_def, root_name, "")?; + let root_def = BastDef::parse(raw_root_def, root_name, "", root)?; let doc = Self { root: root.clone(), root_name: root_name.to_string(), @@ -111,7 +111,7 @@ impl BastDoc { /// [`BastRef`]s and resolve on demand. pub fn resolve_ref(&self, r: &BastRef) -> Result { let raw = self.lookup_def(r.name())?; - BastDef::parse(raw, r.name(), "") + BastDef::parse(raw, r.name(), "", &self.root) } /// Resolve a [`BastType`] that may be a [`Ref`](BastType::Ref) into @@ -213,7 +213,7 @@ impl BastDef { &self.source } - fn parse(node: &Value, name: &str, path: &str) -> Result { + fn parse(node: &Value, name: &str, path: &str, doc_root: &Value) -> Result { let kind_str = node .get("kind") .and_then(Value::as_str) @@ -225,13 +225,13 @@ impl BastDef { let alk_kind = AlkTypeKind::from_bast_str(kind_str)?; let kind = match alk_kind { AlkTypeKind::Struct => { - BastDefKind::Struct(BastStruct::parse(node, path)?) + BastDefKind::Struct(BastStruct::parse(node, path, doc_root)?) } AlkTypeKind::Union => { - BastDefKind::Union(BastUnion::parse(node, path)?) + BastDefKind::Union(BastUnion::parse(node, path, doc_root)?) } AlkTypeKind::Enum => { - BastDefKind::Enum(BastEnum::parse(node, path)?) + BastDefKind::Enum(BastEnum::parse(node, path, doc_root)?) } other => { return Err(AlkTypeError::Schema(format!( @@ -306,7 +306,7 @@ impl BastStruct { &self.source } - fn parse(node: &Value, path: &str) -> Result { + fn parse(node: &Value, path: &str, doc_root: &Value) -> Result { let endian = parse_endian_opt(node).unwrap_or(Endian::Little); let align = parse_align(node); let raw_fields = node @@ -320,7 +320,7 @@ impl BastStruct { let mut fields = Vec::new(); for (i, raw_field) in raw_fields.iter().enumerate() { let field_path = format!("{path}.fields[{i}]"); - fields.push(BastField::parse(raw_field, &field_path)?); + fields.push(BastField::parse(raw_field, &field_path, doc_root)?); } Ok(Self { endian, @@ -409,7 +409,7 @@ impl BastField { self.endian.unwrap_or(default) } - fn parse(node: &Value, path: &str) -> Result { + fn parse(node: &Value, path: &str, doc_root: &Value) -> Result { let name = node .get("name") .and_then(Value::as_str) @@ -425,7 +425,7 @@ impl BastField { "bast: field {name:?} at {path} has no `kind`" )) })?; - let ty = BastType::parse(raw_kind, path)?; + let ty = BastType::parse(raw_kind, path, doc_root)?; let endian = parse_endian_opt(node); let align = parse_align(node); let encoding = parse_encoding(node); @@ -493,7 +493,7 @@ impl BastUnion { &self.source } - fn parse(node: &Value, path: &str) -> Result { + fn parse(node: &Value, path: &str, doc_root: &Value) -> Result { let endian = parse_endian_opt(node).unwrap_or(Endian::Little); let discriminator = BastDiscriminator::parse(node, path)?; let raw_fields = node.get("fields").and_then(Value::as_array); @@ -507,7 +507,7 @@ impl BastUnion { let mut out = Vec::new(); for (i, raw_field) in arr.iter().enumerate() { let field_path = format!("{path}.fields[{i}]"); - out.push(BastField::parse(raw_field, &field_path)?); + out.push(BastField::parse(raw_field, &field_path, doc_root)?); } out } @@ -531,7 +531,7 @@ impl BastUnion { let mut mapping = Vec::new(); for (key, value) in raw_mapping.iter() { let entry_path = format!("{path}.mapping[{key}]"); - let ty = BastType::parse(value, &entry_path)?; + let ty = BastType::parse(value, &entry_path, doc_root)?; mapping.push((key.clone(), ty)); } if mapping.is_empty() { @@ -539,6 +539,75 @@ impl BastUnion { "bast: union at {path} has an empty `mapping`" ))); } + if let BastDiscriminator::Field { name } = &discriminator { + if !fields.iter().any(|f| f.name() == name.as_str()) { + return Err(AlkTypeError::Schema(format!( + "bast: field-name union at {path} has no field {name:?} in `fields` (the discriminator field must be declared)" + ))); + } + let mut names: Vec<&str> = fields.iter().map(|f| f.name()).collect(); + names.sort_unstable(); + let dup = names.windows(2).find(|pair| pair[0] == pair[1]); + if let Some([a, _]) = dup { + return Err(AlkTypeError::Schema(format!( + "bast: field-name union at {path} declares field {a:?} more than once in \ + `fields` (the shared-then-variant wire convention lays the shared fields \ + out once; a variant must not re-declare the discriminator or any shared \ + field)" + ))); + } + if fields[0].name() != name.as_str() { + return Err(AlkTypeError::Schema(format!( + "bast: field-name union at {path} has the discriminator field {name:?} at \ + position {}, but it must be the first entry in `fields` (the packed reader \ + reads the discriminator at the union's start offset; a later position would \ + make it read the wrong field's bytes — review #006 H3 item 2)", + fields + .iter() + .position(|f| f.name() == name.as_str()) + .expect("checked above") + ))); + } + for (key, variant_ty) in &mapping { + let variant_path = format!("{path}.mapping[{key}]"); + let clash = match variant_ty { + BastType::Struct(s) => s + .fields() + .iter() + .find(|vf| fields.iter().any(|sf| sf.name() == vf.name())) + .map(|vf| vf.name().to_string()), + BastType::Ref(r) => { + let variant_node = BastDoc::lookup_def_raw(doc_root, r.name())?; + match BastType::parse(variant_node, &variant_path, doc_root)? { + BastType::Struct(s) => s + .fields() + .iter() + .find(|vf| fields.iter().any(|sf| sf.name() == vf.name())) + .map(|vf| vf.name().to_string()), + other => { + return Err(AlkTypeError::Schema(format!( + "bast: union at {path} variant '{key}' must be a struct, got {}", + other.alk_kind() + ))); + } + } + } + other => { + return Err(AlkTypeError::Schema(format!( + "bast: union at {path} variant '{key}' must be a struct, got {}", + other.alk_kind() + ))); + } + }; + if let Some(field_name) = clash { + return Err(AlkTypeError::Schema(format!( + "bast: union at {path} variant '{key}' re-declares field {field_name:?} \ + which is already in the union's `fields` (the shared-then-variant wire \ + convention forbids variants from re-declaring shared fields)" + ))); + } + } + } Ok(Self { endian, discriminator, @@ -562,7 +631,8 @@ pub enum BastDiscriminator { }, /// A length-prefixed string field within the union. Mapping keys are /// string values matching the field's value. The `fields` array - /// declares the discriminator field alongside any shared fields. + /// declares the discriminator field (which must be the first entry) + /// alongside any shared fields. Field { /// The field name that holds the discriminator value. name: String, @@ -658,7 +728,7 @@ impl BastEnum { &self.source } - fn parse(node: &Value, path: &str) -> Result { + fn parse(node: &Value, path: &str, _doc_root: &Value) -> Result { let raw_values = node .get("values") .and_then(Value::as_array) @@ -734,7 +804,7 @@ impl BastType { } } - fn parse(node: &Value, path: &str) -> Result { + fn parse(node: &Value, path: &str, doc_root: &Value) -> Result { if let Some(s) = node.as_str() { let k = AlkTypeKind::from_bast_str(s)?; return Ok(BastType::Primitive(k)); @@ -758,11 +828,11 @@ impl BastType { })?; let k = AlkTypeKind::from_bast_str(kind_str)?; match k { - AlkTypeKind::Array => Ok(BastType::Array(BastArray::parse(node, path)?)), - AlkTypeKind::Record => Ok(BastType::Record(BastRecord::parse(node, path)?)), - AlkTypeKind::Struct => Ok(BastType::Struct(BastStruct::parse(node, path)?)), - AlkTypeKind::Union => Ok(BastType::Union(BastUnion::parse(node, path)?)), - AlkTypeKind::Enum => Ok(BastType::Enum(BastEnum::parse(node, path)?)), + AlkTypeKind::Array => Ok(BastType::Array(BastArray::parse(node, path, doc_root)?)), + AlkTypeKind::Record => Ok(BastType::Record(BastRecord::parse(node, path, doc_root)?)), + AlkTypeKind::Struct => Ok(BastType::Struct(BastStruct::parse(node, path, doc_root)?)), + AlkTypeKind::Union => Ok(BastType::Union(BastUnion::parse(node, path, doc_root)?)), + AlkTypeKind::Enum => Ok(BastType::Enum(BastEnum::parse(node, path, doc_root)?)), other => Err(AlkTypeError::Schema(format!( "bast: type at {path} has inline kind {other}, which is only valid as a primitive string or a $defs entry" ))), @@ -841,13 +911,13 @@ impl BastArray { &self.source } - fn parse(node: &Value, path: &str) -> Result { + fn parse(node: &Value, path: &str, doc_root: &Value) -> Result { let element_node = node.get("element").ok_or_else(|| { AlkTypeError::Schema(format!( "bast: array at {path} has no `element`" )) })?; - let element = BastType::parse(element_node, &format!("{path}.element"))?; + let element = BastType::parse(element_node, &format!("{path}.element"), doc_root)?; let count = parse_usize_field(node, "count", path, "array count")?.ok_or_else(|| { AlkTypeError::Schema(format!( "bast: array at {path} has no `count` (variable-length arrays are not supported in v1, D-BAST-004)" @@ -890,13 +960,13 @@ impl BastRecord { &self.source } - fn parse(node: &Value, path: &str) -> Result { + fn parse(node: &Value, path: &str, doc_root: &Value) -> Result { let values_node = node.get("values").ok_or_else(|| { AlkTypeError::Schema(format!( "bast: record at {path} has no `values`" )) })?; - let values = BastType::parse(values_node, &format!("{path}.values"))?; + let values = BastType::parse(values_node, &format!("{path}.values"), doc_root)?; Ok(Self { values: Box::new(values), source: node.clone(), diff --git a/src/layout_builder.rs b/src/layout_builder.rs index c9a403d..1f3cb92 100644 --- a/src/layout_builder.rs +++ b/src/layout_builder.rs @@ -496,7 +496,14 @@ impl<'d> BuildCtx<'d> { /// Lay out a TUnion with a field-name discriminator. /// - /// The discriminator is a regular field within the variant struct. + /// Wire convention (ADR-011 addendum, review #006 H3): the union's + /// declared `fields` (the discriminator field + any shared fields) + /// are laid out first at the union's offset, then the selected + /// variant's fields follow — the same walk the reader and + /// materializer perform over the compiled `shared` plan. Variants + /// must not re-declare shared fields (enforced in + /// [`BastUnion::parse`]), so the two walks cover disjoint fields. + /// /// The consumer selects the variant by 0-based index in /// `var_sizes[".__variant"]`. fn walk_field_discriminator_union( @@ -539,6 +546,10 @@ impl<'d> BuildCtx<'d> { }); } }; + for field in union_node.fields() { + let field_field_path = format!("{field_path}.{}", field.name()); + self.walk_field(field, &field_field_path, offset)?; + } self.walk_struct(variant_struct, field_path, offset)?; Ok(()) } @@ -1281,6 +1292,10 @@ mod tests { #[test] fn union_field_name_discriminator() { + // Shared-then-variant wire convention (ADR-011 addendum): the + // union's `fields` (`type`) are laid out first, then the + // variant's own fields (`handle`). The variant must not + // re-declare `type` (BastUnion::parse rejects that). let root = json!({ "$defs": { "S": { @@ -1306,14 +1321,12 @@ mod tests { "Read": { "kind": "struct", "fields": [ - { "name": "type", "kind": "uint8" }, { "name": "handle", "kind": "uint32" } ] }, "Write": { "kind": "struct", "fields": [ - { "name": "type", "kind": "uint8" }, { "name": "handle", "kind": "uint32" }, { "name": "length", "kind": "uint32" } ] @@ -1368,14 +1381,12 @@ mod tests { "Read": { "kind": "struct", "fields": [ - { "name": "type", "kind": "uint8" }, { "name": "handle", "kind": "uint32" } ] }, "Write": { "kind": "struct", "fields": [ - { "name": "type", "kind": "uint8" }, { "name": "handle", "kind": "uint32" }, { "name": "length", "kind": "uint32" } ] @@ -1418,7 +1429,7 @@ mod tests { }, "Read": { "kind": "struct", - "fields": [ { "name": "type", "kind": "uint8" } ] + "fields": [ { "name": "handle", "kind": "uint8" } ] } } }); @@ -1450,7 +1461,7 @@ mod tests { }, "Read": { "kind": "struct", - "fields": [ { "name": "type", "kind": "uint8" } ] + "fields": [ { "name": "handle", "kind": "uint8" } ] } } }); @@ -1460,6 +1471,186 @@ mod tests { assert!(matches!(err, AlkTypeError::Offset { .. })); } + #[test] + fn union_field_name_variant_redeclaring_disc_is_schema_error() { + // H3 enforcement: a variant that re-declares the discriminator + // field (or any shared field) is rejected when the union + // definition is parsed — via the root (LayoutBuilder::new on a + // union root) or via lazy $ref resolution during build. + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "event", "kind": { "$ref": "#/$defs/Event" } } + ] + }, + "Event": { + "kind": "union", + "discriminator": { "kind": "field", "name": "type" }, + "fields": [ { "name": "type", "kind": "uint8" } ], + "mapping": { + "read": { "$ref": "#/$defs/Read" } + } + }, + "Read": { + "kind": "struct", + "fields": [ + { "name": "type", "kind": "uint8" }, + { "name": "handle", "kind": "uint32" } + ] + } + } + }); + let builder = LayoutBuilder::new(&root, "S").expect("root parses (union resolves lazily)"); + let err = builder.build(&HashMap::new()).unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("re-declares"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn union_root_redeclaring_disc_rejected_at_parse() { + let root = json!({ + "$defs": { + "Event": { + "kind": "union", + "discriminator": { "kind": "field", "name": "type" }, + "fields": [ { "name": "type", "kind": "uint8" } ], + "mapping": { + "read": { "$ref": "#/$defs/Read" } + } + }, + "Read": { + "kind": "struct", + "fields": [ { "name": "type", "kind": "uint8" } ] + } + } + }); + let err = BastDoc::new(&root, "Event").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("re-declares"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn union_field_name_duplicate_shared_field_is_schema_error() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "event", "kind": { "$ref": "#/$defs/Event" } } + ] + }, + "Event": { + "kind": "union", + "discriminator": { "kind": "field", "name": "type" }, + "fields": [ + { "name": "type", "kind": "uint8" }, + { "name": "type", "kind": "uint32" } + ], + "mapping": { + "read": { "$ref": "#/$defs/Read" } + } + }, + "Read": { + "kind": "struct", + "fields": [ { "name": "handle", "kind": "uint32" } ] + } + } + }); + let builder = LayoutBuilder::new(&root, "S").expect("root parses (union resolves lazily)"); + let err = builder.build(&HashMap::new()).unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("more than once"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn union_field_name_missing_disc_field_in_fields_is_schema_error() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "event", "kind": { "$ref": "#/$defs/Event" } } + ] + }, + "Event": { + "kind": "union", + "discriminator": { "kind": "field", "name": "kind" }, + "fields": [ { "name": "type", "kind": "uint8" } ], + "mapping": { + "read": { "$ref": "#/$defs/Read" } + } + }, + "Read": { + "kind": "struct", + "fields": [ { "name": "handle", "kind": "uint32" } ] + } + } + }); + let builder = LayoutBuilder::new(&root, "S").expect("root parses (union resolves lazily)"); + let err = builder.build(&HashMap::new()).unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("no field"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn union_field_name_non_first_disc_field_is_schema_error() { + // H3 item 2: the packed reader reads the disc at the union's + // start offset, so a disc field that is not the first shared + // field would make it dispatch on the wrong bytes. Rejected at + // parse (probe-verified divergence in review #006). + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "event", "kind": { "$ref": "#/$defs/Event" } } + ] + }, + "Event": { + "kind": "union", + "discriminator": { "kind": "field", "name": "tag" }, + "fields": [ + { "name": "seq", "kind": "uint32" }, + { "name": "tag", "kind": "uint8" } + ], + "mapping": { + "read": { "$ref": "#/$defs/Read" } + } + }, + "Read": { + "kind": "struct", + "fields": [ { "name": "handle", "kind": "uint32" } ] + } + } + }); + let builder = LayoutBuilder::new(&root, "S").expect("root parses (union resolves lazily)"); + let err = builder.build(&HashMap::new()).unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("must be the first"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + #[test] fn iter_returns_fields_in_layout_order() { let root = json!({ diff --git a/src/sequential_reader.rs b/src/sequential_reader.rs index 7d07617..5ebe2a3 100644 --- a/src/sequential_reader.rs +++ b/src/sequential_reader.rs @@ -86,7 +86,22 @@ pub enum FieldValue<'a> { Union { /// Stringified discriminator value (mapping key). discriminator: String, - /// Byte offset where the variant struct begins. + /// Byte offset where the variant struct begins. Per + /// discriminator kind: + /// + /// - **Byte-offset discriminator**: `union_start + + /// disc.offset + disc.size` — an absolute offset honoring the + /// schema's declared `disc.offset` displacement (which may be + /// nonzero, so the variant can start before or after a naive + /// shared-field walk would place it). + /// - **Field-name discriminator**: the offset after the whole + /// `shared` walk (the union's declared `fields`: the + /// discriminator field + any shared fields). Under the + /// shared-then-variant wire convention (ADR-011 addendum, + /// review #006 H3) the variant's own fields begin exactly + /// here; the variant does not re-declare shared fields, so + /// walking the variant schema at this offset reads its own + /// fields only. variant_start: usize, }, /// `array` — the consumer iterates `count` elements of diff --git a/src/tunion.rs b/src/tunion.rs index e11da40..2bd7a91 100644 --- a/src/tunion.rs +++ b/src/tunion.rs @@ -453,6 +453,9 @@ mod tests { #[test] fn read_field_discriminator_field_not_found_is_schema_error() { + // H3 enforcement: the missing-discriminator-field case is now + // rejected at parse (BastUnion::parse), before any dispatch + // reader can see the union. let root = json!({ "$defs": { "U": { @@ -463,10 +466,13 @@ mod tests { } } }); - let u = doc_union(&root, "U"); - let buf = [0u8; 4]; - let err = read_field_discriminator(&buf, &u, 0, LE).unwrap_err(); - assert!(matches!(err, AlkTypeError::Schema(_))); + let err = BastDoc::new(&root, "U").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("no field"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } } #[test] diff --git a/tests/poc_roundtrip.rs b/tests/poc_roundtrip.rs index 8fb78a8..6e5b593 100644 --- a/tests/poc_roundtrip.rs +++ b/tests/poc_roundtrip.rs @@ -603,4 +603,111 @@ fn tunion_byte_offset_discriminator_size_lookup() -> Result<(), AlkTypeError> { assert_eq!(tunion::discriminator_size(u16_union)?, 2); assert_eq!(tunion::discriminator_size(u32_union)?, 4); Ok(()) +} + +// --------------------------------------------------------------------------- +// L6 (review #006): the single roundtrip test through a field-disc union. +// Write with LayoutBuilder → read with SequentialReader → materialize → +// validate_bytes on the engine — one schema, all three packed-mode +// consumers, so the H3 wire-convention split can never reappear silently. +// +// Fixture shape: the union's `fields` carry the discriminator (`type`, +// uint8) *and* a second shared field (`seq`, uint32); the variant +// (`Read`) declares only its own field (`handle`) — it does NOT +// re-declare the discriminator or any shared field (forbidden since the +// ADR-011 addendum). The disc field is a uint8, so the mapping keys are +// stringified integers ("1" = read). The builder lays out +// shared-then-variant: +// type@0 (1B), seq@1 (4B), handle@5 (4B) — total 9. +// --------------------------------------------------------------------------- + +#[test] +fn field_disc_union_roundtrip_build_read_materialize_validate() -> Result<(), AlkTypeError> { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "endian": "little", + "fields": [ + { "name": "event", "kind": { "$ref": "#/$defs/Event" } }, + { "name": "trailer", "kind": "uint8" } + ] + }, + "Event": { + "kind": "union", + "discriminator": { "kind": "field", "name": "type" }, + "fields": [ + { "name": "type", "kind": "uint8" }, + { "name": "seq", "kind": "uint32" } + ], + "mapping": { + "1": { "$ref": "#/$defs/Read" }, + "2": { "$ref": "#/$defs/Write" } + } + }, + "Read": { + "kind": "struct", + "fields": [ { "name": "handle", "kind": "uint32" } ] + }, + "Write": { + "kind": "struct", + "fields": [ { "name": "handle", "kind": "uint32" } ] + } + } + }); + + // --- Write side: LayoutBuilder (shared-then-variant layout) -------- + let builder = LayoutBuilder::new(&root, "S")?; + let layout = builder.build(&var_sizes(&[("event.__variant", 0)]))?; + assert_eq!(layout.total_size(), 9 + 1, "shared(1+4) + variant(4) + trailer(1)"); + + let handle_pos = layout.get("event.handle").expect("event.handle"); + assert_eq!(handle_pos.offset, 5, "variant fields start after shared (type@0, seq@1)"); + let disc_pos = layout.get("event.type").expect("event.type"); + assert_eq!(disc_pos.offset, 0); + let seq_pos = layout.get("event.seq").expect("event.seq"); + assert_eq!(seq_pos.offset, 1); + + let mut buffer = vec![0u8; layout.total_size()]; + data_access::write_u8(&mut buffer, 0, 1, "event.type")?; // mapping key "1" + data_access::write_u32(&mut buffer, 1, 77, "event.seq", Endian::Little)?; + data_access::write_u32(&mut buffer, 5, 4242, "event.handle", Endian::Little)?; + data_access::write_u8(&mut buffer, 9, 55, "trailer")?; + + // --- Engine (packed) + reader + materializer + validator ----------- + let engine = AlkTypeEngine::compile(&root, "S", LayoutMode::Packed, None)?; + + // validate_bytes (materializer + ValidationPlan) accepts the buffer. + engine.validate_bytes(&buffer)?; + + // SequentialReader: the union field reports the disc value and the + // variant start (after the shared walk). + let mut reader = engine.sequential_reader().expect("packed mode has reader"); + let (name, value) = reader.read_next(&buffer)?.expect("event"); + assert_eq!(name, "event"); + let (disc, variant_start) = match &value { + FieldValue::Union { discriminator, variant_start } => (discriminator.clone(), *variant_start), + other => panic!("expected Union, got {other:?}"), + }; + assert_eq!(disc, "1", "uint8 disc value 1 stringifies to the mapping key"); + assert_eq!(variant_start, 5, "variant starts after the shared walk"); + let (name, value) = reader.read_next(&buffer)?.expect("trailer"); + assert_eq!(name, "trailer"); + assert_eq!(value, FieldValue::U8(55)); + + // materialize_packed through the engine's plan: shared fields land + // in the object, the variant's handle flattens in. + let plan = engine_sequential_plan(&root, "S")?; + let value = alktype::materialize::materialize_packed(&plan, &buffer)?; + assert_eq!(value["event"]["type"], json!(1)); + assert_eq!(value["event"]["seq"], json!(77)); + assert_eq!(value["event"]["handle"], json!(4242)); + assert_eq!(value["event"]["__discriminator"], json!("1")); + assert_eq!(value["trailer"], json!(55)); + + Ok(()) +} + +fn engine_sequential_plan(root: &serde_json::Value, name: &str) -> Result, AlkTypeError> { + Ok(std::sync::Arc::new(ReadPlan::compile(root, name)?)) } \ No newline at end of file