Close a handle by where it sits, not by how the field is spelled (#218)

[milyin]

Closes #218.

The asymmetry

A native handle buried in another value had three different owners, decided only by its Rust spelling:

the handle sits in…	data-class field	callback argument
the field/arg directly	container's close()	proxy, after run()
an enum payload (sealed_class)	nobody	receiver, by hand
a nested data class	nobody	nobody

So swapping a field's type from a handle to an enum carrying that handle silently moved the free onto the consumer, with nothing in the generated Kotlin saying so. PlanFieldKind::destructible() matched only a Projection; everything else fell through to _ => None.

#218 offers "document it" or "cascade" and leans toward documenting. The decision here is cascade, on the grounds that the field's type is an implementation detail: replacing a struct with an enum must not change who frees the handle. That argument also settles the nested-data-class row, which is the same match arm and had no issue of its own.

The shape of the fix

Not "teach each use site to walk into wrappers". Instead the generated type becomes closeable, so every position keeps calling plain close():

public sealed interface Lookup : AutoCloseable {
    public data object Absent : Lookup { override fun close() {} }
    public data class Found(public val v0: Summary) : Lookup { override fun close() { v0.close() } }
    public data class Failed(public val v0: String) : Lookup { override fun close() {} }
}

The when over the alternatives is emitted once, in the sum. A data-class field then cascades with the same one-liner a handle field gets — no when at the container:

public data class Verdict(val id: Long, val outcome: Lookup) : AutoCloseable {
    override fun close() { outcome.close() }
}

…and the callback proxy needed only to be told the reassembled arg is owned; the close-unless-taken finally already existed for bare handle args.

One rule, asked two ways over the same tree: PlanFieldKind::destructible recurses through Sum/Nested for an already-classified field; type_close_strategy answers for a bare type where the caller holds no plan (the sealed emitter, the callback builder). Both refuse a borrowed handle — not the holder's to release — so a fold over a borrowed run emits no per-iteration close that would double-free.

⚠️ Breaking

A sum-carried handle delivered to a callback is now closed when run returns. A body meaning to outlive the call must take() it — exactly the contract a plain impl Fn(Handle) arg has always had. docs/sum-types.md §4.5 is rewritten accordingly: the rule is no longer "the receiver closes, in both positions" but "closed exactly as a bare handle in the same position", across all three.

Downstream impact today: none. zenoh-flat-jni's only two sums (RecoveryMode, InstrumentationTimestamp) carry ULong/ByteArray/Timestamp (a data_class!) — no handles. Confirmed by regen-check.sh --with-zenoh-flat-jni: its generated output is byte-identical. It will apply to ReplyResult, which is what prompted the issue.

Coverage

• sealed.rs::a_data_class_field_may_be_a_sum_carrying_a_handle — its negative assertion (and doc-comment rationale) flipped to the cascade.
• New a_data_class_field_may_be_a_nested_data_class_carrying_a_handle — the previously unfiled row. Verified as a real guard: reverting the Nested arm fails it.
• New a_sum_owning_nothing_native_is_not_closeable — the predicate must not over-fire; a sum of i64 payloads stays a plain sealed interface.
• two_sum_callback_args_keep_their_own_selectors — now also pins that exactly the handle-carrying sum binds a local and closes, and the other does not.
• examples/covertest-kotlin gains Verdict (sum in data-class-field position, beside Holder's plain handle field). Its JVM harness asserts the cascade, the post-run close, and take() keeping the payload alive — 50 sections pass under ./gradlew run, which is what proves close-unless-taken at runtime rather than just in the emission.

Also

The owned path would otherwise have wrecked the emitted formatting — a reassembly is one piece of raw text, and wline width-broke it mid-when into something valid but unreadable. It now binds to a val, and lambda params stay one-per-line as in the common path, so the golden diff reads as the behaviour change alone.

Verified

cargo test --all; cargo clippy --all-targets --all-features -- -D warnings and cargo fmt --check (CI's exact config) on 1.85.0 and stable; examples/regen-check.sh clean; examples/regen-check.sh --with-zenoh-flat-jni byte-identical; covertest-kotlin$ ./gradlew run PASS.

[Copilot]

Pull request overview

This PR fixes Kotlin/JNI handle ownership asymmetry by making generated Kotlin types (sealed interface sums and nested data_classes) AutoCloseable when they reach an owned native handle, so callers always just call .close() in every position (field, callback arg, etc.), independent of whether the handle is spelled directly or wrapped.

Changes:

• Extend close-strategy detection to recurse through Sum and Nested so containers cascade close via a one-line field.close()/field?.close().
• Emit AutoCloseable + per-variant close() overrides for handle-reaching sealed sums, and close reassembled callback args in finally (close-unless-taken), matching bare-handle behavior.
• Update docs and regression tests, and add a Kotlin/JVM runtime harness case (Verdict) to pin the new ownership behavior.

Reviewed changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
prebindgen-jni/src/jni/tests/sealed.rs	Updates/extends tests to assert cascade + sum closeability, adds nested-data-class and “sum owning nothing native” guards, and pins callback proxy close behavior.
prebindgen-jni/src/jni/struct_plan.rs	Makes PlanFieldKind::destructible() recurse through Sum/Nested and adds type_close_strategy() for plan-less close decisions (sealed emitter + callback builder).
prebindgen-jni/src/jni/kotlin_emit.rs	Emits AutoCloseable sealed sums when any alternative reaches an owned handle; generates per-variant close() bodies.
prebindgen-jni/src/jni/iface.rs	Tracks ownership for reassembled typed groups and closes owned reassemblies in finally; improves formatting stability by binding reassemblies to locals when needed.
examples/perftest-flat/src/ext.rs	Updates callback-doc comment to reflect close-after-run behavior; adds Verdict example exercising data-class-field sum cascade.
examples/covertest-kotlin/src/generated_bindings.rs	Regenerates bindings to include new Verdict bridge functions and native entrypoint.
examples/covertest-kotlin/kotlin/REPORT.md	Adds verdict_new and Verdict entries to the coverage report.
examples/covertest-kotlin/kotlin/io/prebindgen/covertest/Test.kt	Updates runtime assertions for callback-delivered sums (closed after run, take() escapes) and adds a Verdict cascade section.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/model.kt	Regenerated Kotlin showing Lookup : AutoCloseable, per-variant close(), callback proxy finally { close() }, and Verdict : AutoCloseable.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest.kt	Regenerated callback proxy code closing optional sums via ?.close() and adds CovNative.verdictNew.
examples/covertest-kotlin/build.rs	Updates test package registration/comments to include Verdict and document new closeability behavior.
docs/sum-types.md	Rewrites §4.5 ownership rule to “close like a bare handle in the same position” and documents the three positions (return/callback/field).

---

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

[milyin]

Review

The direction is right and the framing is the good one: making the type closeable so the when is emitted once, rather than teaching each use site to walk into wrappers, is what keeps Verdict.close() a one-liner and what makes the three positions actually agree. The data object Absent { override fun close() {} } shape reads well, and pinning escaped[0].isClosed() and the take() path in the same section is the right pair of assertions.

Verified locally on pr-386: cargo test -p prebindgen-jni 248 pass, clippy --all-targets --all-features clean, cargo fmt --check clean, cargo doc produces no new warnings.

Points below, roughly in descending order of how much I'd want them addressed before merge.

---

1. The nested-data-class row has no covertest exercise

The PR fixes three rows and the body says so, but examples/covertest-kotlin only gains Verdict — the sum in field position. The PlanFieldKind::Nested arm (struct_plan.rs:503) is covered only by a_data_class_field_may_be_a_nested_data_class_carrying_a_handle, an emission test.

That's the row with the most machinery behind it that nobody has run: the cascade emits a plain field.close() and assumes the inner data class was independently rendered AutoCloseable by render.rs's own destructible_fields pass. Nothing in this PR ties those two decisions together — an emission test asserting the outer close() body will pass whether or not the inner type actually implements AutoCloseable, because the outer test never compiles the inner class. ./gradlew run would.

Per the workspace's own rule (every JniGen feature gets a covertest ext.rs + build.rs + Test.kt exercise in the same PR, lib tests insufficient), I think this needs a nested-handle struct beside Verdict — a data_class whose field is a data_class carrying a Summary — and a section asserting the two-level cascade.

2. "The two forms answer alike by construction" is the claim I'd most like pinned by a test

type_close_strategy (struct_plan.rs:559) and PlanFieldKind::destructible (struct_plan.rs:491) are two independent walks over the same tree, and the doc-comment's "by construction" is doing a lot of work. Two places they are structurally not the same walk:

• Ordering. classify_field classifies Sum first, deliberately, before it will touch output_entry — its comment explains why. type_close_strategy asks output_entry(...).metadata.projection first and only then falls to type_kind. Today that's safe because sums carry no converter, but it's an unstated precondition, not a construction.
• Totality. build_struct_plan propagates None from any field classify_field refuses, so the whole struct has no plan. type_close_strategy walks st.fields from the flat model and refuses nothing — which is exactly the advertised design. So for any subtree where classification fails or is rejected, the two answer differently by construction rather than alike.

A disagreement in one direction is a Kotlin compile error (field.close() on a type that isn't AutoCloseable); in the other it's a silent leak — the #218 failure mode reappearing at the seam the fix introduced. A test over the covertest declaration set asserting type_close_strategy(ty).is_some() == plan_of(ty).destructible() for every declared sum and data class would turn the claim into a guard. Cheap, and it's the invariant the whole design now rests on.

3. type_close_strategy peels the sequence layer to decide, then drops it from the answer

struct_plan.rs:582 and :607:

let core = bare.sequence_elem().unwrap_or(bare);
...
reaches.then(|| whole_value_close(ty.optional_inner().is_some()))

reaches is answered about the element, but the returned strategy is never Iterable. So a Vec<Sum-that-reaches-a-handle> answers Base, and render_handle_close emits v0.close() on a List<…>.

The inline comment justifies this with "the bridge already rejects Vec<sum> and Vec<data class> fields" — but those rejections live in classify_field, and this function's own doc says it exists precisely because two callers hold no plan. write_sealed_classes runs build_sealed_class over every declared sum, whether or not any plan ever classifies it, so the rejection being relied on isn't on that path. Either fold the Iterable layer into the returned strategy, or make the guard explicit here (a panic! naming the shape, in the house style) rather than borrowing one from a function that doesn't run.

4. iface.rs asks for a strategy and throws it away

Three call sites (iface.rs:1228, :1280, :1537) use type_close_strategy(...).is_some(), and the proxy then hand-rolls the close as {local}{dot}close() with dot picked from g.typed.is_nullable() (iface.rs:500) — while kotlin_emit.rs renders the same predicate's answer through render_handle_close(&strategy, …).

So one predicate, two renderings, and the iface one silently assumes the answer is Base or Optional(Base). A Handle projection whose strategy is Iterable returns early at struct_plan.rs:594 with that strategy intact, and owned becomes true — the proxy would then emit __own0.close() on a List.

Carrying close: Option<FoldStrategy> on RunArg/TypedGroup and calling render_handle_close would make both sites the same rendering, and it subsumes the nullable: field — Optional already means ?. in that renderer.

5. The fold change isn't in ⚠️ Breaking

fixed_folder_typed_groups (iface.rs:1537) flips the element from hardcoded owned: false to the predicate, so a Vec<data_class that reaches a handle> now closes each element after each run. That's consistent with the rest of the PR, and I think it's right — but it isn't a sum, docs/sum-types.md §4.5 doesn't reach it, the breaking section doesn't mention it, and there's no test. It also bites the most natural fold body there is ({ acc, e -> acc + e }), which now accumulates closed handles unless every element is take()n. Worth its own bullet in the body at minimum.

6. Minor: reassembly happens outside the try

In the generated LedgerCallback.asRaw, __own0 and __own1 are both built before try. If __own1's when hits the else -> throw IllegalArgumentException, __own0 — already holding a live handle the proxy just took ownership of — is never closed. Narrow (invalid tag means the native side is broken) and the single-owned-arg shape had it already, but this PR is what makes multiple owned reassembled args common, and the leak is now on the ownership path rather than incidental.

7. Nits

• struct_plan.rs:487 — the intra-doc link super::JniGen::write_sealed_interface names no such method; it's Declarations::build_sealed_class. rustdoc doesn't flag it, which is why it's worth fixing by hand.
• whole_value_close hardcodes NullableKind::Boxed for a value whose real kind may be Niche. Harmless only because render_handle_close matches Optional(_, inner); it's a wrong fact in the model waiting for that arm to start branching on the kind.
• struct_plan.rs:565 — depth > 16 returns None where build_struct_plan asserts on the same bound. A cycle the plan builder would reject loudly answers "nothing to close" here, silently, in the leak direction. If the recursion is genuinely bounded elsewhere, an assert! matching the plan builder would say so.

---

Nothing above touches the core decision, which I think is clearly the right one over #218's "document it". (1) and (2) are the two I'd want before merge; (3)–(5) are small and localized.

[milyin]

All seven addressed in cd44250. Point by point, with the two disagreements marked.

1. Nested-data-class covertest exercise — added

Dossier { note: i64, holder: Holder } in perftest-flat/src/ext.rs, declared beside Holder, plus a harness section that closes two levels down with one close() and checks the Summary underneath. 51 sections pass under ./gradlew run.

Your reason for wanting it is the right one and worth keeping visible, so it is in the type's own doc: the cascade is a one-line holder.close() that frees something only because Holder was independently rendered AutoCloseable by another pass, and an emission test cannot see the difference because it never compiles the inner class.

2. Agreement pinned by a test

a_types_close_answer_matches_its_plans asserts type_close_strategy(ty).is_some() == classify_field(ty).destructible() for every field of every declared type in a fixture covering all four ways to reach a handle (direct, optional, through a sum, through a nested data class) and two ways not to. Field-level rather than type-level because there is no public constructor for a bare TypeRef from a name — and it lands on the same invariant.

Verified as a guard, not decoration: reverting the Nested arm fails it with the two forms disagree about 'Everything.inner'.

The doc comment no longer claims construction. It names both differences you found — Sum-before-output_entry ordering as an unstated precondition, and the totality asymmetry as deliberate — and points at the test.

3. Sequence layer folded into the answer

Took the first option: whole_value_close(optional, sequence) puts back exactly the layers peeled, so Vec<sum-that-reaches-a-handle> answers Iterable(Base) and render_handle_close emits the forEach. You are right that the guard being borrowed was one that does not run on this path, which is what made a panic! the wrong choice here — the shape is answerable, so answering it beats naming it.

4. One predicate, one rendering

RunArg / TypedGroup / GroupDesc carry close: Option<FoldStrategy> and the proxy renders through render_handle_close.

One disagreement: it does not subsume nullable. A reassembled sum's nullability comes from its selector — a sum under a conditional value form reconstructs to null where the form was absent — while type_close_strategy is asked of leaf.out_ty, which is not optional. Dropping the field would have emitted .close() on a T? and stopped compiling covertest.kt. So nullable_close adds the Optional layer the type never names, and says why at the field.

5. Fold change documented

Its own bullet in docs/sum-types.md §4.5 and at the call site, including that it is not a sum-only rule (a nested data_class element lands there too) and that { acc, e -> acc + e } accumulates closed handles unless the body take()s.

No test, deliberately. Nothing in the acceptance set reassembles a fold element that reaches a handle — summary_series' element is decomposed to (count, total) leaves, so SummaryFolder never holds one — and the generated output is byte-identical there. Adding a fixture would mean inventing an API to reach the branch. Say the word if you would rather have it invented.

6. Reassembly moved inside the try

Each owned local now opens its own try/finally and everything bound after it happens inside:

val __own0 = when (…) { … }
try {
    val __own1 = when (…) { … }
    try { run(…) } finally { __own1?.close() }
} finally { __own0?.close() }

An invalid tag in the second when no longer strands the first handle. Reverse close order falls out of the nesting.

7. Nits

• Link fixed — Declarations::build_sealed_class.
• Boxed is now stated as the fact it is rather than a default: the receiver is always a generated Kotlin reference, and a niche encoding is a wire fact of a handle projection, which comes back with its own strategy and never passes through whole_value_close. So the arm can start branching on the kind without this becoming wrong.
• Depth bound assert!s, matching build_struct_plan, rather than answering None in the leak direction.

---

CI gate run locally before pushing: cargo test --all, clippy --all-targets --all-features -D warnings and fmt --check (CI's exact config) on 1.85.0 and stable, regen-check.sh clean, covertest-kotlin$ ./gradlew run PASS. cargo doc still reports the 8 pre-existing errors this branch inherits from main — all in mod.rs / decl.rs / trait_impl.rs / builder.rs, none in the files touched here; #381 is what fixes them.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

prebindgen-jni/src/jni/struct_plan.rs:612

• This only recognizes an outer Option followed by one sequence layer. Supported shapes such as Vec<Option<DataClassWithHandle>> (for example as a sealed payload) leave core as Option<DataClassWithHandle>, so type_kind reports Other and the generated sum is not AutoCloseable; similarly, deeper sequence nesting loses the close cascade. Walk the wrapper tree recursively and rebuild the FoldStrategy in the original order (e.g. Iterable(Optional(Base))) rather than peeling these two fixed positions.

    let bare = ty.optional_inner().unwrap_or(ty);
    let core = bare.sequence_elem().unwrap_or(bare);

prebindgen-jni/src/jni/iface.rs:1593

• This does not implement the documented per-iteration close. fixed_folder_typed_groups is used for fixed deconstructors, but kotlin_emit.rs:1191-1214 deliberately suppresses their typed interface and asRaw adapter; the hoisted folder singleton implements the raw twin directly (kotlin_emit.rs:1296-1336, 1389-1421). Consequently this close value is never rendered or executed, while the PR description and docs/sum-types.md claim fold elements are closed after each run. Either implement that contract on the actual user-facing fold path (without closing elements appended by fixed return-list builders), or remove the unsupported claim.

            close: crate::jni::struct_plan::type_close_strategy(ext, registry, &spec.source, 0),