Remove the value blob; fixed-size arrays cross as Kotlin primitive arrays

[milyin]

Supersedes #207, whose equality work is carried forward here (commits preserved).

Why

A value_class! type crossed as the raw memory image of the Rust struct. That representation produced three defects in one review cycle and carried a latent unsoundness:

• Unequal — it surfaced as a ByteArray, which compares by identity, so ZenohId(b) != ZenohId(b) (the original report).
• Unfixable as a value class — Kotlin reserves equals/hashCode members on a value class through 2.2.21 (measured, including -Xvalue-classes), so the wrapper had to become a real class — losing the zero-allocation property that was the entire point.
• Unsound — input read_unaligned::<T>()-ed caller-supplied bytes after a length check only. Copy never implied every bit pattern was a valid T, and no trait asserts that: struct S { flag: bool } plus a byte of 2 was UB.
• Layout-dependent — the JVM held a repr(Rust) struct's memory image, padding included, for a layout with no stability guarantee.

The blob existed for exactly one reason: the resolver could not handle a fixed-size array field. ZenohId { bytes: [u8; ZENOH_ID_MAX_SIZE] } failed with Unresolved { key: TypeKey("[u8 ; 16]") } in both directions.

What

1. [T; N] crosses as the matching Kotlin primitive array. Table-driven off the existing JniPrim scalars — [u8; N] → ByteArray, [i64; N] → LongArray, and so on for all eight — bulk-copied through set_*_array_region / get_*_array_region, boxing nothing. Wider unsigned elements carry raw bits in the signed array, matching the existing scalar rule (u64 already crosses as a raw jlong); Kotlin's ULongArray is @ExperimentalUnsignedTypes and would push an opt-in onto every consumer of a shared tier.

N is never needed at generation time — it is often a const path, not a literal — so the decode leans on TryFrom<&[T]> for [T; N]. That try_into is the length check. Elements convert by cast, never transmute, which is what removes the UB.

2. The blob is deleted — declarator, ProjectionKind::ValueBlob, WrapKind::Blob, write_value_blobs, the __assert_copy guard, ~50 arms across 17 files. ValueClassDecl was a strict subset of DataClassDecl, so both call sites migrated by changing the declarator alone.

