Umbrella: generation-time validation - #6
Merged
Conversation
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.
This was referenced Aug 6, 2026
`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.
`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.
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.
`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.
`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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
A2: a companion object becomes its own type
A3: at most one constructed superclass
A4: a fun interface method has no body
A5: external is a body kind, not a modifier
C1: Kotlin identifier utilities
C2: export KOTLIN_BANNER and merged_file_path
E1: diagnostics infrastructure
B1: check that every declared name is a legal Kotlin identifier
B2: redeclaration checked per scope, per namespace
B3: the shape checks Part A could not remove
E2: gradual-adoption policy and documentation
F1: review follow-ups from the chain
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces generation-time validation for the Kotlin declaration model, aiming to surface model construction mistakes (invalid identifiers, redeclarations, invalid shapes) before emitting .kt files, while keeping rendering infallible for debugging.
Changes:
- Adds a validation pass (
KtFile::validate*) with per-check severity control (ValidationPolicy) and structured diagnostics. - Refactors the model to make many invalid Kotlin shapes unrepresentable (constructor payloads per class kind,
KtCompanion,KtSupertypes,KtFunSig,KtBody::External). - Exposes Kotlin identifier utilities and improves documentation around validation and emitted-file metadata.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/validate.rs | Implements the validation pass, diagnostics, checks, and policy. |
| src/ident.rs | Adds identifier/package validation, mangling, and backtick-escaping utilities. |
| src/model.rs | Reshapes core model types to encode more Kotlin invariants structurally. |
| src/render.rs | Updates rendering/import collection for new model shapes (KtFunSig, KtCompanion, supertypes). |
| src/file.rs | Runs validation during merge/write; adds *_with APIs returning warnings and returns validation errors. |
| src/lib.rs | Exposes new APIs (validator, identifier helpers, new model types, exported constants/helpers). |
| src/tests.rs | Updates existing tests for new APIs and adds extensive validation/shape regression tests. |
| README.md | Documents validation goals/constraints and identifier utilities. |
| docs/validation.md | Adds the design record describing validation scope, rationale, and deliberate omissions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+401
to
+414
| /// A class body is its own scope, and so is its companion's. | ||
| fn check_class_scope(c: &KtClass, scope: &str, d: &mut Diagnostics<'_>) { | ||
| let inner = scope_join(scope, &c.name); | ||
| check_scope(&c.members, c.ctor_params(), &inner, d); | ||
| if let Some(comp) = &c.companion { | ||
| // Only an explicitly named companion declares a type name in the | ||
| // enclosing scope. The implicit `Companion` is a name this crate | ||
| // supplies rather than one the model declares, so treating it as a | ||
| // declaration could fire on a generator that manages the collision | ||
| // itself. | ||
| let cscope = scope_join(&inner, comp.name.as_deref().unwrap_or("Companion")); | ||
| check_scope(&comp.members, &[], &cscope, d); | ||
| } | ||
| } |
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.
This was referenced Aug 6, 2026
Merged
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.
Umbrella for making generation-time mistakes catchable, tracked against
docs/validation.md(added by this PR — design only,no code changes).
The problem
A program writes the declaration model, so the mistakes are program
mistakes: a name derived from a Rust field that happens to be
object, twogenerated classes that mangle to the same name, a branch that built an
objectwhere it meant adata class. None of it is caught today — itsurfaces when
kotlincruns, often in another build, in another repository,against a generated file that gives no hint about what produced it.
Two rules bound the work. Only what the model proves — no type
resolution, no parsing of body text. No false positives, ever — the first
false alarm teaches people to switch the checks off.
Part A — make bad shapes impossible to build
KtClassis one struct with akindtag where every field applies to everykind, which is where most "wrong shape" bugs come from. Doing this first means
the checks below never have to be written.
object Foo(x: Int),empty
data class, wrong-arityvalue class,interface Foo(x: Int).Folds
Plain/Abstractinto one variant with a modifier, makingopenand
sealedexpressible for the first time.KtCompanionbecomes its own type instead of a class kind — acompanion object can no longer be built as a top-level declaration. Also
retires the empty-string-means-anonymous sentinel.
superclass: Option<..>+interfaces: Vec<..>—class A : B(x), C(y)becomes unrepresentable.KtFunSig(no body, no modifiers) forfun interfacemethods,interface members and abstract members — a bodied SAM method becomes
unrepresentable, and the consumer's hand-written body-stripping helper
becomes a typed conversion.
KtFunBody::External— movesexternalout of the free-formmodifier list, so
external fun f() { ... }can't be built.Deliberately not fixed structurally, with reasoning in the doc: bare
val x(too many legal combinations to encode without wrecking the API), andenum entries missing constructor arguments (cross-field agreement over raw
text).
Part C — utilities to expose
is_valid_kotlin_ident,mangle_kotlin_ident,mangle_kotlin_package,escape_kotlin_ident(backtick form — the otherstrategy Kotlin offers, and nobody has it today),
is_kotlin_hard_keyword.prebindgen-jnihas private copies of thefirst three and deletes them.
KOTLIN_BANNERandmerged_file_path— both arepubinprivate modules and unreachable from outside. Oversight.
Part B — what the validator still has to do
predicate, no context, no false positives. The crate does none of this
today, and
prebindgen-jnionly checks names it derives — anythingarriving by another route is unchecked.
bodies, and keep three separate namespaces (types / values / functions),
with functions keyed by name and parameter types so real overloads
pass. Today's check misses
fun interfaceentirely, never looks insideclass bodies — where nearly all generated functions live: one generated
file has 43 functions and none at top level — and doesn't check functions
at all.
val x; an enum withconstructor parameters whose entries supply none; a body-less function
that isn't an interface or abstract-class member.
Deferred: JVM-erasure clashes (
f(List<String>)vsf(List<Int>)). Needs aKotlin-to-JVM mapping with subtle corners —
@JvmInline value classparameters change the answer — and
prebindgen-jnialready does it correctlyfor its own surface.
Part D — what to relax
class Foo+val Fooin one package is rejected today, butKotlin allows it — types and values are separate namespaces. Live false
positive; the B2 split fixes it as a side effect.
name, rerun the build, find the next is a bad loop for a generator.
with a live consumer that regenerates on every build; without an escape
hatch, a check that misfires leaves that consumer pinning an old version.
Leaving alone, deliberately: same-short-name classes in one file render the
loser fully qualified rather than erroring. That repairs the problem instead
of reporting it, and should be documented so nobody "fixes" it.
Plumbing and rollout
model stays a dumb data structure; builders stay infallible;
render()stays infallible so a knowingly-broken model can still be printed for
debugging. Checks run on the write path (
merge_files,write_files).zenoh-flat-jni, confirmclean, then flip the default to error.
Order
A → C → B1 → B2 → B3 → E. Part A first because it deletes work from Part B.
Effect on
prebindgen-jniOnly consumer, local path dependency, so migration lands in the same session.
Part A touches 9
KtClassKindconstruction sites, 12 supertype sites (8builder calls, 4 direct field pushes — all but three pass no constructor
arguments, so the
extends/implementssplit maps cleanly), 5 companionsites and 5
"external"modifier strings.Verification is concrete:
zenoh-flat-jniregenerates its committed Kotlin onevery
cargo build, so a clean regeneration with an unchanged diff provesnothing broke.