A1: class kind carries its own primary constructor - #7
Merged
Conversation
`KtClass` was one struct with a `kind` tag where every field applied to
every kind, so `ctor_params` was available on kinds that have no primary
constructor and the renderer printed them: `object Foo(x: Int)` rendered
happily and did not compile.
Move the constructor into the kind, which makes four shapes
unrepresentable rather than merely detectable:
* `object` / `data object` / `interface` / `sealed interface` /
`companion object` have no field to hold constructor parameters
* `data class` is constructed with its first property, so it can never
have zero
* `value class` holds exactly one property, so it can never have two
`Plain` and `Abstract` fold into `Class { modifier }`, which also makes
`open` and `sealed` classes expressible for the first time.
`ctor_param()` and the new `entry()` panic on kinds that cannot hold
what they are given. That is a generator bug caught at build time with a
precise message, in place of Kotlin that does not compile.
Rendering is unchanged: all 34 pre-existing golden-string tests pass
untouched. Seven tests added for the new invariants.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Kotlin declaration model so that class “kind” variants carry their own payload (primary constructor params / enum entries), making several Kotlin-invalid shapes unrepresentable at construction time and adjusting the renderer + tests to use the new API.
Changes:
- Restructures
KtClassKindto embed ctor params / enum entries per variant and introducesKtClassModifier+ newKtClassconstructors/builders (class_,class_with,data,value,enum_, etc.). - Updates rendering and import-collection to consume the new
ctor_params()/entries()accessors andkeyword()mapping. - Updates and extends tests to cover the new invariants and panic-on-invalid-builder-usage behavior; updates validation design doc snippet.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/model.rs | Introduces KtClassModifier, refactors KtClassKind to carry ctor/entries, adds new builders and accessors, and adds build-time panics for invalid builder usage. |
| src/render.rs | Updates rendering and import collection to use KtClass::ctor_params() and KtClassKind::entries()/keyword() after the model refactor. |
| src/tests.rs | Migrates tests to the new builder API and adds new tests for the intended invariants and panics. |
| src/lib.rs | Re-exports the new KtClassModifier public API. |
| docs/validation.md | Updates the A1 design doc snippet to reflect interface/sealed-interface payload changes. |
Suppressed comments (3)
src/render.rs:226
- The
if !matches!(c.kind, KtClassKind::Companion) ...condition immediately above tries to movekindout of&KtClass(non-Copy). Borrowc.kindin thematches!call (matches!(&c.kind, ...)).
out.push(' ');
out.push_str(&c.name);
}
src/model.rs:420
KtClass::valueis documented as wrapping exactly one property, but it currently accepts a plain ctor parameter (prop: None) orvar, which would allow building avalue classshape Kotlin rejects. Enforce that the field is avalat construction time with a clear panic message (consistent with other build-time checks in this PR).
/// A `@JvmInline value class` wrapping exactly one property.
pub fn value(name: impl Into<String>, field: KtCtorParam) -> Self {
Self::new(
KtClassKind::Value {
field: Box::new(field),
src/model.rs:486
ctor_paramstill allows building invalid shapes:
- For
KtClassKind::Data, it will accept ctor params that are notval/varproperties (Kotlin requires data-class primary ctor params to be properties). - For
KtClassKind::Value, the panic message claims the kind has no primary constructor, but a value class does have one; it just can't accept additional parameters.
Tighten the match to validateDataparams and provide a value-class-specific panic.
other => panic!(
"`{}` has no primary constructor to add parameter `{}` to",
other.keyword(),
p.name
),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+206
to
+207
| if matches!(c.kind, KtClassKind::Value { .. }) && !annotations.iter().any(|a| a == "JvmInline") | ||
| { |
Comment on lines
+413
to
+415
| pub fn data(name: impl Into<String>, first: KtCtorParam) -> Self { | ||
| Self::new(KtClassKind::Data { ctor: vec![first] }, name) | ||
| } |
This was referenced Aug 6, 2026
milyin
added a commit
that referenced
this pull request
Aug 6, 2026
Fixes seven findings raised against PRs #7-#15. All are addressed here, at the tip, so no branch in the chain has to be rewritten and rebased. model.rs: * `data class` now rejects a plain constructor parameter, in both the constructor and `ctor_param`. Kotlin requires every primary-ctor parameter of a data class to be a `val`/`var`. * `value class` now rejects a `var` or a plain parameter — it wraps a single *read-only* property. * `KtCompanion::named("")` panics instead of rendering `companion object ` with a dangling space. * `KtCompanion::extends` added. A companion object may extend a class; rendering and import collection already supported it, but no builder reached it, so the field had to be assigned directly, bypassing the one-superclass invariant. Both `extends` paths now share `KtSupertypes::set_superclass`. * `KtFun::modifier` checks each whitespace-separated word rather than the exact string, so `"external "` and `"external inline"` can no longer smuggle the keyword past the check. validate.rs: * Scope paths no longer gain a leading `/` in the root package (`Outer`, not `/Outer`). * Import-collision diagnostics report against the FIRST-registered FQN, which is the one `ImportSet` actually gives the simple name to. Previously three colliding imports named whichever was seen last. render.rs: KOTLIN_BANNER's doc said an override "falls back to" the constant, which is backwards — the fallback applies when there is no override. One review comment is not acted on: that `matches!(c.kind, ...)` moves out of a shared reference and cannot compile. It does compile — the pattern binds nothing, so nothing moves.
milyin
added a commit
that referenced
this pull request
Aug 6, 2026
* Add validation design document
Proposes what kotlin-codegen should check at generation time: which bad
shapes to make unrepresentable in the model, which checks the validator
still needs, which identifier utilities to expose, and which current
behaviours are too strict.
Design only — no code changes.
* A1: class kind carries its own primary constructor
`KtClass` was one struct with a `kind` tag where every field applied to
every kind, so `ctor_params` was available on kinds that have no primary
constructor and the renderer printed them: `object Foo(x: Int)` rendered
happily and did not compile.
Move the constructor into the kind, which makes four shapes
unrepresentable rather than merely detectable:
* `object` / `data object` / `interface` / `sealed interface` /
`companion object` have no field to hold constructor parameters
* `data class` is constructed with its first property, so it can never
have zero
* `value class` holds exactly one property, so it can never have two
`Plain` and `Abstract` fold into `Class { modifier }`, which also makes
`open` and `sealed` classes expressible for the first time.
`ctor_param()` and the new `entry()` panic on kinds that cannot hold
what they are given. That is a generator bug caught at build time with a
precise message, in place of Kotlin that does not compile.
Rendering is unchanged: all 34 pre-existing golden-string tests pass
untouched. Seven tests added for the new invariants.
* A2: a companion object becomes its own type
`KtClassKind::Companion` was documented as "only valid as
`KtClass::companion`" and nothing enforced it, so a companion object
could be built as a top-level declaration and rendered into a file where
`companion object { … }` is meaningless.
Give it its own type. `KtCompanion` has no `impl Into<KtDecl>`, so it
cannot reach a declaration position at all — proved by a `compile_fail`
doctest rather than by a runtime check. Being a distinct type it also
has no primary constructor to misuse, and no `KtClassKind` to be given
the wrong one.
Two incidental cleanups fall out:
* `name: Option<String>` replaces the empty-string-means-anonymous
sentinel the renderer used to special-case.
* `KtClass::companion` drops its `Box`: the indirection now comes from
the `Vec<KtDecl>` inside `KtCompanion`.
Supertype rendering is shared between class and companion rather than
duplicated. Rendering is otherwise unchanged: all 41 pre-existing tests
pass untouched.
* A3: at most one constructed superclass
Supertypes were one undifferentiated `Vec<(KtType, Option<KtCode>)>`
where any number of entries could carry constructor arguments, so
`class A : B(x), C(y)` — which Kotlin rejects, a class may construct at
most one superclass — rendered happily.
Split the list by role:
pub struct KtSupertypes {
superclass: Option<KtSuperclass>, // at most one, may be constructed
interfaces: Vec<KtType>, // any number, never constructed
}
`.supertype(ty, args)` is replaced by `.extends(ty, args)` and
`.implements(ty)`, which forces the caller to say which role a supertype
plays instead of leaving it implied by whether arguments were passed. A
second `.extends` panics rather than silently dropping the first.
Render order becomes structural: the superclass always leads, whatever
order the builder calls came in.
All 43 pre-existing tests pass untouched; three added.
* A4: a fun interface method has no body
`KtFunInterface::method` was a full `KtFun`, which can carry a body, and
`render_fun_interface` rendered the method verbatim — body included. A
`fun interface` must have exactly one *abstract* method, so that output
had none at all and did not compile.
Introduce `KtFunSig`: everything a `KtFun` has except a body and
modifiers. `KtFunInterface::method` holds one, so a bodied SAM method is
unrepresentable.
The type earns its keep beyond that: an abstract member *is* a
signature. `KtFunSig` converts into `KtFun` (body `None`) and into
`KtDecl`, so an interface can declare `.member(KtFunSig::new(...))`
instead of reaching for a body-less `KtFun`; `KtFun::signature()` goes
the other way, replacing the strip-body-and-modifiers helper a consumer
would otherwise hand-write.
`KtFun` and `KtFunSig` share one layout implementation through an
internal `SigView`, so the width-driven parameter breaking is not
duplicated. All 46 pre-existing tests pass untouched.
* A5: external is a body kind, not a modifier
`external` was a free-form string in `KtFun::modifiers` while the body
lived in a separate field, so `external fun f(): Int { … }` — a function
that is both natively implemented and has a body — was representable and
rendered.
Make it a `KtBody` variant. Both live in the one `body` field now, so
setting either clears the other and the combination has nowhere to
exist. `KtFun::external()` replaces `.modifier("external")`, and passing
the string panics with a message pointing at it, so the old spelling
cannot quietly reintroduce the hole.
`KtBody::None` stays: whether a bodiless function is legal depends on
where it sits — an interface member needs no keyword — and position is
not something a type can capture here. That one remains for the
validator.
All 49 pre-existing tests pass; `external` renders in the same place as
before. Three added.
* C1: Kotlin identifier utilities
A program generating Kotlin gets its names from somewhere else — a Rust
field, a C symbol, a JSON key — and some of those are never legal
Kotlin. This crate writes the file but offered nothing for dealing with
that, so every consumer has to reimplement it.
Add the primitives:
* is_valid_kotlin_ident / is_kotlin_hard_keyword / KOTLIN_HARD_KEYWORDS
* mangle_kotlin_ident — deterministic, idempotent
* mangle_kotlin_package, is_valid_kotlin_package
* escape_kotlin_ident — the back-tick strategy, which keeps the name
instead of changing it
Soft and modifier keywords (`data`, `value`, `inline`, `operator`) are
deliberately absent from the keyword list: they are contextual and are
valid identifiers, so mangling them would be wrong. A test pins that.
Two properties are tested rather than assumed: mangling always yields a
valid identifier, and is idempotent, over a table of awkward inputs.
Every function carries a doctest.
* C2: export KOTLIN_BANNER and merged_file_path
Both were marked `pub` but sit in private modules and were never
re-exported, so they were reachable from inside the crate and from
nowhere else. An audit of every `pub` item confirms these are the only
two.
They are worth having: KOTLIN_BANNER is what `KtFile::banner` falls back
to, so a consumer prepending its own header has nothing to match against
without it; merged_file_path is how write_files lays out its output, and
predicting those paths without writing anything is a reasonable thing to
want.
The new test is written against the public paths, so it stops compiling
if either is un-exported again.
* E1: diagnostics infrastructure, with the existing checks moved onto it
Adds the validation pass the rest of the umbrella hangs off, and moves
the two checks that already existed inside merge_files onto it without
changing what they detect.
Shape of it:
* KtFile::validate() / validate_with(&ValidationPolicy) -> Vec<Diagnostic>
* Diagnostic carries the Check that fired, a Severity, a scope path,
and a message
* ValidationPolicy sets any check to error / warning / off, all error
by default
* merge_files_with and write_files_with expose the policy and return
surviving warnings; merge_files and write_files keep their
signatures and deny everything
The model, builders and render() are untouched — validation is a
separate read-only pass, and rendering stays infallible so a model you
already know is broken can still be printed while debugging.
This also lands D2: every problem is reported at once instead of
stopping at the first. Fixing one name and rerunning the whole build to
find the next is a poor loop for a generator.
The class/typealias/property false positive (D1) is deliberately
preserved here — this is a move, not a fix. B2 addresses it.
* B1: check that every declared name is a legal Kotlin identifier
Names in a generated file come from somewhere else — a Rust field, a C
symbol, a JSON key — and some are never legal Kotlin. Nothing checked
them, so `my-field`, `2fast` and `object` were written out verbatim and
first surfaced as a kotlinc error on a generated file.
Walks the whole declaration tree — class bodies, companion bodies, the
method of a fun interface — and reports each bad name with a scope path
(`io.p/My-Class/object`) locating it, since the model has no source
positions.
Back-ticked names are accepted: `escape_kotlin_ident` produces them, so
rejecting them would fire on output this crate itself recommends. Two
new predicates back that: is_escaped_kotlin_ident and the
checker-facing is_writable_kotlin_ident.
Three things are deliberately left unchecked because they are free-form
fragments rather than plain identifiers, and checking them would produce
false positives: generic parameter lists (`out R`,
`T : Comparable<T>`), annotations and modifiers, and a Raw block's name
— which is a merge identity and is never rendered. Tests pin all three.
* B2: redeclaration checked per scope, per namespace
The duplicate check had three problems. It never looked inside class
bodies — where nearly all generated declarations live; one generated
file in the downstream consumer has 43 functions and none at top level.
It did not check `fun interface` at all, so two of them merged into a
broken file silently. And it did not check functions at all, on the
grounds that same-named functions might be overloads — true only when
their parameter types differ.
It also had a false positive: classes, type aliases and properties went
into one pool of names, but Kotlin keeps types and values in separate
namespaces, so `class Foo` and `val Foo` may coexist and were rejected.
That is D1, fixed here as a consequence of the split.
Now: walk the file, each class body and each companion body as its own
scope, and keep three tallies per scope — types (classes, fun
interfaces, type aliases), values (properties and `val`/`var`
constructor parameters), functions (by name *and* parameter types). A
plain non-property constructor parameter is constructor-local and
declares nothing.
Raw blocks get a rule of their own. They are hoisted singletons keyed by
name, so two fragments may legitimately carry the same one; identical
blocks now collapse during merge, and only genuinely differing blocks
sharing a name are reported.
Limitation, documented on the check: parameter types are compared as
written, so `io.p.Foo` and `Foo` are different keys and generic renaming
does not collide. It misses some real duplicates. A net, not a proof.
* F1: review follow-ups from the chain
Fixes seven findings raised against PRs #7-#15. All are addressed here,
at the tip, so no branch in the chain has to be rewritten and rebased.
model.rs:
* `data class` now rejects a plain constructor parameter, in both the
constructor and `ctor_param`. Kotlin requires every primary-ctor
parameter of a data class to be a `val`/`var`.
* `value class` now rejects a `var` or a plain parameter — it wraps a
single *read-only* property.
* `KtCompanion::named("")` panics instead of rendering
`companion object ` with a dangling space.
* `KtCompanion::extends` added. A companion object may extend a class;
rendering and import collection already supported it, but no builder
reached it, so the field had to be assigned directly, bypassing the
one-superclass invariant. Both `extends` paths now share
`KtSupertypes::set_superclass`.
* `KtFun::modifier` checks each whitespace-separated word rather than
the exact string, so `"external "` and `"external inline"` can no
longer smuggle the keyword past the check.
validate.rs:
* Scope paths no longer gain a leading `/` in the root package
(`Outer`, not `/Outer`).
* Import-collision diagnostics report against the FIRST-registered
FQN, which is the one `ImportSet` actually gives the simple name to.
Previously three colliding imports named whichever was seen last.
render.rs: KOTLIN_BANNER's doc said an override "falls back to" the
constant, which is backwards — the fallback applies when there is no
override.
One review comment is not acted on: that `matches!(c.kind, ...)` moves
out of a shared reference and cannot compile. It does compile — the
pattern binds nothing, so nothing moves.
* B3: the shape checks Part A could not remove
Three shapes are still buildable after the structural work, because each
depends on a relation between fields or on where a declaration sits
rather than on one field's type:
* `val x` with no type, no value and no accessors
* an enum entry passing no arguments to a constructor the enum declares
* a function with no body somewhere that does not mean "abstract"
The last one needs context, so the walker tracks its container: an
interface member is abstract by position and needs no keyword; a member
of an abstract or sealed class may be abstract but has to say so; at top
level, inside a concrete class, or inside a companion — which is
concrete — a body is always required. `external` is a body kind since
A5, so it is never flagged.
The property check is deliberately narrow. Only the all-three-absent
case is unambiguously wrong: a type alone is an abstract property and a
value alone infers its type, so flagging either would be a false
positive. Tested.
Two existing tests built bodiless functions incidentally while asserting
something else; they now supply bodies.
* E2: gradual-adoption policy and documentation
Adds ValidationPolicy::warn_all() and Check::ALL. A generator that
already produces output can turn every check into a warning, look at
what comes back, and drop the call once it is quiet — much less
disruptive than a build that starts failing on output that was fine
yesterday. Check::ALL keeps warn_all correct as checks are added, and a
test pins that every variant is in it.
Documents what shipped: README gains sections on validation and on the
identifier utilities, and drops a stale reference to the old `Code`
name. docs/validation.md is marked implemented and its work order
updated to the one the chain actually followed.
The remaining half of the rollout — running warn mode against the
downstream generated output and confirming it is quiet — belongs with
the consumer migration, which the umbrella scopes out of this chain.
* Fix clippy large_enum_variant: re-box the companion
CI runs `cargo clippy --all-targets -- --deny warnings`, which rejects
`KtDecl` because `Class(KtClass)` is 512 bytes against a next-largest
variant of 264 — more than the 200-byte spread the lint allows.
232 of those 512 are the companion object, held inline by every class
including the ones that have none. A2 removed its `Box` and justified
it with "the indirection now comes from the `Vec<KtDecl>` inside
`KtCompanion`" — true for recursion, which is why it compiled, but
irrelevant to size, which is what the lint is about. Restoring the box
takes `KtClass` to 288 against 264.
A test pins the invariant so the next field added to `KtClass` fails
here with an explanation rather than in CI with a lint name.
My local checks missed this twice over: `cargo clippy --quiet` without
`--deny warnings` suppressed it, and plain `cargo fmt` does not apply
the unstable import options CI passes. Both exact commands now verified.
* Declare a named companion object in its class's type namespace
Review catch on the umbrella: `check_class_scope` carried a comment
saying "only an explicitly named companion declares a type name in the
enclosing scope", but nothing ever declared it. The comment described an
intention that was never implemented, so
class Outer {
class Factory
companion object Factory { }
}
passed validation while Kotlin rejects it — both are classifiers nested
in `Outer`.
The companion is part of its class's scope rather than a member of it,
so `check_scope` now takes it alongside the members and constructor
parameters whose names it already tallies.
The anonymous case stays excluded, and the reason is now recorded where
the code enforces it rather than only in the design doc: `Companion` is
a name this crate supplies, not one the model declares, so flagging it
would second-guess a generator that resolves the collision itself by
renaming the companion. A test pins that, alongside one showing a
companion may reuse a name from the value namespace, and one showing
two classes may each have a `Factory` companion.
* temporary doc removed
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.
First step of #6. Based on
docs/validation-umbrella.KtClasswas one struct with akindtag where every field applied to everykind.
ctor_paramssat there for every kind including the ones Kotlin gives noconstructor, and the renderer printed it —
object Foo(x: Int)rendered happilyand did not compile.
Moving the constructor into the kind makes four shapes unrepresentable rather
than merely detectable:
object Foo(x: Int), and the same fordata object/interface/sealed interface/companion objectdata class Foowith no propertiesKtClass::data(name, first)takes the first, so zero is not expressiblevalue class Foowith zero or two propertiesKtClassKind::Value { field }holds exactly onePlainandAbstractfold intoClass { modifier: Option<KtClassModifier> },which also makes
openandsealedclasses expressible for the first time.On the two panics
ctor_param()and the newentry()panic when handed to a kind that cannothold them. A panic is a runtime check, which this umbrella is otherwise trying
to avoid — but the check is at build time with a precise message, and the
invalid value still cannot exist, so nothing invalid can ever reach the
renderer. The alternative that avoids it entirely is typestate, which is far
too much machinery for this crate.
Verification
All 34 pre-existing golden-string tests pass untouched — the rendered output
is byte-identical, so this is a pure representation change. Seven tests added
covering the new invariants: the three class modifiers rendering, value-class
arity, data-class minimum, the three panics, and an enum with a primary
constructor.
cargo clippy --all-targetsclean.Consumer migration (
prebindgen-jni, 9 sites) is deliberately out of scope perthe umbrella and lands later.