Model extension functions instead of packing the receiver into the name - #21
Conversation
A generator emitting `fun Foo.asRaw()` had nowhere to put `Foo`, so it put it in `KtFun::name` — the string `"Foo<R>.asRaw"`. That renders correctly and is invisible until something tries to read the name as a name, which the new `InvalidIdentifier` check does: it reported every such declaration as an invalid identifier, correctly. Add `receiver: Option<KtType>` to `KtFun` and `KtFunSig`, with a `.receiver(ty)` builder. The renderer emits it after the generic parameter list and before the name, through the import set like any other type, so `name` goes back to being a plain identifier. `KtFun::signature()` carries the receiver across. Dropping it there would silently turn an extension into a member — the same class of lossy conversion this change exists to close. `DuplicateFunction` now keys on the receiver too. Kotlin dispatches an extension on its receiver, so `Foo.f()` and `Bar.f()` are two declarations in one package; keying on the name and parameter types alone reported them as a redeclaration. The diagnostic prints the same form it keys on, `io.p.Cb.asRaw()`. Six tests: rendering with and without generics, the identifier check passing on an extension, `signature()` retention, and the three duplicate-key cases (same receiver collides, different receivers do not, an extension does not collide with a member of that name).
Review follow-ups on the receiver change, plus the example coverage it
was missing.
showcase now renders every form the receiver enables:
* a top-level extension, `fun Sample.summary(): String`
* a generic one, showing the order generics / receiver / name:
`fun <R> List<Reply>.mapValues(...)`
* a member extension inside a class
* an abstract member extension in an interface, carried by KtFunSig,
with its override in the implementor — so the signature type is
shown keeping the receiver, not just the concrete one
invalid gains two extensions on one receiver, which collide, alongside a
third on a different receiver that does not. That demonstrates the
receiver-aware overload key and the diagnostic form it prints:
`duplicate function `io.example.Codec.asRaw()``.
Two doc-comment fixes:
* `param_signature`'s doc had been left attached to the new
`fun_signature`, so one function carried two stacked doc paragraphs
and the other had none.
* The limitation note on `check_scope` explains that parameter types
are compared as written. The receiver is compared the same way and
is now part of the key, so the note says so.
|
Reviewed and pushed two follow-ups. ReviewThe change reads well and I could not fault the substance. Checked specifically:
Two doc-comment fixes
The limitation note on Example coverage
public interface Describable {
fun describe(): String
fun Sample.label(): String // abstract member extension (KtFunSig)
}
public open class Session(initialPtr: Long) : NativeHandle(initialPtr), Describable {
public override fun Sample.label(): String = "${keyExpr}@${describe()}"
public fun ByteArray.toSample(): Sample = Sample(describe(), this)
}
public fun Sample.summary(): String = "$keyExpr (${payload.size} bytes)"
public fun <R> List<Reply>.mapValues(transform: (sample: Sample) -> R): List<R> = …The interface pair is deliberate: it shows
One thing I deliberately did not addA check on whether a Goldens regenerated. 113 unit + 3 golden + 10 doctests, clippy |
There was a problem hiding this comment.
Pull request overview
This PR extends the Kotlin function model to represent extension functions explicitly via a dedicated receiver: Option<KtType> on KtFun/KtFunSig, rather than encoding the receiver into the function name string. This unblocks identifier validation and makes duplicate-function detection align with Kotlin’s overload identity for extensions.
Changes:
- Add
receiver: Option<KtType>+.receiver(ty)builder toKtFunandKtFunSig, and ensureKtFun::signature()preserves it. - Render extension receivers in function signatures and include the receiver in duplicate-function validation keys/diagnostics.
- Add tests and update examples/goldens/docs to cover extension receiver rendering and duplicate detection behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/golden/showcase.txt | Golden output updated to include rendered extension functions. |
| tests/golden/invalid.txt | Golden diagnostics updated to show duplicate extension signature form. |
| src/validate.rs | Duplicate-function validation now keys/prints signature including extension receiver. |
| src/tests.rs | Adds unit tests covering receiver rendering, identifier validity, signature retention, and duplicate rules for extensions. |
| src/render.rs | Adds receiver rendering into fun signatures (extension syntax). |
| src/model.rs | Adds receiver to KtFunSig/KtFun, builder methods, and signature conversion behavior. |
| README.md | Updates feature list to mention extension functions. |
| examples/showcase.rs | Showcase generator expanded to emit member and top-level extension functions. |
| examples/invalid.rs | Invalid example expanded to demonstrate duplicate detection for extensions keyed by receiver. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(recv) = f.receiver { | ||
| out.push_str(&recv.render(imports)); | ||
| out.push('.'); | ||
| } |
| #[derive(Clone, Debug)] | ||
| pub struct KtFunSig { | ||
| pub name: String, | ||
| pub vis: KtVis, | ||
| pub annotations: Vec<String>, | ||
| pub kdoc: Option<String>, | ||
| /// Generic type-variable names: `["R"]` → `fun <R> …`. | ||
| pub generics: Vec<String>, | ||
| /// Extension receiver: `Some(Foo)` → `fun Foo.name(…)`. A separate field | ||
| /// rather than part of `name`, so `name` stays a plain identifier that can | ||
| /// be checked as one. | ||
| pub receiver: Option<KtType>, | ||
| pub params: Vec<KtParam>, | ||
| pub ret: Option<KtType>, | ||
| } |
Two review findings on the receiver change.
A function type in receiver position needs its own parentheses, or the
`.` binds to the return type and the output does not compile:
fun (value: Int) -> String.asRaw() // before, uncompilable
fun ((value: Int) -> String).asRaw() // after
`KtType::render` only parenthesizes a function type when it is nullable,
which is why the nullable form was already correct and the far more
likely non-nullable one was not. Receiver position is its own syntactic
context, so it gets its own method — `KtType::render_receiver` — rather
than a special case buried in the renderer. showcase now emits one, so
the golden covers it.
The PR description claimed the change was additive and left existing
callers compiling. That is not true for anyone constructing `KtFun` or
`KtFunSig` with a struct literal, and v0.1.0 is published, so the claim
mattered. CHANGELOG gains an Unreleased section recording it.
That section covers every breaking change since 0.1.0, not just this
one: the validation umbrella restructured `KtClassKind`, moved companion
objects to `KtCompanion`, split supertypes, made `KtFunInterface::method`
a `KtFunSig` and turned `external` into a body kind. The changelog
stopped at 0.1.0 and so described a crate that no longer exists.
Cargo.toml is left at 0.1.0 deliberately — the release version is a call
for the maintainer, and the changelog now says what it needs to be.
|
Both review comments addressed. 1. Function-type receivers were uncompilable — real bug, fixedReproduced before fixing: The Receiver position is its own syntactic context, so it gets its own method rather /// A function type needs parentheses there, or the `.` binds to its return
/// type instead: `fun ((Int) -> String).ext()`, never `fun (Int) -> String.ext()`.
pub fn render_receiver(&self, imports: &mut ImportSet) -> StringThree tests: the non-nullable case, the nullable case not double-parenthesized, 2. Breaking change — you were right, and it is worse than the PR saidI checked: While writing the changelog entry I found the larger problem: I left 116 unit + 3 golden + 11 doctests. Clippy |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/validate.rs:410
fun_signatureformats extension receivers viaDisplay, which prints non-null function types as(…) -> …. In receiver position this reads ambiguously (and unlike the renderer) because Kotlin requires an extra pair of parentheses:((…) -> …).ext(). This can make duplicate-function diagnostics misleading for extensions on function types.
fn fun_signature(f: &KtFun) -> String {
let params = param_signature(&f.params);
match &f.receiver {
Some(r) => format!("{r}.{}({params})", f.name),
None => format!("{}({params})", f.name),
| )) | ||
| .returns(KtType::cls("io.example.api.internal.RawSink")) | ||
| .expr_body(KtCode::new().line("RawSink { raw -> this(raw) }")); |
… diagnostics
The asRaw example did not type-check. RawSink's method takes a `Reply`,
so the lambda parameter is a `Reply`, but the extension receiver takes a
`Sample` — `this(raw)` passed the wrong type. A showcase that emits
Kotlin which would not compile is worse than no showcase, since the
whole point is to be read as a reference.
Fixed by writing what the proxy idiom actually does: narrow the raw
`Reply` before handing it to the typed callback.
RawSink { raw -> if (raw is Reply.Value) this(raw.sample) }
Also addresses the suppressed note on `fun_signature`: it formatted the
receiver through `Display`, which does not parenthesize a non-nullable
function type, so a duplicate-extension diagnostic printed
`(value: Int) -> Unit.asRaw()` while the renderer would emit
`((value: Int) -> Unit).asRaw()`. A diagnostic that does not read as the
syntax it describes is a small trap. Both now go through one predicate,
`KtType::needs_receiver_parens`, so they cannot drift.
|
Addressed, and the catch was a good one. The
|
…le (#380) * Migrate prebindgen-jni to kotlin-codegen's class model and ident module #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. * Pin KOTLIN_CODEGEN_REV to the merged kotlin-codegen#21 commit 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. * Build asRaw's receiver and return type with structured type arguments 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`). * Depend on kotlin-codegen 0.2.0 from crates.io 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.
The problem
A generator emitting
fun Foo.asRaw()had nowhere to putFoo, so it put it inKtFun::name— the literal string"Foo<R>.asRaw". That renders correctly, and is invisible until something tries to read the name as a name.The new
InvalidIdentifiercheck does exactly that, and reported every such declaration:The check is right. The model was missing a concept.
The change
KtFunandKtFunSiggainreceiver: Option<KtType>with a.receiver(ty)builder. The renderer emits it after the generic parameter list and before the name, resolved through the import set like any other type:nameis a plain identifier again, so the identifier check passes on its own merits rather than by exemption.Two follow-on details:
KtFun::signature()carries the receiver. Dropping it would silently turn an extension into a member — the same kind of lossy conversion this change exists to close.DuplicateFunctionkeys on the receiver. Kotlin dispatches an extension on its receiver, soFoo.f()andBar.f()are two declarations in one package. Keying on name + parameter types alone reported them as a redeclaration — which is what surfaced once the receiver moved out of the name. The diagnostic now prints the same form it keys on:duplicate function `io.p.Cb.asRaw()`.Tests
Six, covering both halves:
InvalidIdentifiersignature()retains the receivercargo test(113 unit + 3 golden + 10 doc),cargo fmt --checkandcargo clippy --all-targets --all-features -- -D warningspass on stable and on MSRV 1.85.0. Goldens unchanged.Downstream
Found while migrating
prebindgen-jnionto this crate's current API — itsasRawproxy is the emitter described above. The matching change is milyin/prebindgen, which needs this merged first so its CI can pinKOTLIN_CODEGEN_REVto the resulting commit.Compatibility
No existing field or method changes meaning, so callers using the builders are
unaffected. It is not fully additive, though:
KtFunandKtFunSighavepublic fields, so anyone constructing either with a struct literal must add
receiver.v0.1.0is published, so that counts — the next release needs a0.2.0bump, whichCHANGELOG.mdnow records along with the other breakingchanges that have landed since
0.1.0. Cargo.toml is left alone; the releaseversion is the maintainer's call.