jnigen: give byte-backed Kotlin values content equality - #207
Conversation
A generated class mirrors a Rust type that derives `PartialEq`/`Eq`, so two
values with equal contents must compare equal. Kotlin arrays compare by
IDENTITY, so every `ByteArray`-backed property broke that silently:
Timestamp(1uL, byteArrayOf(1,2,3)) == Timestamp(1uL, byteArrayOf(1,2,3))
// false
Kotlin is inconsistent here rather than uniformly identity-based: a data
class's generated `hashCode`/`toString` DO special-case arrays
(`contentHashCode`/`contentToString`), its `equals` does not. So a broken
value hashes equal and then fails `equals` — it finds the right HashMap
bucket and is rejected there. `==` is the observable defect; a hashCode
check alone would not have found it.
`equality::content_equality_members` emits `equals`/`hashCode`/`toString`
for any class with an array-backed constructor property, and nothing for
the rest, so existing classes keep the compiler's own generation. Three
emitters feed it: `data_class!` (render.rs), value blobs and
`sealed_class!` variant payloads (kotlin_emit.rs).
Value blobs stop being `@JvmInline value class`. Kotlin 1.9 — the version
this generator targets downstream — rejects `equals`/`hashCode` members on
a value class outright ("Member with the name 'equals' is reserved for
future releases"), and the typed-equals replacement is experimental behind
an opt-in every consumer would have to set. An inline value class simply
cannot carry value equality at that language level, so a value blob is now
a plain `data class`. That costs one small allocation per crossing at the
wrapper tier; the JNI ABI is untouched, because externs already declare
`ByteArray` and the wrapper passes `.bytes` explicitly.
Coverage: nothing exercised the broken shapes — covertest had the `Stamp`
value blob but no data class with a `ByteArray` field, which is exactly why
this reached a consumer. `BlobValue` adds both (a `Vec<u8>` field beside a
scalar, plus a nested value blob) and the new section asserts content
equality, hash equality, HashSet de-duplication, `toString`, and that each
component participates. Verified the section fails without this fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Timestamp`, `ZenohId`, `EntityGlobalId` and `SourceInfo` compared by array IDENTITY, so two identically built values were unequal — the shape a consumer hits keying a map on a peer id or comparing a sample's timestamp. Fixed in the generator (milyin/prebindgen#207); this is the regeneration plus the regression test. `ZenohId` is no longer a `@JvmInline value class`. Kotlin 1.9 reserves `equals`/`hashCode` members on a value class, so an inline one cannot carry the value equality its Rust counterpart has; it is a plain `data class` now. The JNI ABI is unchanged — the externs already took `ByteArray` and the wrapper already passed `.bytes`. `ValueEqualityTest` covers the reported cases and their nesting, asserting `HashSet` de-duplication rather than only `==`: Kotlin's `data class` codegen special-cases arrays in `hashCode` but not in `equals`, so equal hashes prove nothing on their own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| .map(|d| format!("{d}\n\n{framework_line}")) | ||
| .unwrap_or(framework_line); | ||
| let mut class = KtClass::new(ClassKind::ValueInline, &class_name) | ||
| let mut class = KtClass::new(ClassKind::Data, &class_name) |
There was a problem hiding this comment.
Changing the wrapper to a reference class also changes its JVM representation when it is stored in another generated object, but the whole-object input decoder still assumes value-class erasure. In this PR's generated JObject_to_BlobValue, native code does:
env.get_field(v, "stamp", "[B")while the generated Kotlin field is now val stamp: Stamp, whose descriptor is Lio/prebindgen/covertest/model/Stamp;.
I reproduced this by marking the new BlobValue declaration .jobject_input(), adding blob_value_echo(BlobValue) -> BlobValue, and round-tripping the value. The JVM fails with:
java.lang.NoSuchFieldError: io.prebindgen.covertest.model.BlobValue.stamp [B
Please update the ProjectionKind::ValueBlob branch in emit/flat_input.rs to read the Stamp object and then its bytes (including nullable handling), and add this jobject-input round trip to the covertest. The old [B lookup was valid only while the field was inline-erased.
| /// compares by value (scalars, `String`, enums, nested generated classes). | ||
| fn array_prop(ty: &kt::KtType) -> Option<ArrayProp> { | ||
| match ty.simple_name() { | ||
| Some("ByteArray") => Some(ArrayProp::Bytes { |
There was a problem hiding this comment.
This classification fixes only a directly declared ByteArray, but supported container shapes can still contain arrays and retain identity equality. For example, adding chunks: Vec<Vec<u8>> to the new BlobValue generates:
val chunks: List<ByteArray>
// ...
chunks == other.chunksTwo independently returned values with equal chunks then fail the PR's own equality assertion, and toString() renders the nested element as [B@...:
BlobValue(... chunks=[[B@3830f1c0], ...) !=
BlobValue(... chunks=[[B@39ed3c8d], ...)
Vec<Vec<u8>> resolves and the binding compiles, so this is not an unsupported source shape. Please make the content operations recursive through array-bearing containers (and cover this case), or explicitly reject such properties instead of silently emitting identity semantics.
`BlobValue` carried an `n: i64` that earned nothing. The generator has two per-property branches — array (`contentEquals`) and non-array (`==`) — and `n` took the identical path as the nested `stamp`, emitting identical code. Two fields already exercise the multi-property `hashCode` fold, so it was padding. Removing it exposed a gap it had been hiding: the array sat FIRST, so the fixture only produced `var result = id.contentHashCode()`. A real value has the bytes last (`Timestamp(ntp64, id)`), which emits the other form, `result = 31 * result + id.contentHashCode()`. The fields are now ordered `(stamp, id)` so the covered shape is the one downstream actually gets. Verified the remaining two fields still catch the defect on their own, by suppressing the members for `BlobValue` alone: the data-class assertion fails without them, independently of the value-blob assertion above it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both reported on the PR, both real, both mine. **Whole-object input read the wrong descriptor.** A value-blob field's JVM slot was `[B` only while the wrapper was `@JvmInline`-erased. It is a real class now — it has to be, to carry value equality — so `struct_input_body`'s `ProjectionKind::ValueBlob` branch has to read the wrapper object and then its `bytes`. The old lookup raised `NoSuchFieldError: BlobValue.stamp [B` on the first decode. A null wrapper still yields a null `[B`, so the field's own converter carves `None` exactly as before. `read_kotlin_property` (sealed-class payloads) made the same assumption by falling through to `jni_field_access`, which maps a `JByteArray` wire to `[B`. Fixed there too rather than waiting for it to be reported: the two paths differ only in which JVM object they read the field off. **Content operators stopped at the property.** `Vec<Vec<u8>>` resolves and compiles, and its `List<ByteArray>` inherits `ByteArray`'s identity equality, so equal chunks compared unequal and `toString` printed `[[B@3830f1c0]`. `array_bearing` / `eq_expr` / `hash_expr` / `str_expr` now recurse through containers to the arrays underneath. A container of *classes* is untouched — those already compare by value, so only an array at the bottom makes a property array-bearing. The container hash folds with the same 31 multiplier `Arrays.hashCode` uses, so a `List` and the array it came from agree. Coverage for both: `BlobValue` gains a `chunks: Vec<Vec<u8>>` field and is declared `.jobject_input()` with a `blob_value_echo` round trip. Verified each catches its own defect — reverting the recursion reproduces the identity-compared chunks, reverting the descriptor reproduces `NoSuchFieldError` — rather than assuming the assertions bite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both confirmed and fixed in 1. Whole-object input read the wrong descriptorExactly as you diagnosed: env.get_field(v, "stamp", "Lio/prebindgen/covertest/model/Stamp;") // wrapper
env.get_field(&__stamp_jobj, "bytes", "[B") // then bytesA null wrapper still yields a null I also fixed 2. Content operators stopped at the propertyYou're right that this isn't an unsupported source shape. chunks.size == other.chunks.size && chunks.indices.all { __i ->
val __x = chunks[__i]; val __y = other.chunks[__i]; __x.contentEquals(__y) }
// hash
result = 31 * result + (chunks.fold(1) { __acc, __e -> 31 * __acc + __e.contentHashCode() })
// toString -> chunks=[[9], [8, 7]]A container of classes is deliberately untouched — those already compare by value, so only an array at the bottom makes a property array-bearing. The container fold uses the same 31 multiplier as Coverage
I verified each assertion catches its own defect rather than assuming:
|
|
Superseded by #209, which removes the value blob entirely rather than making it comparable. The equality work here is carried forward in #209 (commits preserved) — it is still needed for Why supersede instead of merge: the review of this PR surfaced two more defects in the blob's raw-memory representation (a wrong JVM field descriptor once the wrapper stopped being Teaching the resolver about |
* Realign the binding with zenoh-flat HEAD
zenoh-flat renamed part of its surface and moved several handle types to
value forms, so this binding's declarations named items that no longer
exist and `Registry::resolve` refused the whole generation. Nothing new is
bound here: this is the same surface, remapped.
Renames: `keyexpr_get_str` -> `keyexpr_as_str`, `zbytes_as_bytes` ->
`zbytes_to_bytes`, `*_get_keyexpr` -> `*_get_key_expr`, and the
`*_get_zid` / `*_get_eid` pairs -> a single `*_get_id` returning
`EntityGlobalId`. `config_new_from_json` is gone (json5 remains). The
Kotlin method names follow their source idents, as they always have, so
`getStr` becomes `asStr` and `asBytes` becomes `toBytes`; nothing
hand-written referenced either.
Handles that became values, each now declared as what it is rather than as
an opaque handle plus accessors:
* `Timestamp` — `ptr_class!` + `timestamp_get_ntp64` / `_get_id` becomes
`data_class!`, so it crosses as leaves reassembled in Kotlin bytecode.
* `SourceInfo` replaces `sample_get_source_{zid,eid,sn}`; `EntityGlobalId`
replaces `reply_get_replier_{zid,eid}`. Optionality now lives on the
whole value, which is what the source crate models.
* `ZenohId`'s `zenoh_id_to_bytes` accessor is redundant — the blob IS the
value class's `bytes` property.
* `Selector` — `session_get` takes the whole selector, so its
`.split_on_param("key_expr")` no longer has a param to split.
* `RecoveryMode` became a data-carrying enum, so it is `sealed_class!`.
Test fallout, both from source-crate signature changes rather than from
this binding: `encoding_get_schema` yields raw bytes now, so the encoding
correspondence test transcodes UTF-8 at the boundary (its whole corpus is
text); `zenoh_id_to_string` is fallible, so the zid test takes the typed
`onError` and drops the all-zero id — those bytes are not an identifier,
so there is no native rendering to correspond to.
The zenoh-flat CI pin moves to 3f431b6b, whose surface this generates
against byte-for-byte.
The ~70 zenoh-flat items added alongside these changes (links, transports,
timestamp stacks, the `*_to_struct` value forms, publisher matching
listeners) stay UNBOUND and therefore visible as `skipping undeclared`
warnings. They are pending work, not acknowledged exclusions, so they do
not belong in the `ignore` list.
Requires milyin/prebindgen#205: `RecoveryMode`'s `Duration` payload needs
the converter stage chain that PR restores at sum-payload leaves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Regenerate against prebindgen main
The committed artifacts were generated before prebindgen picked up the
boundary aliasing guards (#200) and the converter stage-chain fix (#205),
so a plain `cargo build` no longer reproduced them.
The one behavioral change is in `session.get`: `Selector` carries its key
expression as a nested handle, so the new guards now reject a call that
passes the same native resource twice — as `this` and the selector's key
expression, or as the selector's key expression and the encoding. Those
are consumed-handle aliases, which the previous generation let through.
The Rust side is cosmetic only: a chain-less converter call is now
wrapped in a block, which is how the composed form degenerates when a
type has no conversion stages.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Regenerate: byte-backed values now compare by content
`Timestamp`, `ZenohId`, `EntityGlobalId` and `SourceInfo` compared by array
IDENTITY, so two identically built values were unequal — the shape a
consumer hits keying a map on a peer id or comparing a sample's timestamp.
Fixed in the generator (milyin/prebindgen#207); this is the regeneration
plus the regression test.
`ZenohId` is no longer a `@JvmInline value class`. Kotlin 1.9 reserves
`equals`/`hashCode` members on a value class, so an inline one cannot carry
the value equality its Rust counterpart has; it is a plain `data class`
now. The JNI ABI is unchanged — the externs already took `ByteArray` and
the wrapper already passed `.bytes`.
`ValueEqualityTest` covers the reported cases and their nesting, asserting
`HashSet` de-duplication rather than only `==`: Kotlin's `data class`
codegen special-cases arrays in `hashCode` but not in `equals`, so equal
hashes prove nothing on their own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* ZenohId is an ordinary data class
The value blob is gone from the generator (milyin/prebindgen), so `ZenohId`
is declared `data_class!`: a plain value with one fixed-width byte field,
which now crosses as a Kotlin `ByteArray` through the generic fixed-size
array support rather than as the raw memory image of the Rust struct.
The Kotlin API is unchanged. `data class ZenohId(val bytes: ByteArray)`
either way, with the same content `equals`/`hashCode`/`toString`; the ctor
property loses an explicit `public` that a data-class property has by
default anyway. `zidString()` and the SDKs' `inner.bytes` keep working.
What this buys: the JVM no longer holds a `repr(Rust)` struct's memory
image (padding included, layout unguaranteed), and the decode no longer
`read_unaligned`s caller-supplied bytes after only a length check — `Copy`
never implied every bit pattern was a valid value.
One cold-path cost: `session.zid()` and the other bare `ZenohId` returns now
go through the `__ZenohIdBuilder` 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. Tracked as milyin/prebindgen#208 together with packing a small
array into scalar slots.
18/18 JVM tests, fmt + clippy clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* CI: one prebindgen main per run, and don't cache a stale generator
Two jobs in the same run could see different generators, and a run could
see a generator older than its own cache.
`branch = "main"` was resolved independently by each of the four jobs, so a
prebindgen merge landing mid-run split the run across two generators. Resolve
main to a commit once, in a `resolve-prebindgen` job, and pin every job to
that rev. Still always the latest main — just one of them per run.
The `target/` cache was worse. zenoh-flat's build-script OUT_DIR lives at
`target/<profile>/build/zenoh-flat-<hash>/out`; the proc-macro only ever
*adds* uniquely-named `.jsonl` files there (`create_new`, never truncating),
and `Source::read_group` reads every matching file in the directory and dedups
by record name in `read_dir` order. A `target/` restored from a run with
different sources therefore hands the generator the union of old and new
records: items deleted upstream survive, and collisions resolve arbitrarily.
The prefix `restore-keys` made that the normal case, since the key hashed
`Cargo.lock` files that are gitignored and never exist. In run 30292836956,
Lint and Build both pinned zenoh-flat at 3f431b6b yet reported different line
numbers for the same file (`advanced_subscriber/mod.rs:164` vs `:184`,
`session/mod.rs:262` vs `:327`) — two different stale unions.
So key the target caches on both source revisions and drop the prefix
restore-keys: a hit is coherent, a miss is a clean build. Hoist the zenoh-flat
pin to `env.ZENOH_FLAT_REF` so the four copies cannot drift, and echo the
rewritten dependency lines to make the resolved generator visible in the log.
This is a workaround for the generator reading stale sibling files; the
directory-hygiene fix belongs in prebindgen.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* CI: restore the target cache restore-keys
The previous commit dropped them on a mistaken diagnosis of mine, which this
corrects.
I read run 30292836956's Lint and Build jobs reporting different source
locations for the same pinned zenoh-flat (`advanced_subscriber/mod.rs:164` vs
`:184`) as a stale `target/` feeding the generator a union of old and new
JSONL records. It is not. Both jobs report the *same* 18 unresolved types;
only the call site each is attributed to differs, because `Source::read_group`
dedups records into a `HashMap` and returns `into_values()`, so when several
functions share an unresolved type — several take an `impl Fn(Miss)`, several
reference `ZenohId` — which one is blamed is unordered. And the hygiene works:
`init_prebindgen_out_dir()` wipes the directory on every build-script run, and
a source change does re-run it, verified by renaming a `#[prebindgen]` item and
watching the old record disappear.
So there was no corruption to protect against, and keying the cache to an exact
prebindgen commit with no fallback bought nothing while making every prebindgen
merge — several a day right now — a cold rebuild of zenoh and its dependency
tree. Cargo's own fingerprinting is what keeps an incremental build correct; a
cache key only has to avoid an absurd mismatch.
The exact key stays keyed on both source revisions, which is still an
improvement on the old one: that hashed `Cargo.lock` files that are gitignored
and never exist, so it was the constant `${{ runner.os }}-cargo-build-target-`
and never varied at all. Now a run prefers its own revisions, falls back to the
same zenoh-flat, then to any.
The `resolve-prebindgen` job is unaffected and stands on its own: pinning every
job in a run to one resolved `main` removes a real race where a merge landing
mid-run splits the run across two generators.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Reported on ZettaScaleLabs/zenoh-flat-jni#16: generated values with equal contents compare unequal.
A generated class mirrors a Rust type deriving
PartialEq/Eq, so this is the generator contradicting the type it mirrors — not a binding bug.What Kotlin actually does
Measured on Kotlin 1.9.0 before writing any code:
==hashCodeequal@JvmInline value class V(val b: ByteArray)data class D(val n: ULong, val id: ByteArray)data classholding the value classKotlin is inconsistent, not uniformly identity-based: a data class's generated
hashCode/toStringdo special-case arrays (contentHashCode/contentToString) while itsequalsdoes not. That is why the report saweq=false hashEq=true— a broken value finds the rightHashMapbucket and is then rejected.==is the observable defect; a hashCode check alone would not have found it. (hashSetOf(t1, t2).size == 2confirms the practical damage.)Why value blobs lose
@JvmInlineKotlin 1.9 rejects the members outright:
So an inline value class cannot be given value equality at this language level, and the experimental typed-equals replacement would force an opt-in flag on every consumer of a shared tier. Value blobs are therefore emitted as a plain
data class.Cost: one small allocation per crossing at the wrapper tier. The JNI ABI is untouched — externs already declare
ByteArray(external fun zenohIdToString(z: ByteArray, …)) and the wrapper passes.bytesexplicitly, so the erasure was never load-bearing. Value blobs are also documented as off-hot-path.The change
equality::content_equality_membersemitsequals/hashCode/toStringfor any class with an array-backed constructor property and nothing for the rest, so existing classes keep the compiler's own generation and the diff stays confined. Three emitters feed it:data_class!(render.rs), value blobs andsealed_class!variant payloads (kotlin_emit.rs).Coverage
Nothing exercised the broken shapes — covertest had the
Stampvalue blob but no data class with aByteArrayfield, which is precisely why this reached a consumer.BlobValueadds both (aVec<u8>field beside a scalar, plus a nested value blob) and the new section asserts content equality, hash equality,HashSetde-duplication,toString, and that each component participates (a comparison ignoring one would otherwise still pass).Verified the section fails without the fix:
PASS - 46 sections, 334 lib tests, fmt/clippy clean.🤖 Generated with Claude Code