B1: check that every declared name is a legal Kotlin identifier by milyin · Pull Request #15 · milyin/kotlin-codegen

milyin · 2026-08-06T11:17:11Z

Step B1 of #6. Stacked on #14.

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 that gives no hint about what produced it.

The check walks the whole declaration tree — class bodies, companion bodies, the
method of a fun interface — and locates each bad name with a scope path, since
the model has no source positions:

error [invalid-identifier] in `io.p`: class name `My-Class` is not a valid Kotlin identifier
error [invalid-identifier] in `io.p/My-Class`: constructor parameter name `2fast` is not a valid Kotlin identifier
error [invalid-identifier] in `io.p/My-Class/object`: parameter name `in` is not a valid Kotlin identifier

Back-ticked names are accepted

C1 (#12) ships escape_kotlin_ident, which produces `object`. A checker
that only accepted plain identifiers would fire on output this crate itself
recommends — a false positive of exactly the kind the umbrella says to avoid.
Two predicates close that: is_escaped_kotlin_ident, and the checker-facing
is_writable_kotlin_ident (plain or escaped).

Three things deliberately left unchecked

Each would be a false positive, and each has a test pinning the decision:

Not checked	Why
generic parameter lists	`out R`, `in T`, `T : Comparable<T>` are parameter declarations, not identifiers
annotations and modifiers	free-form fragments (`final override`)
a `Raw` block's `name`	a merge identity, never rendered — it need not be a legal Kotlin name

Verification

All 65 pre-existing tests pass. Five added, including the three
must-not-fire cases above.

Copilot

Pull request overview

Adds generation-time validation to ensure all declared names (and package paths) are writable Kotlin identifiers, including support for backticked/escaped identifiers, and reports diagnostics with scope paths through the declaration tree.

Changes:

Add InvalidIdentifier / InvalidPackage validation checks and run them during KtFile::validate_with.
Add identifier predicates is_escaped_kotlin_ident and is_writable_kotlin_ident and re-export them publicly.
Add tests covering invalid identifiers (with scope paths), invalid packages, and explicitly-not-checked cases.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/validate.rs	Adds identifier/package validation pass and declaration-tree walk for name checks.
src/ident.rs	Introduces predicates for escaped identifiers and “writable” identifiers (plain or escaped).
src/lib.rs	Re-exports new identifier utilities.
src/tests.rs	Adds tests for identifier/package validation behavior and non-checked cases.

Suppressed comments (1)

src/validate.rs:320

Class/member scope paths are also constructed with unconditional "{scope}/..." concatenation, producing leading / when validating a root-package file. Use the same conditional join logic here (including for companion scope) so scopes remain stable and consistent.

fn check_class_identifiers(c: &KtClass, scope: &str, d: &mut Diagnostics<'_>) {
    check_ident(&c.name, "class", scope, d);
    let inner = format!("{scope}/{}", c.name);
    for p in c.ctor_params() {
        check_ident(&p.name, "constructor parameter", &inner, d);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        KtDecl::Fun(f) => {
+            check_ident(&f.name, "function", scope, d);
+            let inner = format!("{scope}/{}", f.name);
+            for p in &f.params {
+                check_ident(&p.name, "parameter", &inner, d);
+            }
+        }
+        KtDecl::FunInterface(i) => {
+            check_ident(&i.name, "fun interface", scope, d);
+            let inner = format!("{scope}/{}", i.name);
+            check_ident(&i.method.name, "function", &inner, d);
+            let method = format!("{inner}/{}", i.method.name);
+            for p in &i.method.params {
+                check_ident(&p.name, "parameter", &method, d);
+            }
+        }


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.

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.

* 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

milyin requested a lite review from Copilot August 6, 2026 11:18

Copilot started reviewing on behalf of milyin August 6, 2026 11:18 View session

milyin mentioned this pull request Aug 6, 2026

B2: redeclaration checked per scope, per namespace #16

Merged

Copilot AI reviewed Aug 6, 2026

View reviewed changes

This was referenced Aug 6, 2026

Umbrella: generation-time validation #6

Merged

F1: review follow-ups from the chain #19

Merged

milyin force-pushed the step/e1-diagnostics branch from d439317 to 67466ee Compare August 6, 2026 11:40

milyin force-pushed the step/b1-name-validity branch from f572b22 to 28cfcd4 Compare August 6, 2026 11:40

milyin changed the base branch from step/e1-diagnostics to docs/validation-umbrella August 6, 2026 11:41

milyin merged commit cc2ee88 into docs/validation-umbrella Aug 6, 2026
0 of 4 checks passed

Navigation Menu

B1: check that every declared name is a legal Kotlin identifier - #15

B1: check that every declared name is a legal Kotlin identifier#15
milyin merged 1 commit into
docs/validation-umbrellafrom
step/b1-name-validity

milyin commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

milyin commented Aug 6, 2026

Back-ticked names are accepted

Three things deliberately left unchecked

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants