B2: redeclaration checked per scope, per namespace - #16
Merged
Conversation
15 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Implements Step B2 of the validation umbrella by replacing the old top-level-only duplicate check with a scope-aware redeclaration validator that tracks Kotlin’s separate namespaces (types / values / functions) and adds merge-time deduplication semantics for Raw blocks.
Changes:
- Replace
DuplicateDeclarationwithDuplicateType/DuplicateValue/DuplicateFunction/DuplicateRaw, and validate collisions per scope (file, class body, companion body). - Add function redeclaration detection keyed by
(name, parameter types as written)to allow overloads. - Deduplicate identical
Rawblocks duringmerge_files_with, and add tests covering the new rules (including negative/legal cases).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/validate.rs | Reworks redeclaration validation to be per-scope and per-namespace; adds function signature-based collision detection and new Check variants. |
| src/file.rs | Deduplicates identical Raw blocks during merge so only genuinely differing duplicates are surfaced by validation. |
| src/tests.rs | Adds targeted tests for nested scopes, namespaces, overload rules, ctor-property vs member collisions, fun interface duplicates, and Raw dedup behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+53
to
+56
| /// Two `Raw` blocks in one scope share a name. Identical ones are merged | ||
| /// rather than reported (see [`merge_files`](super::merge_files)), so this | ||
| /// means two different blocks are claiming one identity. | ||
| DuplicateRaw, |
Comment on lines
+338
to
351
| /// 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 = format!("{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 = format!("{inner}/{}", comp.name.as_deref().unwrap_or("Companion")); | ||
| check_scope(&comp.members, &[], &cscope, d); | ||
| } | ||
| } |
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.
milyin
force-pushed
the
step/b1-name-validity
branch
from
August 6, 2026 11:40
f572b22 to
28cfcd4
Compare
milyin
force-pushed
the
step/b2-scopes-and-namespaces
branch
from
August 6, 2026 11:40
4405e14 to
093b90d
Compare
milyin
changed the base branch from
step/b1-name-validity
to
docs/validation-umbrella
August 6, 2026 11:41
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.
Step B2 of #6. Stacked on #15. The core of the umbrella.
The duplicate check had three problems and one false positive.
It was looking where the problem isn't
It only ever examined top-level declarations. In the downstream consumer's
real output, functions overwhelmingly live inside classes and objects:
session.ktkeyexpr.ktpubsub.ktIt skipped two whole declaration kinds
fun interfacewas never checked — two identically named ones merged into abroken file silently. Functions were never checked either, on the grounds that
same-named functions might be overloads. True, but only when their parameter
types differ.
And it had a false positive (D1)
Classes, type aliases and properties went into one pool of names. Kotlin keeps
types and values in separate namespaces, so
class Fooandval Foomaycoexist — and were rejected. Fixed here as a consequence of the split.
The new rule
Walk the file, each class body and each companion body as its own scope. Per
scope, three tallies:
fun interfaces, type aliasesval/varconstructor parametersA plain non-property constructor parameter is constructor-local and declares
nothing, so it cannot collide with a member — tested.
Raw blocks get their own rule
Rawblocks are hoisted singletons keyed by name, so two fragments maylegitimately carry the same one — that is deduplication, not a mistake.
Identical blocks now collapse during merge; only genuinely differing blocks
sharing a name are reported. A hard error on any repeated name (the obvious
implementation) would have rejected generation that is correct today.
Documented limitation
There is no type resolver here, so parameter types are compared as written:
io.p.FooandFooare different keys even when they name the same type, andfun <T> f(x: T)does not collide withfun <R> f(x: R). The check thereforemisses some real duplicates. It is a net, not a proof — and a net that never
catches a fish it shouldn't. This is on the check's doc comment, not just here.
Verification
All 70 pre-existing tests pass. Ten added, half of them negative — the legal
shapes each check must not fire on: class + property + function sharing a name,
genuine overloads, a plain constructor parameter, a nested class as its own
scope, identical
Rawblocks merging.