E1: diagnostics infrastructure - #14
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new validation/diagnostics infrastructure as a read-only pass on KtFile, and wires it into the merge/write path so generation can report all problems (with per-check severity policy) without making builders/rendering fallible.
Changes:
- Introduces
Check,Severity,Diagnostic, andValidationPolicy, plusKtFile::{validate, validate_with}. - Refactors
merge_files/write_filesto validate via the new pass, adds*_withvariants that support policy + warnings. - Adds tests covering multi-diagnostic reporting, policy downgrade/disable, warnings behavior, and write refusal on validation errors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/validate.rs | New validation pass implementation and public diagnostics/policy types. |
| src/file.rs | Integrates validation into merge/write flows; adds *_with APIs and WriteKotlinError::Validation. |
| src/lib.rs | Re-exports validation and new *_with APIs from the crate root. |
| src/tests.rs | Adds unit tests for diagnostics reporting and policy behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+208
to
+211
| let unique_required = matches!( | ||
| decl, | ||
| KtDecl::Class(_) | KtDecl::TypeAlias { .. } | KtDecl::Property(_) | ||
| ); |
Comment on lines
+231
to
+248
| fn check_extra_imports(file: &KtFile, d: &mut Diagnostics<'_>) { | ||
| let mut by_simple: BTreeMap<&str, &str> = BTreeMap::new(); | ||
| for imp in &file.extra_imports { | ||
| let simple = imp.rsplit_once('.').map(|(_, s)| s).unwrap_or(imp.as_str()); | ||
| if simple.chars().next().is_some_and(|c| c.is_lowercase()) { | ||
| continue; | ||
| } | ||
| if let Some(prev) = by_simple.insert(simple, imp.as_str()) { | ||
| if prev != imp.as_str() { | ||
| d.push( | ||
| Check::ImportCollision, | ||
| &file.package, | ||
| format!("import simple-name collision: `{prev}` and `{imp}`"), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
This was referenced Aug 6, 2026
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.
milyin
force-pushed
the
step/e1-diagnostics
branch
from
August 6, 2026 11:40
d439317 to
67466ee
Compare
milyin
force-pushed
the
step/c2-export-stranded
branch
from
August 6, 2026 11:40
069bd24 to
3c1dfab
Compare
milyin
changed the base branch from
step/c2-export-stranded
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 E1 of #6. Stacked on #13.
Adds the validation pass the rest of the umbrella hangs off, and moves the two
checks that already lived inside
merge_filesonto it without changing whatthey detect.
It stays out of the generation code
model.rs,render.rsandcode.rsare untouched. The builders stayinfallible — that is the crate's whole idiom — and
render()stays infallibletoo, because rendering a model you already know is broken is exactly what you
want while debugging a generator. Checks run on the write path instead.
merge_filesbecomes purely about merging; both checks moved out of it, and itnow merges first and validates the merged result, which is where the questions
"is this name duplicated in this package?" actually live.
Also lands D2: report everything
Previously
merge_filesreturnedErrat the first duplicate, so clearingthree of them took three build-fix-rebuild cycles. Now:
Why the policy exists
This crate is published and has a live consumer that regenerates on every
cargo build. Without a per-check escape hatch, a check that turns out tomisfire leaves that consumer no option but to pin an old version.
Checkis#[non_exhaustive]so later steps can add variants without breaking anyone.merge_files/write_fileskeep their existing signatures and denyeverything;
merge_files_with/write_files_withtake a policy and return thewarnings that survived, which is what E2's warn-first rollout will use.
Deliberately not fixed here
The
class Foo+val Foofalse positive (D1) is preserved. This PR is amove, not a behaviour change; B2 fixes it as part of the namespace split. Same
for functions and
Rawblocks going unchecked.Verification
All 60 pre-existing tests pass untouched. Five added: report-everything, policy
downgrade and off, warnings not stopping a merge, the error message listing each
diagnostic, and
write_filesrefusing to touch the output root when validationfails.