Migrate prebindgen-jni to kotlin-codegen's class model and ident module - #380
Merged
Conversation
#379 renamed the types; it did not migrate the model restructure behind them. prebindgen-jni did not compile against kotlin-codegen `main` — 24 errors across render.rs, kotlin_emit.rs and iface.rs. Class model. `KtClassKind` now carries what its kind can hold, so the constructors replace the unit variants: `class_`, `class_with(Abstract)`, `data(name, first)`, `data_object`, `enum_`, `interface_`, `sealed_interface`. A `data class` takes its first constructor property at construction — Kotlin requires one — so the two emitters that built the parameter list first now hand over its head, and a fieldless struct panics with a message instead of rendering Kotlin that never compiled. Supertypes split: `.extends(ty, args)` for the one superclass a class constructs, `.implements(ty)` for interfaces, replacing the flat `.supertype`/`supertypes.push`. `KtCompanion` replaces `KtClass::companion_object`, and `KtFunInterface::new` takes a `KtFunSig` — a SAM method cannot have a body, so `to_decl` and `to_raw_decl` build signatures now. Four `.modifier("external")` became `.external()`. `external` is a body kind rather than a modifier in the new model, and `.modifier` asserts against the string, so these were latent runtime panics, not just compile errors. `asRaw` needs milyin/kotlin-codegen#21. It was built by packing the extension receiver into the function name; the new `InvalidIdentifier` check reported all twelve. It now rides `KtFun::receiver`, so the name is a plain identifier. KOTLIN_CODEGEN_REV is bumped accordingly. Identifier de-duplication. symbols.rs carried its own `HARD_KEYWORDS`, `is_valid_kotlin_ident`, `mangle_kotlin_ident` and `mangle_package` — byte-identical in behaviour to `kotlin_codegen::ident`, which did not exist when they were written. Two copies of the keyword list means the crate that mangles a name and the crate that validates the file it lands in can drift apart. Deleted, along with the four tests that duplicate kotlin-codegen's own, and re-exported upstream; `mangle_package` takes the upstream spelling so there is one name. `check_ident` deliberately keeps `is_valid_kotlin_ident` rather than kotlin-codegen's `is_writable_kotlin_ident`: a back-ticked name is legal Kotlin but cannot be a native-symbol component, so the generator must not emit one. validate_symbols stays. It runs before emission, names the Rust origin and the build.rs knob that produced a bad name, and its JVM-erasure overload table catches platform declaration clashes that `Check::DuplicateFunction`, a structural parameter-list comparison, does not. Banner restored to "Auto-generated by JniGen" via `KtFile::banner`, after the merge so it does not depend on fragment order. The generated files had started naming kotlin-codegen as their generator. Verified by forced regeneration (`cargo clean -p covertest-kotlin -p perftest-kotlin`, rebuild): the emitted Kotlin differs from main in exactly those 8 banner lines and nothing else, so the migration is output-preserving. Workspace tests pass with default and all features; fmt and clippy -D warnings clean on stable and 1.85.0.
Was the PR branch head, so CI could run before that PR merged. It squash-merged as 796c25c, which is the commit this change is actually tested against.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates prebindgen-jni to compile and generate Kotlin correctly against kotlin-codegen’s restructured class model and centralized identifier utilities, while restoring the intended “JniGen” generator banner in emitted .kt files.
Changes:
- Migrates Kotlin emission to the new
kotlin-codegenclass model APIs (KtClass::{class_,data,enum_,…},KtCompanion,extends/implements,.external()). - De-duplicates Kotlin identifier mangling/validation by re-exporting
kotlin_codegen::identhelpers fromprebindgen-jniand updating call sites. - Restores a stable generated-file banner by applying
KtFile::bannerafter fragment merging.
Reviewed changes
Copilot reviewed 7 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| prebindgen-jni/src/jni/symbols.rs | Re-exports identifier helpers from kotlin-codegen and removes duplicated implementations/tests. |
| prebindgen-jni/src/jni/render.rs | Adapts enum/data-class/handle rendering to the new class model (KtCompanion, enum_().entry, data(...first), extends/implements, .external()). |
| prebindgen-jni/src/jni/kotlin_emit.rs | Applies a consistent generated-file banner after merge_files; updates class construction/supertypes and companion handling. |
| prebindgen-jni/src/jni/iface.rs | Switches SAM method construction to KtFunSig and uses KtFun::receiver for asRaw. |
| prebindgen-jni/src/jni/decl.rs | Switches package sanitization to mangle_kotlin_package spelling. |
| prebindgen-jni/src/jni/config.rs | Switches base-package sanitization to mangle_kotlin_package spelling. |
| examples/perftest-kotlin/kotlin/generated/io/prebindgen/perftest/storage.kt | Regenerated banner line. |
| examples/perftest-kotlin/kotlin/generated/io/prebindgen/perftest.kt | Regenerated banner line. |
| examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/storage.kt | Regenerated banner line. |
| examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/model.kt | Regenerated banner line. |
| examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/esc_pkg.kt | Regenerated banner line. |
| examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/errors.kt | Regenerated banner line. |
| examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/analytics.kt | Regenerated banner line. |
| examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest.kt | Regenerated banner line. |
| .github/workflows/rust.yml | Updates pinned KOTLIN_CODEGEN_REV used by CI. |
Suppressed comments (1)
prebindgen-jni/src/jni/iface.rs:556
- The generic arguments are still embedded into the return type’s class-name string (
KtType::cls(format!("{}{gen_args}", ...))), which drops structured type arguments fromKtType(it leavesargsempty and bakes<...>intofqn). That can break any logic that inspectsKtType::Named { fqn, args, .. }(e.g. erasure/descriptor validation). Build the generic return type viaKtType::genericinstead.
let mut f = KtFun::new("asRaw").vis(KtVis::Public).receiver(recv);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // below: both are same-package short names with the generic arguments | ||
| // already applied. It rides `KtFun::receiver`, not the name, so `asRaw` | ||
| // stays a plain identifier the Kotlin checker can accept. | ||
| let recv = KtType::cls(format!("{}{gen_args}", self.name)); |
Review feedback on #380: both were built as `KtType::cls(format!("{name}{gen_args}"))`, which bakes `<R>` into the `fqn` of a `KtType::Named` while leaving its `args` empty. The type then renders correctly but reads wrong — `erase_kt_type` takes `simple_name()` and falls through to `fqn`, so `Foo<A>` and `Foo<B>` would erase to two distinct JVM tokens instead of the one raw class they share. Nothing consumes it that way today (the generated proxies do not enter the overload table), so this is a latent trap rather than a live bug. `KtType::generic` guards its argument list with `if !args.is_empty()`, so the non-generic case renders identically and needs no branch. The return type carried the same construction and gets the same fix. Output is unchanged: a forced regeneration of both Kotlin examples is byte-identical, covering the generic form (`fun <R> LedgerBuilder<R>.asRaw(): LedgerBuilderRaw<R>`) and the bare one (`fun PayloadCallback.asRaw(): PayloadCallbackRaw`).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
prebindgen-jni/src/jni/kotlin_emit.rs:1988
ctor_params_mutmust not fabricate an empty mutable slice (&mut []). For kinds that cannot have primary-ctor params, returnNoneinstead; otherwise return the underlying storage slice.
fn ctor_params_mut(class: &mut KtClass) -> &mut [KtCtorParam] {
match &mut class.kind {
KtClassKind::Class { ctor, .. }
| KtClassKind::Data { ctor }
| KtClassKind::Enum { ctor, .. } => ctor,
KtClassKind::Value { field } => std::slice::from_mut(field),
KtClassKind::Object
| KtClassKind::Interface
| KtClassKind::SealedInterface
| KtClassKind::DataObject => &mut [],
}
.github/workflows/rust.yml:37
- PR description says
KOTLIN_CODEGEN_REVis temporarily pointing at a PR branch head and there is a TODO on this line to re-point it before merging. That TODO isn't present in the workflow, increasing the chance it gets merged still pointing at a non-main commit.
# Local development still tracks kotlin-codegen `main` (see
# download_repositories.sh), matching how the other sibling repos work.
KOTLIN_CODEGEN_REV: 796c25c19d2dbeecbdc58637194d3d3ee9da2993
Comment on lines
1941
to
1944
| if include_ctor_props { | ||
| for p in &mut class.ctor_params { | ||
| for p in ctor_params_mut(class) { | ||
| if p.prop.is_some() { | ||
| iface = iface.member( |
It was a sibling path dependency because the crate was unpublished. 0.2.0 is on crates.io and carries the extension-receiver API this branch needs, so the workspace takes it from the registry like any other dependency. That removes the scaffolding the path dep required: the three CI checkouts of milyin/kotlin-codegen and the KOTLIN_CODEGEN_REV pin, whose whole purpose was making an external git revision reproducible. Cargo's own resolution does that now, and a version requirement is the normal way to say which one is needed. The jobs keep their `path: prebindgen` + `working-directory: prebindgen` layout — no longer strictly needed, but unrelated to this change. README no longer describes the dependency as a path dep. Verified against the published crate (registry source confirmed via cargo metadata, not a stale path override): workspace builds and tests pass with default and all features, fmt and clippy -D warnings are clean on stable and 1.85.0 — which is also the crate's own MSRV — and a forced regeneration of both Kotlin examples is byte-identical.
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#379 renamed kotlin-codegen's types (
Code→KtCode, …). It did not migrate the model restructure sitting behind those names, soprebindgen-jnidid not actually compile against kotlin-codegenmain:24 errors across
render.rs,kotlin_emit.rsandiface.rs.Class model
KtClassKindnow carries what each kind can hold, so the constructors replace the unit variants:KtClassKind::PlainKtClass::class_(name)KtClassKind::AbstractKtClass::class_with(KtClassModifier::Abstract, name)KtClassKind::DataKtClass::data(name, first)KtClassKind::DataObjectKtClass::data_object(name)KtClassKind::Enum(entries)KtClass::enum_(name)+.entry(e)KtClassKind::Interface/SealedInterfaceKtClass::interface_/sealed_interfaceKtClass::companion_object()KtCompanion::new().supertype(ty, args).extends(ty, args)/.implements(ty)class.supertypes.push((ty, None))class.supertypes.interfaces.push(ty)KtFunInterface::new(name, KtFun)… (name, KtFunSig)Three of these needed more than a substitution:
data classtakes its first constructor property at construction, because Kotlin requires at least one. The two emitters that built the whole parameter list first now hand over its head and append the rest. Adata_class-declared struct with no fields now panics with a message naming the struct, rather than renderingdata class Foo()— which never compiled as Kotlin anyway.NativeHandleis extended,AutoCloseableand the generated<Name>Apiare implemented.KtFunInterfacetakes a signature, which is the honest type — a SAM interface's one method is abstract by definition.to_decl/to_raw_declbuildKtFunSignow.apply_class_interfacemarks ctor propertiesoverride, which needs mutable access to storage that moved onto the kind; a smallctor_params_muthelper covers it.Four latent runtime panics
.modifier("external")→.external().externalis a body kind rather than a modifier in the new model, and.modifierasserts against the string — so these four sites were not merely compile errors, they would have panicked at generation time.asRaw— needs kotlin-codegen#21The extension-function proxy was built by packing the receiver into the function name (
"PayloadCallback<G>.asRaw"). The newInvalidIdentifiercheck reported all twelve, correctly. WithKtFun::receiverfrom milyin/kotlin-codegen#21 the name is a plain identifier again, and the rendered output is unchanged.Identifier de-duplication
symbols.rscarried its ownHARD_KEYWORDS,is_valid_kotlin_ident,mangle_kotlin_identandmangle_package. All four are byte-identical in behaviour tokotlin_codegen::ident— same 28-keyword list in the same order, same predicate, same mangling rules — which did not exist when they were written. Two copies means the crate that mangles a name and the crate that validates the file it lands in can drift apart.Deleted, together with the four tests that duplicate kotlin-codegen's own, and re-exported upstream.
mangle_packagetakes the upstream spelling (mangle_kotlin_package) so there is one name rather than an alias. −179 lines in that file.Two deliberate non-changes:
check_identkeepsis_valid_kotlin_ident, not kotlin-codegen'sis_writable_kotlin_ident. A back-ticked name is legal Kotlin but cannot be a native-symbol component, so the generator must not emit one — the stricter predicate is the correct one here, and there is now a comment saying so.validate_symbolsstays. It is not duplicated work: it runs before emission, its messages name the Rust origin and the build.rs knob that produced a bad name, and its JVM-erasure overload table catches platform declaration clashes thatCheck::DuplicateFunction— a structural parameter-list comparison — does not.Banner
Generated files had started announcing
kotlin-codegenas their generator, since kotlin-codegen changed its default constant and this crate never set one. Restored toAuto-generated by JniGenviaKtFile::banner, applied after the merge so it does not depend on fragment order.Verification
The result worth checking: after a forced regeneration —
cargo clean -p covertest-kotlin -p perftest-kotlin, then rebuild, since a cached build would pass without regenerating anything — the emitted Kotlin differs frommainin exactly the 8 banner lines and nothing else. The class-model migration and theasRawchange are output-preserving.cargo build --workspace --all-featurescargo test --workspace, and again with--all-features— CI runs bothcargo fmt --checkwith the CI config flagscargo clippy --all-targets --all-features -- -D warningson stable and 1.85.0