3. Content equality (from #207) is carried forward and widened — every Kotlin primitive array compares by identity, not just ByteArray.

Found by doing it, not assumed

• Consts in type position were never qualified. QualifyEmittedTypes implemented only visit_type_path_mut, but an array length is an expression path — so [u8; ZENOH_ID_MAX_SIZE] emitted a const that wasn't in scope. The new visit_expr_path_mut is restricted to registry-indexed const idents; that restriction is load-bearing, since expression paths also cover every local in a converter body (v, env).
• Four tables independently encoded "which wires are object-shaped" / "which Kotlin types are arrays". Adding seven wires needed all four, and I found that by breaking three in turn (missing lifetime specifier → unsupported wire JShortArray → NoSuchMethodError: run). Now one table each in wire_access.
• is_leaf_vec_element's doc contradicted its body (claimed opaque handles excluded; cfg.is_opaque() includes them). Pre-existing, corrected in passing.

Coverage

Arrays carries one field per primitive family member plus [u64; 2] pinning the raw-bits rule, round-tripped both directions with per-element value checks — an equality-only check between two echoes would survive a cast bug. BlobValue keeps the array-backed equality cases. Stamp becomes the negative case: a scalar-only data class the content operators must leave alone.

The two binding-error sections that sourced their failure from the blob's byte-length guard are re-pointed at the fixed-size-array length guard, its direct successor.

Cost, measured

ZenohId's generated Kotlin is byte-identical apart from the now-false blob KDoc — the check that this removal is API-neutral.

One cold-path regression, tracked in #208: a bare ZenohId return now goes through the fromParts builder upcall where the blob returned its wire directly. Nested uses (EntityGlobalId.zid inside SourceInfo — the sample callback's hot path) ride their parent's single fromParts and are unaffected. #208 also covers packing a small array into scalar slots, which would retire the equality machinery for such types entirely.

Verification

334 lib tests · PASS - 46 sections · fmt + clippy clean. Downstream ZettaScaleLabs/zenoh-flat-jni#16 builds against this with 18/18 JVM tests.

🤖 Generated with Claude Code

[milyin]

This visitor rewrites expression paths in the entire generated item, not only const expressions in type positions, so a captured const can collide with generator-owned locals. I reproduced it with the legal source:

#[allow(non_upper_case_globals)]
#[prebindgen]
pub const env: usize = 4;

#[prebindgen]
pub struct Arrays {
    pub bytes: [u8; env],
}

The const need not even be declared to JniGen (prebindgen reports it as skipped). Because registry.consts still places env in const_names, generation rewrites every JNI local use:

perftest_flat::env.get_java_vm()
converter(&mut perftest_flat::env, value)

The isolated covertest build then fails with roughly 1,750 errors such as no method named get_java_vm found for type usize.

Please scope this qualification to syn::TypeArray::len rather than installing a global visit_expr_path_mut pass. A visit_type_array_mut implementation can visit the element normally and run a small const-path visitor only over arr.len; local variables cannot occur in that type-level expression. A regression using a lowercase const named env would pin the distinction.

[milyin]

Confirmed and fixed in 41ad36b. Your diagnosis was exact, including the part I got wrong in reasoning.

What I got wrong

I did consider this hazard — the commit message says "expression paths are also every local in a converter body, and qualifying those indiscriminately would rewrite v and env" — and concluded that restricting to registry-indexed const idents closed it. It doesn't, for the reason you found: a source crate can legally declare a const named env. The restriction narrows which idents get rewritten; it does nothing about a const whose name collides with a generator-owned local. Reasoning where I should have tested.

Fix

Exactly as you proposed — visit_type_array_mut visiting the element normally and running a small QualifyConstPaths visitor over arr.len alone. An array length is the only place a const legitimately appears inside a type, and a local can never appear there, which is what makes the scope safe rather than merely narrower.

Why no test caught it

Because the corpus contained only consts that follow Rust naming convention:

fixture	array length	collides with a local?
covertest Arrays	[u8; 4], [i16; 2], …	no const at all
zenoh-flat ZenohId	[u8; ZENOH_ID_MAX_SIZE]	SCREAMING_SNAKE — never a local

So the real downstream case exercised the qualification path and passed, and nothing exercised the collision. Triggering it needs a lowercase const, which requires #[allow(non_upper_case_globals)] — unusual, but legal, and the generator has no business breaking on it.

Regression

array_length_const_is_qualified_without_touching_locals names its const env on purpose and asserts in both directions:

assert!(rc.contains("[u8;myflat::env]"));                      // length IS qualified
assert!(rc.contains("env.byte_array_from_slice"));             // local is NOT
assert!(!rc.contains("myflat::env.byte_array_from_slice"));
assert!(!rc.contains("myflat::env,"));

Neither direction alone pins the behavior — the first passes under a no-op pass, the rest pass if qualification is simply deleted. I verified the pair fails under the old whole-item pass before keeping it, and it reproduces your symptom precisely (myflat::env, ×10 and myflat::env. ×4 in converter bodies).

335 lib tests, PASS - 46 sections, fmt/clippy clean.

[milyin]

This only qualifies bare, single-segment constants, so an associated const used as the length remains in the generated crate scope. I reproduced it with both types declared to JniGen:

#[prebindgen]
pub struct ArrayLen { pub marker: u8 }

impl ArrayLen { pub const N: usize = 4; }

#[prebindgen]
pub struct Arrays { pub bytes: [u8; ArrayLen::N] }

cargo build -p covertest-kotlin --offline then emits [u8; ArrayLen::N] in three converter signatures/ascriptions and fails each with E0433: use of undeclared type ArrayLen; the expressions need to be perftest_flat::ArrayLen::N. Since the fixed-array feature accepts ordinary Rust length expressions, please also qualify the leading source-type segment of multi-segment paths inside arr.len (the existing source_names map has the origin when that type is declared), and add an associated-const regression.

[milyin]

Confirmed and fixed in 205bba9.

The gap

You're right that an array length is an ordinary Rust const expression, so it reaches the source crate two ways and I handled only one. The shape now decides which map applies:

length	segments	looked up in	result
[u8; MAX]	1 — a const	const_names	myflat::MAX
[u8; Holder::N]	2+ — leading segment is a TYPE	source_names	myflat::Holder::N

Only the leading segment is rewritten; ::N stays relative to the type that owns it, so the result is never myflat::Holder::myflat::N (asserted).

One thing worth recording, because my first assertion was wrong

I initially asserted the bare spelling appears nowhere in the file. That failed even with the fix applied, and the reason is legitimate: the decode's diagnostic embeds the type as the source wrote it —

fixed-size array decode: `[u8;Holder::N]` expects a different length

That message should name the type as declared, not the generated-crate path. So the assertions now name the two code positions (return type, param type) instead of pattern-matching the whole file. Worth flagging because a naive "bare form must not appear" check would either fail spuriously or push someone into qualifying a user-facing message.

Regression

Extended the existing test rather than adding a second one, so the free-const and associated-const cases are pinned side by side against the same fixture — including the env local-collision case from your previous comment. Verified it fails with the leading-segment lookup removed (Result<[u8;Holder::N] present, qualified form absent) before keeping it.

335 lib tests, PASS - 46 sections, fmt/clippy clean.

Boundary I did not cross

Qualification uses source_names, so it resolves when the owning type is declared to JniGen — same condition under which a bare Holder in type position resolves today. An associated const on an undeclared type stays unqualified. That's consistent rather than special-cased, but say the word if you'd rather it fall back to registry.origin_module for any indexed item.

[milyin]

This lookup still makes qualification depend on the associated-const owner being declared as part of the JniGen surface. I reproduced the remaining case by removing only .class(data_class!(Holder)) from the new regression while keeping the captured Holder item and declared Blob: the targeted test then generates bare [u8; Holder::N] in the converter return/parameter types and fails (a compiled consumer hits the same E0433 as before).

Holder is not a boundary field type here; it is only the Rust namespace for a compile-time length. Requiring its declaration emits an otherwise-unused Kotlin class merely to make generated Rust compile, and is asymmetric with the free-const path above, which qualifies indexed consts even when they are skipped by JniGen. Please derive the associated-owner lookup from all indexed source structs/enums plus origin_module (or an equivalent registry-wide map), and leave Holder undeclared in the regression so this opt-in boundary is pinned.

[milyin]

Fixed in c103958. You're right, and your framing is better than the one I offered.

I defended the old behavior as "consistent — same condition under which a bare Holder in type position resolves". That comparison was wrong: a type in field position is a boundary type and should need declaring, whereas Holder here is only the Rust namespace owning a compile-time length. Nothing about it crosses. And as you point out it was not even internally consistent — the free-const branch one line above already qualifies any indexed const, declared or not.

Change

Both halves of a length expression now read one registry-wide map (length_names: indexed consts + structs + enums → origin_module), so:

length	owner must be declared?
[u8; MAX]	no (unchanged)
[u8; Holder::N]	no (was: yes)

QualifyConstPaths → QualifyLengthPaths since it is no longer const-specific. The leading-segment rule is untouched: whichever shape the path has, the first segment carries the origin module and everything after stays relative to it.

Regression

Holder is now left undeclared, so the boundary is pinned rather than incidentally satisfied. Verified it fails with the registry-wide lookup removed, reproducing your exact symptom — bare Result<[u8; Holder::N] in the converter signature.

That is the third distinct defect this one visitor has had (whole-item scope → single-segment-only → declared-surface-only). Each was found by you rather than by the tests, and each time I shipped on reasoning where a test would have settled it; the regression now covers all three shapes against one fixture, each verified red before green.

335 lib tests, PASS - 46 sections, fmt/clippy clean.

[milyin]

The registry-wide map still omits functions, but an array length can call a source-defined const fn. I reproduced this on c103958 by adding an indexed-but-undeclared item and one field to the new regression:

pub const fn array_len() -> usize { 4 }
// in Blob
pub called: [u8; array_len()],

The focused test reports skipping undeclared #[prebindgen] fn array_len and generates bare [u8; array_len()] in the input/output converter signatures and ascription; a consumer build therefore fails with E0425 instead of resolving myflat::array_len(). This is the same compile-time-namespace case as the skipped free const and Holder: the function does not belong in the Kotlin surface merely because its result determines an array length. Please include registry.functions in length_names and extend the regression with an undeclared const-fn call.

[milyin]

Fixed in 227d32b.

Root cause, not the instance

Adding registry.functions would have fixed your case and left the identical question open for the next item kind. The real defect is that this site enumerated item kinds when what it wanted was "anything the source crate defines" — and that enumeration had already missed twice before you found this one (associated consts, then const fns).

The map is now built from registry.item_origins — the very map origin_module reads:

let length_names = registry.item_origins.keys()
    .map(|ident| (ident.to_string(), registry.origin_module(ident)…))
    .collect();

It covers every indexed item by construction, cannot drift from the lookup it feeds, and needs no revisiting when a new kind is indexed.

Nothing gets caught by mistake. A length's paths are consts, the types owning them, and const fns — all indexed items. A const generic is not indexed, and generated code defines no locals in a length position, so the wider map has no false-positive surface.

Regression

All three shapes against one fixture, with none of the three owners declared to JniGen:

pub const env: usize = 4;              // free const, name collides with a local
impl Holder { pub const N: usize = 4; }  // associated const
pub const fn array_len() -> usize { 4 }  // const fn call

Verified with the map narrowed back to consts-only: Holder::N and array_len() both emit bare while myflat::env still qualifies — so the test discriminates the three shapes rather than passing on one of them.

Standing back

That is four defects in this one visitor, all found by you. The first three I fixed as reported, which is exactly why there was a fourth: each fix addressed the instance and left the enumeration pattern that produced it. This one changes the pattern.

335 lib tests, PASS - 46 sections, fmt/clippy clean.

[milyin]

item_origins is not actually the set of every indexed item: Registry::from_items only inserts into it when SourceLocation::crate_name is Some. Core explicitly supports origin-less hand-built streams and documents that adapters fall back to crate for them; the previous item-map loops retained those names and applied that fallback.

I reproduced the regression on 227d32b by changing only the new test fixture to let loc = SourceLocation::default() and expecting the documented crate:: prefix. The focused test then emits bare [u8; env], [u8; Holder::N], and [u8; array_len()] in converter code (while declared Blob correctly becomes crate::Blob) and fails all three qualification assertions; this also breaks if the generated file is included below the crate root. Please derive the name set from all indexed named items independently of whether an origin stamp exists, then use origin_module(...).unwrap_or_else(default_module) for the value. A registry-level named-item iterator would avoid returning to per-adapter kind enumeration; an origin-less variant of this regression would pin the fallback.

[milyin]

Fixed in 58e4d13. You're right, and this one is worse than the previous rounds: I introduced it while claiming to fix the class.

item_origins is populated only when SourceLocation::crate_name is Some, so for an origin-less stream the map is empty and every length emitted bare. The kind-enumeration loops I replaced did not have that bug — they read the item maps and fell back to default_module. I asserted "covers every indexed item by construction" without checking how the map I switched to is built, and traded a miss for a regression.

Change

Two separate concerns, now separated:

• name set — every named item the registry indexes, independent of origin stamps;
• value — origin_module(..).unwrap_or_else(default_module), which is what supplies the documented crate:: fallback.

The kind enumeration moves to core as Registry::named_item_idents() rather than back into the adapter. That was the real defect two rounds ago — an adapter listing item kinds it doesn't own — so the list now sits beside the maps, where a new kind is added once at the point it is introduced.

Regression

Both invariants pinned, and they discriminate: the existing test covers a stamped stream, the new array_length_qualification_falls_back_to_crate_without_an_origin covers an origin-less one. Re-deriving the name set from item_origins leaves the stamped test passing and fails only the origin-less one — exactly your reproduction.

336 lib tests, PASS - 46 sections, fmt/clippy clean.

[milyin]

This safety premise is not true once the length contains an inline const block: those blocks may bind locals, and the length visitor currently has no scope tracking. This is valid Rust (verified with rustc):

pub const fn array_len() -> usize { 4 }

pub struct Blob {
    pub local: [u8; const {
        let array_len = 3;
        array_len
    }],
}

With array_len indexed but undeclared, adding this field to the new regression on 58e4d13 rewrites the tail expression to myflat::array_len, producing converter types like:

[u8; const { let array_len = 3; myflat::array_len }]

That is a function item rather than the local usize, so the generated consumer no longer compiles (and even a same-typed collision could silently change the length). Please make the length visitor scope-aware for bindings introduced inside inline const/block expressions and leave locally bound paths untouched. Skipping inline consts wholesale would also miss legitimate source references inside them, so a regression should cover both a shadowing local and a source item used in the same kind of block.

[milyin]

Confirmed in 0820084 — but rejected rather than scope-tracked, which is a deliberate departure from what you asked for.

Your analysis is right: the premise in that doc comment ("a local cannot occur in a type-level expression") is false for an inline const block, and qualifying a shadowing local into a source item is exactly the silent-wrongness this pass exists to avoid. The same-typed collision you mention is the worse case — it would compile and change the length.

Why reject instead of scope-track

Scope tracking is the general answer, and it means teaching a name-qualification pass the binding rules of an expression grammar — let, patterns, shadowing, nested blocks. That is a real amount of machinery for a shape that has no business in an FFI boundary struct, and it is machinery whose bugs would be silent rather than loud. Refusing costs nothing a source crate can't trivially work around:

const LOCAL_LEN: usize = 3;
pub local: [u8; LOCAL_LEN],   // an ordinary path again

The whole family goes: inline const, block, unsafe block, closure, async.

A block that binds nothing is refused too, deliberately. "Binds nothing" is precisely the judgement that needs the scope tracking to be trustworthy, so accepting on it would smuggle the premise back in. That does refuse a legitimate source reference inside such a block, as you noted — the conservative direction, and the same hoist fixes it. If you'd rather have the general machinery, say so and I'll build it; I did not want to add a silent-failure surface on a code path that has now had five defects.

Regression

array_length_opening_a_scope_is_rejected uses your shadowing case, so it pins the rejection rather than the mere presence of a block. Verified with the guard removed: the generator accepts the input and emits the mis-qualified length, so the test discriminates.

337 lib tests, PASS - 46 sections, fmt/clippy clean.

[milyin]

This list is not every expression form that can introduce a binding. match arms bind patterns directly, and if let/while let do likewise; their nested bodies are not guaranteed to appear as an Expr::Block node seen by this matcher.

I verified this with valid Rust and then replaced only the new regression field on 0820084:

pub local: [u8; match 3 {
    array_len => array_len,
}],

where the indexed const fn array_len() already exists. cargo test -p prebindgen --lib array_length_opening_a_scope_is_rejected --offline then fails with test did not panic as expected. Generation proceeds and the arm-body path is subject to the same rewrite from local array_len to the source function.

Please reject pattern-binding forms too (Match, If/Let, and any supported loop forms). Given the intentionally conservative policy here, a whitelist of expression shapes proven not to introduce bindings may be safer than another blacklist: otherwise each omitted/new syn::Expr variant recreates the silent-misqualification risk. Add at least the match-arm case to the rejection regression.

[milyin]

Fixed in cab1390, and I took the whitelist — you were right that another blacklist just recreates the risk.

match arms bind their patterns with no Expr::Block node for the matcher to see, so my list missed them; if let / while let / for are the same shape. Reproduced your case exactly: generation proceeded and rewrote the arm-body array_len into the indexed const fn.

Whitelist

Accepted: Lit, Path, Call, and const arithmetic over those (Paren, Group, Binary, Unary, Cast) — which is every form the three supported length shapes actually use (4, MAX, Holder::N, array_len(), A + 1, X as usize). Every other syn::Expr, existing or newly added, is refused.

The failure mode flips from "a local is silently mistaken for a source item" to "a legitimate length is refused" — loud, and worked around by hoisting into a named const.

Worth naming plainly: this is the third defect in this code path from enumerating the bad cases instead of the good ones. First the visitor scoped to the whole item, then the length-name map enumerated item kinds, now the rejection enumerated binding forms. Each time I fixed the instance and left the shape that produced it. Inverting the check is the part that should have happened two rounds ago.

Regression

Three cases sharing one fixture — inline const block, match arm, if let — each shadowing the indexed array_len. Verified the previous blacklist lets the match case through (test did not panic as expected), so they discriminate rather than all passing for the same reason.

Still a judgement call, flagging again

Refusing is a deliberate departure from your original ask for scope-aware tracking, taken on the repo owner's instruction. A block that binds nothing is refused too, so a legitimate source reference inside one is collateral. If you'd prefer the real scope tracking, I'll build it — but I'd rather not add a silent-failure surface to a path with this history without you choosing it explicitly.

339 lib tests, PASS - 46 sections, fmt/clippy clean.

[milyin]

The whitelist accepts every Expr::Path, but this branch explicitly skips paths with qself. That leaves a valid associated-const spelling accepted yet unqualified:

pub struct Holder { pub marker: u8 }
// source impl: `impl Holder { const N: usize = 4; }`
pub qualified_assoc: [u8; <Holder>::N],

I verified the syntax with rustc, then added only this field to the existing qualification fixture on cab1390. The focused test generates [u8; <Holder>::N] unchanged in the input/output converter signatures and ascription (rather than [u8; <myflat::Holder>::N]), so a consumer build cannot resolve Holder in the generated crate.

Please either qualify the type inside qself with the same registry-wide origin map, or—consistent with the new conservative policy—reject Expr::Path when qself.is_some() during whitelist validation. The current combination silently accepts a form the qualifier declines to process. A regression using <Holder>::N would pin the chosen behavior; <Holder as Trait>::N deserves the same explicit decision.

[milyin]

Confirmed, and deliberately not fixed here — filed as #210 instead.

Your finding is correct: the whitelist accepts every Expr::Path while QualifyLengthPaths skips qself, so <Holder>::N is accepted and then silently left unqualified. <Holder as Trait>::N is the same shape.

The reason for an issue rather than an eighth patch: this is the seventh defect in this code path, and the last one states the pattern outright. There are two independent walks over the length expression — a validator deciding what is accepted, and a rewriter deciding what it qualifies — with nothing tying them together. Every round has been the two disagreeing about one input:

#	Symptom	Which walk was wrong
1	locals rewritten (env)	rewriter scoped too widely
2	Holder::N unqualified	rewriter: single-segment only
3	undeclared owner unqualified	rewriter read the declared surface
4	array_len() unqualified	name set enumerated item kinds
5	origin-less streams unqualified	name set omitted them
6	const { let … } mis-qualified	validator's premise was false
7	match / if let mis-qualified	validator enumerated binding forms
8	<Holder>::N accepted, unqualified	validator accepts what the rewriter declines

Fixing #8 on its own — qualify qself, or reject it — leaves the mechanism that produced #1–#8 in place, and I have now demonstrated seven times that I will not spot the next instance by inspection.

#210 proposes collapsing the two walks into one fallible pass whose contract is "Ok only if every path in the length was resolved and rewritten", so acceptance becomes a consequence of qualification instead of a parallel judgement. It also carries the qself decision, a table-driven test over length shapes, and a note to revisit the conservative-rejection policy adopted here under time pressure — with a single walk that reports precisely what it could not resolve, real scope tracking becomes tractable and the collateral refusal of binding-free blocks could go away.

Your call whether #209 ships with the qself hole open (it is a silent wrong-output case, so there is a reasonable argument for a one-line rejection here and the restructure in #210) or waits. Happy to do either — I did not want to make that call unilaterally after this many rounds.