Let a value form carry an Option<sum> field (#220)

[milyin]

Closes #220.

The asymmetry

Option<sum> was refused as a value-form field while the same field on a data class worked — the fromParts bridge emits a <field>__present flag beside its <field>__tag (PlanFieldKind::Sum { optional }). Same shape, one position over, different answer.

Rejecting was right when #213 landed: without gating, the emitter composes match &(&__vf).result on an Option<Sum> against bare variant patterns — E0308 in the consumer crate. A named error beat emitting that.

The issue's premise is out of date

#220 proposes teaching core a present-flag notion on the output side, dual to FlatLeaf::is_present_flag. Core does not need one. The conditional value form work already built absence into the leaf model, and it covers this case unchanged — prebindgen-registry/src/unfold.rs::flatten today already:

• marks the field's last path step optional — PathStep::field(last, opt && decomposed), and decomposed is true for a sum (FieldDecon::Leaves);
• ORs the field's own opt into every synthesized leaf's nullable.

And everything downstream of that flag is already correct: the tag boxes so JVM null cannot alias tag 0 (leaf_is_prim), leaf_slot knows each slot's wire default, iface.rs types the selector Int? and the group nullable, and kotlin_emit.rs puts null -> null ahead of the real tags. All of it JVM-tested today through ledgerEach.

So this PR changes no core, no iface.rs, no kotlin_emit.rs.

The one function

delivery.rs's sum-segment loop folded the selector's path with fold_steps, which has no optional handling — and said so:

// no step on it is optional, since an optional sum is refused where the leaves are built.

A sum segment can't take the per-leaf treatment reach_leaf gives an ordinary optional field: its leaves aren't independent, only one group is live. So the whole segment gates as one tuple bind whose absent arm carries every slot's default — the shape a conditional hoist already emits, applied to an optional step inside the segment's own path:

let (__cb0_obj1, __cb0_obj2, __cb0_obj3): (JObject, JObject, JObject) = {
    let __so1: &::core::option::Option<_> = &(&__vf0).outcome;   // coerced, #268
    match __so1 {
        Some(__sg1) => { /* today's encode_sum_group, unchanged */ (…) }
        None => (JObject::null(), JObject::null(), JObject::null()),
    }
};

encode_sum_group is reused untouched. The optional step follows reach_leaf's own division: a field step goes through bind_as_option so the destructure doesn't care how the source spelled the optional (#268); a call step keeps its direct match, since an owned position can't be coerced that way.

Vec<sum> stays refused — variable arity genuinely has no fixed layout.

Coverage

• a_sum_field_behind_option_or_vec_is_rejected_by_name → a_vec_sum_field_is_rejected_by_name; the Option row and its rationale are gone.

• New an_optional_sum_field_gates_its_whole_segment — the coercion site, the tuple-bound absent arm, the tag defaulting to JVM null rather than a raw jint, and Kotlin's null -> null.

• New a_bare_sum_field_takes_no_gate — the dual, so the optional path isn't entered when there's nothing to gate.

• examples/covertest-kotlin gains Probe, whose value form has an Option<Lookup> field. The JVM harness pins the distinction the boxing exists for:

  probeNew(9, -2, …) { seq, tag, _, _ -> "$seq:$tag" }  // "9:null"  — no sum at all
  probeNew(9,  0, …) { seq, tag, _, _ -> "$seq:$tag" }  // "9:0"     — a PRESENT Lookup.Absent

  A raw jint selector could not tell those apart, which is the whole reason no present flag is needed. 50 sections pass under ./gradlew run.

  Deliberately kept off ReportStruct: Report is embedded twice in Ledger, so a field there would add 3 positional slots to reportEach and 6 to ledgerEach and bury the change under signature churn.

Verified

cargo test --all; clippy -D warnings and cargo fmt --check (CI's exact config) on 1.85.0 and stable; examples/regen-check.sh clean. Generated output is additive only — 866 insertions, no deletions, no existing declaration touched — and regen-check.sh --with-zenoh-flat-jni is byte-identical, since zenoh-flat's ReplyStruct.result is a bare ReplyResult.

docs/sum-types.md §4.3 now records that both output paths gate Option<sum>, by two different means — a present flag on the fromParts bridge, the selector's own nullability in a leaf list.

[Copilot]

Pull request overview

This PR removes the previous restriction that refused Option<sum> when used as a value-form field in the JNI generator, by teaching the Rust-side delivery emitter to gate an entire decomposed-sum segment as a unit when an optional step appears in that segment’s access path.

Changes:

• Update encode_plan_leaves sum-segment emission to detect an optional path step and wrap the whole sum segment in a single Option::{Some,None} gate, defaulting all slots together in the None arm.
• Remove the declaration-time rejection for Option<sum> value-form fields (while keeping Vec<sum> rejected).
• Add focused regression tests plus Kotlin/Rust covertest coverage via a new Probe handle/value-form with an Option<Lookup> field, and document the output-path behavior.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
prebindgen-jni/src/jni/tests/value_form.rs	Reworks the prior rejection test to keep only Vec<sum> rejection and adds new tests asserting gated vs ungated sum-segment emission for Option<sum> vs bare sum.
prebindgen-jni/src/jni/emit/delivery.rs	Implements optional-step detection in the sum-segment loop and emits a tuple-bind gate so Option<sum> defaults the entire segment together.
prebindgen-jni/src/jni/builder.rs	Removes the assert that rejected Option<sum> for value-form fields; keeps Vec<sum> refusal rationale.
examples/perftest-flat/src/ext.rs	Adds Probe and related APIs to exercise a value-form field outcome: Option<Lookup> across absent and present sum cases.
examples/covertest-kotlin/src/generated_bindings.rs	Regenerates Rust JNI bindings to include Probe handle support and callback/builder delivery for the new value-form shape.
examples/covertest-kotlin/kotlin/REPORT.md	Updates the generated Kotlin report with probe_each/probe_new and the decomposed leaf shape.
examples/covertest-kotlin/kotlin/io/prebindgen/covertest/Test.kt	Adds a runtime Kotlin section that distinguishes “absent sum” (null) from Lookup.Absent (tag 0) to validate selector boxing semantics.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/model.kt	Regenerates Kotlin model to include Probe, its callbacks/builders, and nullable selector mapping for the optional sum field.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest.kt	Regenerates Kotlin native interface declarations for probeEach/probeNew.
examples/covertest-kotlin/build.rs	Extends covertest binding generation inputs to include Probe and its value-form expansion.
docs/sum-types.md	Documents that Option<sum> is supported in both output paths (fromParts present-flag vs value-form selector nullability) and that Vec<sum> remains refused.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[milyin]

This negative assertion is vacuous — the test cannot fail.

prettyplease renders a tuple-valued match arm as ::core::option::Option::None => { + newline + (, never None => ( on one line. I verified this empirically: dropping this exact assertion into an_optional_sum_field_gates_its_whole_segment (which does emit the gate) makes it pass there too.

So the dual test the PR advertises as pinning "the optional path isn't entered when there's nothing to gate" would stay green even if the gate were emitted for a bare sum field. Only the !kotlin.contains("null -> null") check below carries weight.

The sibling test already gets this right by asserting on the whitespace-stripped text (rc); this one should do the same.

Suggested change

	!rust.contains("::core::option::Option::None => ("),
	!rust.split_whitespace().collect::<String>().contains("::core::option::Option::None=>{("),

[milyin]

Fixed in 7925993, with the whitespace-stripped form.

One thing worth recording, because it changes what the test is worth: the assertion is now able to fail — the needle ::core::option::Option::None=>{( is exactly what the sibling test asserts is present in the gated emission — but I could not construct a mutation that makes it fail in this fixture. Forcing the predicate (find(|_| true)) leaves the test green, because for a bare sum field project_leading_fields consumes the result step as a plain field, so lead == path.len() and the search range is empty before the predicate is ever consulted.

So the dual’s real guard against the gate firing is structural rather than asserted, and only the Kotlin null -> null check is falsifiable by touching the gate. The fix is still worth having — it is now a claim about the output instead of a claim about prettyplease’s line breaking — but it should not be read as pinning more than it does.

[milyin]

Stale comment directly above this line now contradicts the code (lines 951-953, just outside the hunk):

// The value to match on. The selector's own path reaches the sum
// (empty when the sum IS the value); no step on it is optional, since
// an optional sum is refused where the leaves are built.

That is exactly the invariant this PR deletes in builder.rs, and the very next statement (let opt_at = …) exists because a step on it can now be optional. In a codebase where these comments are the primary record of why the emitter is shaped the way it is, leaving this one in place will send the next reader looking for a refusal that is gone.

[milyin]

Fixed in 7925993. It now reads:

The value to match on. The selector's own path reaches the sum (empty when the sum IS the value), and a step on it MAY be optional: the refusal that used to guarantee otherwise is gone (#220), which is what the gate below exists for.

Agreed on the reasoning — a comment asserting a refusal that the same PR deletes is worse than no comment, because it reads as a reason not to look.

[milyin]

Two things about the tail after the gated step, both latent today but worth stating since the removed builder.rs assert was the only named diagnostic in this area:

1. owned = true disagrees with reach_leaf. In the FIELD branch below, #bind comes from matching a &::core::option::Option<_>, so match ergonomics binds it as &Sum — a borrow, not an owned value. reach_leaf passes false for its analogous recursion (reach_leaf(.., quote!(#nested), /*base_is_ref=*/true, ..) → fold_steps(.., false)). With true, a Call step in path[k + 1..] would be composed as &(&Sum) and hand the accessor &&Sum → E0308 in the consumer crate. true is only right for the CALL branch (owned Option<T> scrutinee); one flag is serving two branches that disagree.

2. opt_at finds only the first optional step, and fold_steps has no optional handling, so a second optional step in the tail would compose match &Option<..> against bare variant patterns — the same E0308 the deleted assert was written to pre-empt, now with no named error.

Both are unreachable right now only because synth_sum_leaves gives every sum leaf an empty path, so path[k + 1..] is always empty. That invariant isn't stated anywhere here. A debug_assert!(path[k + 1..].is_empty(), ..) (or just passing path[k].yields_owned() instead of the literal true) would keep this honest.

[milyin]

Both taken in 7925993, and your framing of (1) is better than mine — I had flagged the same flag as "inert today", but reach_leaf passing false for the analogous recursion is the argument that settles it: two branches were sharing a literal that only one of them makes true.

Fixed as path[k].yields_owned() rather than a branch on path[k].is_field(). yields_owned is Call { owned: true } and nothing else, so it answers correctly for both arms from the step itself — the FIELD arm gets false (the &Option<_> scrutinee binds &Sum) and an owned-yielding CALL gets true — and there is no second place stating which arm is which.

For (2) I took the debug_assert!(path[k + 1..].is_empty(), ..) over my own suggestion of asserting no second optional step: emptiness is the invariant that actually holds and it subsumes the other. It names the E0308 the deleted builder.rs assert used to pre-empt, so the diagnostic that left builder.rs has a home. 253 lib tests, both Kotlin examples and emitcheck run without it firing.

Independently of the reachability question, I probed the shape that would put a second optional step in one path — Option<Child> where Child's own value form holds Option<sum> — through emitcheck. It is refused upstream by name ("a value form nested under another one that is reached through Option — conditional hoists do not nest"), so the assert documents an invariant that two separate mechanisms currently keep.

[milyin]

Review

Merged main in (c07ce87) — details at the end. All six jobs green on the merge.

The core argument holds up, and it is the good one: #220 asks for a present flag and the answer is that the conditional-value-form work already paid for absence, so the diff is one function and no core, no iface.rs, no kotlin_emit.rs. Gating the whole segment rather than each slot is right for the stated reason — a sum's leaves are not independent — and reusing encode_sum_group untouched is what keeps the live path provably unchanged. The field / call split following reach_leaf's own division (rather than restating it) is the detail I'd have most expected to go wrong, and it doesn't.

Verified locally: cargo test --all, clippy -D warnings + fmt --check (CI's config) on 1.85.0 and stable, regen-check.sh clean, cargo doc clean, and covertest-kotlin$ ./gradlew run → 52 sections post-merge.

What I went after, and what it found

The one thing that worried me is that the gate handles the first optional step and folds the remainder with fold_steps, which still has no optional handling. So: can a sum leaf's path carry two optional steps? I pushed on it with emitcheck (#382), which is exactly the right instrument since this shape's emitted Rust is otherwise only judged as text.

Three routes, all resolved:

1. Option<sum> directly — compiles. (Also covertest's Probe.)

2. Option<DataClass> where the data class holds Option<sum> — compiles; it crosses the fromParts bridge, whose PlanFieldKind::Sum { optional } already handled it, so it never reaches the segment loop.

3. Option<Child> where Child's own value form holds Option<sum> — the shape that would genuinely stack two optional steps in one path. Refused upstream, by name:

   output expansion: z_child_to_struct not yet supported: a value form nested under another one that is reached through Option — conditional hoists do not nest

So the gap I was looking for is closed, and project_leading_fields' is_plain_field() (which stops at the first optional) means opt_at starting at lead cannot skip one either. No finding.

Two notes, both "say it", not "fix it"

1. The remainder's triviality is load-bearing and unstated. opt_at takes the first optional and fold_steps(&path[k+1..], …) assumes nothing after it is optional. That is true today only because of the nesting refusal above — a guard in a different file, phrased about a different thing (hoists nesting), which nobody editing this loop would think to check. A debug_assert!(!path[k + 1..].iter().any(PathStep::is_optional)) naming that refusal would cost one line and make the coupling visible where it is relied on. This is the same class of thing the review of #386 asked me for, so it seems fair to ask it back.

2. owned: true on the inner fold is only right for one of the two arms. fold_steps(&qualify, &path[k + 1..], quote!(#bind), true) — in the is_field() branch the scrutinee is &Option<_> (that's what bind_as_option is for), so match ergonomics binds __sgN as &T, not an owned value. It is inert today (path[k + 1..] is empty for every shape that gets here, and even non-empty it would only add a &&T → &T coercion), but it reads as a fact about the binding and the fact is wrong in that arm. Either path[k].is_field()-conditional, or a comment saying why the conservative value is safe.

Neither blocks. Both are about the same thing: this function now has an invariant it does not state.

Smaller things

• The assert! you removed took the Vec and Option rows together; the surviving Vec message is still accurate on its own, and the comment above it now explains why Option left. Good — that comment is the one a future reader needs.
• a_bare_sum_field_takes_no_gate as the dual of the new gate test is the right pairing: it pins that the optional path is not entered when there is nothing to gate, which is what a "find the first optional" implementation could plausibly get wrong.
• Keeping Probe off ReportStruct was the right call — Ledger embeds Report twice, so it would have added 3 slots to reportEach and 6 to ledgerEach and buried the change.
• The "9:null" vs "9:0" pair in the harness is the assertion that actually earns the boxing. Worth keeping visible in the KDoc if that ever gets rewritten.

The merge

One conflict, entirely generated: covertest/model.kt, where this branch's Probe handle class lands next to #218's Verdict cascade — adjacent output, both wanted. Not hand-merged: the file is regenerated, and every other generated file with it (covertest.kt, both generated_bindings.rs, and the new emitcheck one from #382). regen-check.sh is byte-identical afterwards, and the merged tree passes the JVM harness, so the two features compose rather than merely co-exist.

[milyin]

@copilot resolve the merge conflicts in this pull request

[Copilot]

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

[milyin]

Both halves taken, in b2b3e0c.

assert!, not debug_assert! — and the argument is stronger than "invariants should be enforced": a build script inherits the consumer's profile, so debug_assert! is absent from exactly the release build of a downstream crate where a mis-emission is most expensive to diagnose. That defeats the reason I gave for adding it — the deleted builder.rs assert was the only named diagnostic here, and a debug-only replacement hands back a bare E0308 in generated code the author did not write. It now matches reach_leaf's own optional-step assert a few hundred lines up, message shape included.

The condition — agreed, with the reasoning worth stating because it cuts against my own commit message. Emptiness is true today (a sum leaf's path stops at the sum) but it is not the safety condition: a non-optional tail composes correctly through fold_steps, the more so now that the fold takes path[k].yields_owned() rather than a literal. Asserting emptiness would refuse a shape that works. A second optional step is the one that composes match &Option<..> against bare variant patterns, so that is what is asserted:

assert!(
    !path[k + 1..].iter().any(PathStep::is_optional),
    "jnigen unfold: leaf `{}` reaches its sum through TWO optional steps — the \
     segment gate has one `None` arm, so the second would be matched as if it \
     were the sum itself",
    leaf.name,
);

For the record on reachability: the shape that would trip it — Option<Child> where Child's own value form holds Option<sum> — is refused upstream by name ("a value form nested under another one that is reached through Option — conditional hoists do not nest"), measured through emitcheck. So this assert is a second lock on a door that is already shut, which is the right weight for it — not a load-bearing guard, but the named error if the first lock ever opens.

255 lib tests, regen-check.sh byte-identical, clippy -D warnings + fmt --check on 1.85.0 and stable.

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into this branch and fixing the conflicts in the merge commit 058e4a4.