Skip to content

F1: review follow-ups from the chain - #19

Merged
milyin merged 2 commits into
docs/validation-umbrellafrom
step/f1-review-fixes
Aug 6, 2026
Merged

F1: review follow-ups from the chain#19
milyin merged 2 commits into
docs/validation-umbrellafrom
step/f1-review-fixes

Conversation

@milyin

@milyin milyin commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Follow-up step of #6. Stacked on #18, at the tip of the chain.

Addresses the review findings raised against #7#15. They land here rather
than in the branches they were raised on
, so nothing in the chain has to be
rewritten and every stacked PR below stays mergeable as-is.

Correctness

Finding PR Fix
data class accepted a plain constructor parameter #7 rejected in both data() and ctor_param() — Kotlin requires every primary-ctor parameter of a data class to be a val/var
value class accepted a var or plain parameter #7 rejected — a value class wraps a single read-only property
KtCompanion::named("") rendered companion object with a dangling space #8 panics; KtCompanion::new() is the anonymous form
a companion could not express : Base() through the builder #9 KtCompanion::extends added
.modifier("external ") bypassed the guard #11 each whitespace-separated word is checked, not the exact string

The companion fix is the most substantive: rendering and import collection
already supported a companion's superclass, but no builder reached it, so a
consumer had to assign supertypes.superclass directly — bypassing the
one-superclass invariant #9 exists to enforce. Both extends paths now share
KtSupertypes::set_superclass, so neither can drift.

.modifier("external inline") is also caught now — modifier strings legitimately
hold several keywords ("final override" appears in the downstream consumer),
so an exact-string compare was never sufficient. A modifier that merely
contains the substring (externalish) still passes; there is a test.

Diagnostics quality

  • Scope paths no longer gain a leading / in the root package (B1: check that every declared name is a legal Kotlin identifier #15) —
    Outer, not /Outer. One scope_join helper replaces nine format! sites.
  • Import-collision diagnostics report against the first-registered FQN
    (E1: diagnostics infrastructure #14) — which is the one ImportSet actually gives the simple name to. With
    three colliding imports the report previously named whichever was seen most
    recently, so it disagreed with the behaviour it was describing.

Documentation

KOTLIN_BANNER's doc said an override "falls back to" the constant, which is
backwards — the fallback applies when there is no override (#13).

One comment not acted on

#7 flagged that matches!(c.kind, KtClassKind::Value { .. }) "attempts to move
kind out of &KtClass, which does not compile". It does compile: the pattern
binds nothing, so nothing is moved. The code has been building and passing tests
since that commit.

Also already resolved by the chain

#14 flagged that check_top_level_names ignored KtDecl::FunInterface. Correct
at that commit — E1 was a deliberate verbatim move — and fixed two steps later
by B2 (#16), which replaced that function with the namespace-aware walker.

Verification

All 88 pre-existing tests pass. Thirteen added, covering every fix plus the
must-not-fire cases (val+var data properties accepted, externalish accepted).
101 tests, 10 doctests, cargo clippy --all-targets clean.

@milyin milyin mentioned this pull request Aug 6, 2026
15 tasks
milyin added 2 commits August 6, 2026 13:40
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.
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.
@milyin
milyin force-pushed the step/e2-rollout-and-docs branch from ce16ec3 to f13aef6 Compare August 6, 2026 11:40
@milyin
milyin force-pushed the step/f1-review-fixes branch from 18159e0 to 4856236 Compare August 6, 2026 11:40
@milyin
milyin changed the base branch from step/e2-rollout-and-docs to docs/validation-umbrella August 6, 2026 11:42
@milyin
milyin merged commit b7320d6 into docs/validation-umbrella Aug 6, 2026
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant