Resolve L2/L3: real schema threaded through ReadPlan compile; dead locals dropped (review #006)
L2: compile() builds Arc<Value> once and threads &Arc<Value> down the compile walk; every real ReadPlan carries the document at construction — the Value::Null-placeholder-then-map-overwrite dance is gone. The one remaining Null in wrap_leaf is documented as correct-by-construction (anonymous synthetic wrapper, never escapes). L3: materialize_plan_field drops its plan parameter (taken solely to discard) and the field-disc union arm's disc_field/let _ pair is deleted — the order-walk + by-name capture is the materializer's correct design, as the finding's parity note described. 508 tests green, clippy -D warnings clean, wasm32 build green, cargo doc zero warnings.
This commit is contained in:
1 parent
dcfe9d16ff
commit
8739d29550
3 files changed
+91
-41
No files matched your search
@@ -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<Value>` (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<Value>` once and threads `schema:
|
||||
&Arc<Value>` 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
|
||||
|
||||
|
||||
+5
-13
@@ -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<Value, AlkTypeError> {
|
||||
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 {
|
||||
|
||||
+55
-23
@@ -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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<ReadPlan, AlkTypeError> {
|
||||
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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<Vec<FieldPlan>, 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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<FieldPlan, AlkTypeError> {
|
||||
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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<(ReadKind, Option<CompositePlan>), 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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<(ReadKind, Option<CompositePlan>), 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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<(ReadKind, Option<CompositePlan>), 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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<CompositePlan, AlkTypeError> {
|
||||
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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<CompositePlan, AlkTypeError> {
|
||||
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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<CompositePlan, AlkTypeError> {
|
||||
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<String>,
|
||||
schema: &Arc<Value>,
|
||||
) -> Result<CompositePlan, AlkTypeError> {
|
||||
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],
|
||||
|
||||
Reference in new issue
Block a user