diff --git a/docs/reviews/006-implementation-review-030.md b/docs/reviews/006-implementation-review-030.md index 982baaf..fa7ff8f 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, H3, L5, L6, H2, M1, M2, M3 resolved 2026-09-02) +status: in-progress (H1, L1, H3, L5, L6, H2, M1, M2, M3, L2, L3 resolved 2026-09-02) last_updated: 2026-09-02 reviewed_artifacts: - src/read_plan.rs @@ -97,11 +97,11 @@ sub-types, `fingerprint()` methods; `Hash` on `Endian`/ |----------|------:|--------| | High | 3 (H1, H2, H3) | all resolved 2026-09-02 | | Medium | 4 (M1, M2, M3, M4) | M1, M2, M3 resolved 2026-09-02; M4 ongoing | -| Low | 6 (L1–L6) | L1, L5, L6 resolved 2026-09-02 | +| Low | 6 (L1–L6) | L1, L2, L3, L5, L6 resolved 2026-09-02 | | Nit | 2 (N1, N2) | open | -All three Highs and three of four Mediums are resolved. Remaining: -M4 (ongoing per-fix coverage posture), L2/L3/L4/N1/N2 opportunistic. +All three Highs, three of four Mediums, and five of six Lows are +resolved. Remaining: M4 (ongoing per-fix coverage posture), L4, N1, N2. The three Highs are adversarial-input crashes (H1, H2) and a cross-consumer wire-layout convention gap (H3) — all three are the @@ -137,6 +137,10 @@ them became materially easier to hit with the 0.3.0 surface. adjacent surfaces, per the recommended order) — see the resolution blocks on each finding. 508 tests green, clippy `-D warnings` clean, wasm build green, `cargo doc --no-deps` zero warnings. +- **L2 + L3 (2026-09-02):** resolved in one commit (both touch the + 0.3.0 compiled-form files already in play) — see the resolution + blocks on each finding. 508 tests green, clippy `-D warnings` clean, + wasm build green, `cargo doc --no-deps` zero warnings. --- @@ -787,6 +791,19 @@ adds a consumer mid-walk. Passing `schema: &Arc` (or the already-`Arc`'d clone) down through `compile_struct` removes the placeholder, the `map`, and the smell in one small change. +**Resolution (2026-09-02):** the recommended change, exactly. +`compile` builds the `Arc` once and threads `schema: +&Arc` down through `compile_struct`/`compile_field_list`/ +`compile_field`/`compile_typeref`/`compile_resolved`/`compile_def_kind`/ +`compile_union`/`compile_variant`/`compile_array`/`compile_record`; +every real `ReadPlan` carries `Arc::clone(schema)` at construction — +the placeholder-then-`map`-overwrite dance is gone. One +`Arc::new(Value::Null)` remains in `wrap_leaf`, now documented: the +wrapper is an anonymous synthetic node over a TypeRef (not a schema +struct), it never escapes (the materializer unwraps it; +`schema()` returns the root plan's doc), so `Null` is correct by +construction rather than a lie. No public-surface change. + ### L3. Dead locals left from the phase-2 rewrite **Files**: `src/materialize.rs:68` (`let _ = plan;` in @@ -805,6 +822,15 @@ and use it to assert the walk actually encountered the disc field (the current `disc_value.ok_or_else(...)` at :382-384 already does that job, so deletion is cleaner). +**Resolution (2026-09-02):** deletion taken on both counts. The `plan` +parameter is gone from `materialize_plan_field` (all four call sites +updated); the `disc_field`/`let _` pair is deleted, with +`DiscriminatorPlan::Field`'s `field_index` now matched as +`field_index: _` (the index is the *reader's* fast-path handle — +the materializer's order-walk + by-name capture is the correct +design here, exactly as the finding's parity note described). No +behavioral change; 508 tests green. + ### L4. `OffsetMap::get` / `PackedLayout::get` are linear scans positioned as random access **Files**: `src/offset_map.rs:154-159`, `src/layout_builder.rs:83-88` @@ -985,7 +1011,7 @@ Worth recording, because the findings shouldn't eclipse it: 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 done - with H1; L5/L6 done with H3). + with H1; L5/L6 done with H3; L2/L3 done with the M-fix session). ## Notes diff --git a/src/materialize.rs b/src/materialize.rs index 03422b1..f435c01 100644 --- a/src/materialize.rs +++ b/src/materialize.rs @@ -48,7 +48,7 @@ pub fn materialize_packed( let mut offset = 0usize; let mut obj = Map::new(); for field in plan.fields() { - let value = materialize_plan_field(plan, field, field.name(), buffer, &mut offset)?; + let value = materialize_plan_field(field, field.name(), buffer, &mut offset)?; obj.insert(field.name().to_string(), value); } Ok(Value::Object(obj)) @@ -59,13 +59,11 @@ pub fn materialize_packed( /// recursing into the compiled composite body. `endian` is the field's /// effective endianness baked into the plan. fn materialize_plan_field( - plan: &ReadPlan, field: &FieldPlan, field_path: &str, buffer: &[u8], offset: &mut usize, ) -> Result { - let _ = plan; match field.kind() { ReadKind::Primitive(kind) => { materialize_plan_primitive(*kind, field.endian(), field_path, buffer, offset) @@ -220,7 +218,7 @@ fn materialize_plan_struct( let mut obj = Map::new(); for field in plan.fields() { let path = format!("{field_path}.{}", field.name()); - let value = materialize_plan_field(plan, field, &path, buffer, offset)?; + let value = materialize_plan_field(field, &path, buffer, offset)?; obj.insert(field.name().to_string(), value); } Ok(Value::Object(obj)) @@ -276,7 +274,7 @@ fn materialize_plan_composite( && only.name().is_empty() && matches!(only.kind(), ReadKind::Primitive(_) | ReadKind::Enum) { - return materialize_plan_field(plan, only, field_path, buffer, offset); + return materialize_plan_field(only, field_path, buffer, offset); } } } @@ -353,16 +351,10 @@ fn materialize_plan_union( materialize_plan_composite(variant, field_path, endian, buffer, offset)?; Ok(tag_union_value_number(disc_value, variant_value)) } - DiscriminatorPlan::Field { name, field_index } => { + DiscriminatorPlan::Field { name, field_index: _ } => { let shared_plan = shared.as_ref().ok_or_else(|| AlkTypeError::Schema(format!( "internal: field-disc union at {field_path} has no shared plan" )))?; - let disc_field = shared_plan.fields().get(*field_index).ok_or_else(|| { - AlkTypeError::Schema(format!( - "internal: union {field_path} discriminator index {field_index} out of range" - )) - })?; - let _ = disc_field; // Walk the whole shared plan; capture the discriminator // field's value for the mapping key as we pass it. The // result object preserves the 0.2.0 key order: @@ -372,7 +364,7 @@ fn materialize_plan_union( let mut shared_values: Vec<(String, Value)> = Vec::new(); for field in shared_plan.fields() { let path = format!("{field_path}.{}", field.name()); - let value = materialize_plan_field(shared_plan, field, &path, buffer, offset)?; + let value = materialize_plan_field(field, &path, buffer, offset)?; if field.name() == name.as_str() { disc_value = Some(value); } else { diff --git a/src/read_plan.rs b/src/read_plan.rs index 00e90e6..5bf0ceb 100644 --- a/src/read_plan.rs +++ b/src/read_plan.rs @@ -175,10 +175,16 @@ impl ReadPlan { } }; let mut seen = BTreeSet::new(); - compile_struct(&doc, struct_node, struct_node.endian(), "", 0, &mut seen).map(|mut plan| { - plan.schema = Arc::new(bast_doc.clone()); - plan - }) + let schema = Arc::new(bast_doc.clone()); + compile_struct( + &doc, + struct_node, + struct_node.endian(), + "", + 0, + &mut seen, + &schema, + ) } /// The root container default endianness. @@ -281,11 +287,12 @@ fn compile_struct( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result { if depth > MAX_COMPILE_DEPTH { return Err(depth_err(path)); } - let fields = compile_field_list(doc, s.fields(), container_endian, path, depth, seen)?; + let fields = compile_field_list(doc, s.fields(), container_endian, path, depth, seen, schema)?; let mut by_name = BTreeMap::new(); for (i, field) in fields.iter().enumerate() { by_name.entry(field.name().to_string()).or_insert(i); @@ -294,7 +301,7 @@ fn compile_struct( endian: container_endian, fields, by_name, - schema: Arc::new(Value::Null), + schema: Arc::clone(schema), }) } @@ -305,11 +312,12 @@ fn compile_field_list( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result, AlkTypeError> { let mut out = Vec::with_capacity(fields.len()); for field in fields { let field_path = dotted(path, field.name()); - out.push(compile_field(doc, field, container_endian, &field_path, depth, seen)?); + out.push(compile_field(doc, field, container_endian, &field_path, depth, seen, schema)?); } Ok(out) } @@ -321,9 +329,10 @@ fn compile_field( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result { let endian = field.effective_endian(container_endian); - let (kind, body) = compile_typeref(doc, field.ty(), endian, path, depth, seen)?; + let (kind, body) = compile_typeref(doc, field.ty(), endian, path, depth, seen, schema)?; Ok(FieldPlan { name: field.name().to_string(), kind, @@ -344,6 +353,7 @@ fn compile_typeref( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result<(ReadKind, Option), AlkTypeError> { match ty { BastType::Ref(r) => { @@ -352,11 +362,11 @@ fn compile_typeref( return Err(cycle_err(name, path)); } let def = doc.resolve_ref(r)?; - let out = compile_def_kind(doc, def.kind(), container_endian, path, depth + 1, seen)?; + let out = compile_def_kind(doc, def.kind(), container_endian, path, depth + 1, seen, schema)?; seen.remove(name); Ok(out) } - other => compile_resolved(doc, other, container_endian, path, depth, seen), + other => compile_resolved(doc, other, container_endian, path, depth, seen, schema), } } @@ -368,24 +378,25 @@ fn compile_resolved( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result<(ReadKind, Option), AlkTypeError> { match ty { BastType::Primitive(k) => Ok((ReadKind::Primitive(*k), None)), BastType::Enum(_) => Ok((ReadKind::Enum, None)), BastType::Struct(s) => { - let plan = compile_struct(doc, s, container_endian, path, depth + 1, seen)?; + let plan = compile_struct(doc, s, container_endian, path, depth + 1, seen, schema)?; Ok((ReadKind::Struct, Some(CompositePlan::Struct(plan)))) } BastType::Union(u) => { - let plan = compile_union(doc, u, container_endian, path, depth + 1, seen)?; + let plan = compile_union(doc, u, container_endian, path, depth + 1, seen, schema)?; Ok((ReadKind::Union, Some(plan))) } BastType::Array(a) => { - let plan = compile_array(doc, a, container_endian, path, depth + 1, seen)?; + let plan = compile_array(doc, a, container_endian, path, depth + 1, seen, schema)?; Ok((ReadKind::Array, Some(plan))) } BastType::Record(r) => { - let plan = compile_record(doc, r, container_endian, path, depth + 1, seen)?; + let plan = compile_record(doc, r, container_endian, path, depth + 1, seen, schema)?; Ok((ReadKind::Record, Some(plan))) } BastType::Ref(_) => Err(AlkTypeError::Schema(format!( @@ -403,14 +414,15 @@ fn compile_def_kind( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result<(ReadKind, Option), AlkTypeError> { match kind { BastDefKind::Struct(s) => { - let plan = compile_struct(doc, s, container_endian, path, depth, seen)?; + let plan = compile_struct(doc, s, container_endian, path, depth, seen, schema)?; Ok((ReadKind::Struct, Some(CompositePlan::Struct(plan)))) } BastDefKind::Union(u) => { - let plan = compile_union(doc, u, container_endian, path, depth, seen)?; + let plan = compile_union(doc, u, container_endian, path, depth, seen, schema)?; Ok((ReadKind::Union, Some(plan))) } BastDefKind::Enum(_) => Ok((ReadKind::Enum, None)), @@ -424,6 +436,7 @@ fn compile_union( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result { if depth > MAX_COMPILE_DEPTH { return Err(depth_err(path)); @@ -450,7 +463,8 @@ fn compile_union( let shared = match u.discriminator() { BastDiscriminator::Byte { .. } => None, BastDiscriminator::Field { .. } => { - let plan = compile_field_list(doc, u.fields(), container_endian, path, depth, seen)?; + let plan = + compile_field_list(doc, u.fields(), container_endian, path, depth, seen, schema)?; let mut by_name = BTreeMap::new(); for (i, field) in plan.iter().enumerate() { by_name.entry(field.name().to_string()).or_insert(i); @@ -459,14 +473,15 @@ fn compile_union( endian: container_endian, fields: plan, by_name, - schema: Arc::new(Value::Null), + schema: Arc::clone(schema), })) } }; let mut variants = Vec::with_capacity(u.mapping().len()); for (key, variant_ty) in u.mapping() { let variant_path = format!("{path}.mapping[{key}]"); - let body = compile_variant(doc, variant_ty, container_endian, &variant_path, depth, seen)?; + let body = + compile_variant(doc, variant_ty, container_endian, &variant_path, depth, seen, schema)?; variants.push(((*key).to_string(), body)); } Ok(CompositePlan::Union { @@ -486,6 +501,7 @@ fn compile_variant( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result { if depth > MAX_COMPILE_DEPTH { return Err(depth_err(path)); @@ -505,8 +521,11 @@ fn compile_variant( path, depth + 1, seen, + schema, )?), - BastDefKind::Union(u) => compile_union(doc, u, container_endian, path, depth + 1, seen)?, + BastDefKind::Union(u) => { + compile_union(doc, u, container_endian, path, depth + 1, seen, schema)? + } BastDefKind::Enum(_) => { return Err(AlkTypeError::Schema(format!( "read_plan: union variant at {path} must be a struct or union, got enum" @@ -523,8 +542,11 @@ fn compile_variant( path, depth + 1, seen, + schema, )?)), - BastType::Union(u) => compile_union(doc, u, container_endian, path, depth + 1, seen), + BastType::Union(u) => { + compile_union(doc, u, container_endian, path, depth + 1, seen, schema) + } other => Err(AlkTypeError::Schema(format!( "read_plan: union variant at {path} must be a struct or union, got {kind}", kind = other.alk_kind() @@ -539,12 +561,14 @@ fn compile_array( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result { if depth > MAX_COMPILE_DEPTH { return Err(depth_err(path)); } let element_path = format!("{path}.element"); - let (kind, body) = compile_typeref(doc, a.element(), container_endian, &element_path, depth, seen)?; + let (kind, body) = + compile_typeref(doc, a.element(), container_endian, &element_path, depth, seen, schema)?; let element = wrap_leaf(kind, body, container_endian, &element_path)?; let element_stride = fixed_composite_size(&element)?.unwrap_or(0); let count = a.count(); @@ -573,12 +597,14 @@ fn compile_record( path: &str, depth: usize, seen: &mut BTreeSet, + schema: &Arc, ) -> Result { if depth > MAX_COMPILE_DEPTH { return Err(depth_err(path)); } let values_path = format!("{path}.values"); - let (kind, body) = compile_typeref(doc, r.values(), container_endian, &values_path, depth, seen)?; + let (kind, body) = + compile_typeref(doc, r.values(), container_endian, &values_path, depth, seen, schema)?; let value = wrap_leaf(kind, body, container_endian, &values_path)?; Ok(CompositePlan::Record { value: Box::new(value), @@ -608,6 +634,12 @@ fn wrap_leaf( max_length: None, body: None, }; + // The wrapper is an anonymous synthetic node, not a + // schema struct: it carries no document (its single + // field is a TypeRef, not a named field), so `schema` + // is `Value::Null` by construction. It never escapes — + // `materialize_plan_composite` unwraps it and the + // public `schema()` accessor returns the root plan's. Ok(CompositePlan::Struct(ReadPlan { endian: container_endian, fields: vec![field],