feat(analyzers): generated migration catalog (AL task #27) - #178
Conversation
…e strings Closes task #27 ("AL wave: rewrite analyzer docs generator (post-renumber)"). What landed (no AL package version bump — pure doc + internal tools change): New under tools/ANcpLua.Analyzers.DocsGenerator/ (docs-only, never shipped in the analyzer DLL because the 2.0.1 analyzer no longer emits AL0xxx; nothing at runtime needs the rename map): - AlIdMigrationCatalog.cs — 89 AL0xxx → AL1xxx rename rows hand-transcribed from eng/analyzer-renumber-plan.md §2. Validate() asserts structural invariants (no duplicate OldId/NewId, every NewId matches ^AL1[0-8]\d{2}$, every OldId matches ^AL\d{4}$). No hardcoded ExpectedCount — count is a consequence of the invariants, not a property worth asserting on its own. Public class (not internal) so the tests project can call Validate() via ProjectReference without InternalsVisibleTo — IVT would expose the tools Exe's top-level Program and collide with the tests project's own Program (CS0433). Public is safe because the tools assembly is never packed. - MigrationCatalogRenderer.cs — mirrors QYL's CatalogStatistics-driven section-array pattern (qyl repo's tools/Qyl.OpenTelemetry.SemanticConventions.Analyzers.DocsGenerator/ MigrationCatalogRenderer.cs:21-186). 6 sections: header, summary, completion audit, band breakdown table, Old→New mapping table sorted by NewId, regenerate footer. Orchestrator wiring (DocsGenerator.cs): - Compute MigrationCatalogStats once at Run() and thread to Audit/Check/ Generate. Mirrors the qyl repo's CatalogStatistics threading. - Generate step 3 writes docs/migration-catalog.md. - Check step 3 enforces drift detection on the same file. - Audit prints catalog stats (89 renames, 9 bands). RepoLayout.cs: new MigrationCatalogPath(repoRoot) accessor. Cosmetic fix in IndexDocsRenderer.cs (4 occurrences): - Replace post-renumber-stale "AL00xx–AL18xx" with "AL10xx–AL18xx". The AL00xx range no longer hosts this analyzer's IDs — sibling packages (AotReflection, ExtensibleEnumMirror, DiscriminatedUnion) own AL0xxx per eng/analyzer-renumber-plan.md §0. Each replacement has a "// renumber: bump if AL bands shift again" signpost above it. - Added a "See also" link to the new docs/migration-catalog.md. EnforceIdsRewriter.cs comment: "/// AL00XX:" placeholder → "/// AL####:" (the actual regex was already AL-prefix-agnostic; only the comment was stale). Mandatory unit test (the highest-leverage piece of the whole change): - tests/ANcpLua.Analyzers.Tests/AnalyzerConventionTests.cs gains AlIdMigrationCatalog_StructuralInvariants_Hold which runs Validate() on every CI build. Catches hand-transcription drift in the 89-row Entries array immediately, instead of only when someone runs --check on a dev machine. Generated docs/migration-catalog.md committed for --check drift gating. Verification: - dotnet build tools/ANcpLua.Analyzers.DocsGenerator/ --nologo -warnaserror -> 0 warnings, 0 errors - dotnet build tests/ANcpLua.Analyzers.Tests/ --nologo -warnaserror -> 0 warnings, 0 errors - dotnet run --project tests/ANcpLua.Analyzers.Tests/ -> 758 pass, 0 fail - dotnet run --project tools/ANcpLua.Analyzers.DocsGenerator/ -> emits 7 artifacts including the new docs/migration-catalog.md - dotnet run --project tools/ANcpLua.Analyzers.DocsGenerator/ -- --check -> all artifacts up to date (clean drift gate) - dotnet run --project tools/ANcpLua.Analyzers.DocsGenerator/ -- --audit -> 89 renames across 9 bands; counts match renumber-plan §1 exactly - grep -c AL00xx docs/ANcpLua.Analyzers.md -> 0 - grep -cE '^\| `AL0' docs/migration-catalog.md -> 89
|
@coderabbitai autofix |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 20 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The implementation of the migration catalog is robust and successfully meets all core acceptance criteria, including automated generation, structural validation, and CI synchronization checks. The code is well-structured and follows the project's convention of keeping documentation tools separate from the analyzer logic to minimize assembly bloat.
While the quality is up to standards, there are two primary areas for improvement before merging: ensuring the generated catalog is sorted by legacy IDs to facilitate easier user lookup, and making the CI file-check logic resilient to cross-platform line ending differences (CRLF vs LF). Additionally, enhancing the validation logic to verify that catalog entries actually exist in the current DescriptorCatalog would prevent the documentation from becoming out-of-sync with the implementation.
About this PR
- The system currently lacks a check to ensure that the 'NewId' entries in the migration catalog correspond to actual active rules in the project. This could lead to documentation drift over time.
Test suggestions
- Verify that the migration catalog contains no duplicate OldId or NewId entries.
- Verify that all NewId entries conform to the ^AL1[0-8]\d{2}$ regex, ensuring they fall within the correct 100-wide domain bands.
- Verify that the generator correctly renders the migration catalog markdown with accurate band counts and a sorted mapping table.
- Verify that the CI 'Check' mode correctly identifies if the migration catalog file on disk is missing or out of sync with the source data.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| // Post-renumber band: AL1000..AL1899 (9 bands of 100, per renumber-plan §1). | ||
| // Sibling packages (AotReflection, ExtensibleEnumMirror, DiscriminatedUnion) | ||
| // own slots inside AL0xxx — a leak into NewId means the renumber regressed. | ||
| var badNew = Entries.FirstOrDefault(e => !NewIdRegex.IsMatch(e.NewId)); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Update the Validate method to verify that each AlIdRename entry's NewId starts with the range prefix defined in its Band string (e.g. check that if Band starts with AL10, the NewId also starts with AL10).
| Console.Error.WriteLine($"Missing migration catalog: {Path.GetRelativePath(repoRoot, migrationPath)}"); | ||
| return 1; | ||
| } | ||
| if (!string.Equals(File.ReadAllText(migrationPath), MigrationCatalogRenderer.Render(migrationStats), StringComparison.Ordinal)) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The comparison is sensitive to line endings. Since the renderer forces \n (via .ReplaceLineEndings("\n")), you should normalize the file content before comparison to ensure cross-platform consistency.
| if (!string.Equals(File.ReadAllText(migrationPath), MigrationCatalogRenderer.Render(migrationStats), StringComparison.Ordinal)) | |
| if (!string.Equals(File.ReadAllText(migrationPath).ReplaceLineEndings("\n"), MigrationCatalogRenderer.Render(migrationStats), StringComparison.Ordinal)) |
| sb.AppendLine(); | ||
| sb.AppendLine("| Old ID | New ID | Title |"); | ||
| sb.AppendLine("| -- | -- | -- |"); | ||
| foreach (var e in AlIdMigrationCatalog.Entries.OrderBy(x => x.NewId, StringComparer.Ordinal)) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The mapping table should be sorted by OldId to facilitate easier lookup for users migrating from legacy configurations.
| foreach (var e in AlIdMigrationCatalog.Entries.OrderBy(x => x.NewId, StringComparer.Ordinal)) | |
| foreach (var e in AlIdMigrationCatalog.Entries.OrderBy(x => x.OldId, StringComparer.Ordinal)) |
|
|
||
| namespace ANcpLua.Analyzers.DocsGenerator; | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The XML documentation block for the catalog is misplaced. It should be moved from its current position above the AlIdRename record to its intended position above the AlIdMigrationCatalog class definition.
| { | ||
| sb.AppendLine("## Old → New mapping"); | ||
| sb.AppendLine(); | ||
| sb.AppendLine("Sorted by `NewId` — scan to find your current ID's predecessor."); |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Update the terminology to reflect that users are looking for the successor (replacement) of their current legacy ID.
| sb.AppendLine("Sorted by `NewId` — scan to find your current ID's predecessor."); | |
| sb.AppendLine("Sorted by `OldId` — scan to find your current ID's successor."); |
Triage Bot report
Threads marked |
1 similar comment
Triage Bot report
Threads marked |
Summary
Closes the persistent task #27 AL wave: rewrite analyzer docs generator (post-renumber). Lands two things together:
A generated
docs/migration-catalog.md— the 89-rowAL0xxx → AL1xxxrename map from the 2.0.0 break, sourced fromeng/analyzer-renumber-plan.md§2. Mirrors the QYL repo'sMigrationCatalogRendererpattern. Includes a band breakdown that matches §1's nine 100-wide domain bands exactly (Correctness 13, ASP.NET Core 10, Roslyn Utilities 21, Async 15, AOT 10, Roslyn-author 6, Package/version 7, Style 4, Agent gov 3).Post-renumber cleanup of 4 stale
AL00xx–AL18xxstrings inIndexDocsRenderer.cs(replaced withAL10xx–AL18xx+ a// renumber: bump if AL bands shift againsignpost), plus 1 stale comment inEnforceIdsRewriter.cs.No AL package version bump — this is a pure docs + internal tools change.
DoD coverage (per the approved plan)
AlIdMigrationCatalog.csplaced intools/, NOTsrc/— the 2.0.1 analyzer no longer emitsAL0xxx, so nothing runtime needs the rename map. Shipped DLL stays slim.ExpectedCount = 89.Validate()asserts structural invariants: no dupes onOldId/NewId,NewIdmatches^AL1[0-8]\d{2}$,OldIdmatches^AL\d{4}$.AnalyzerConventionTests.AlIdMigrationCatalog_StructuralInvariants_Hold— runs on every CI build, not just when someone runs--checkon a dev machine. Five lines, catches hand-transcription drift.Generate/Check, mirror of QYL's section numbering. Adding a future artifact stays a "one renderer class + one numbered step" change.--check,--audit, defaultGenerateall wired and verified locally.Why "public" instead of "internal + InternalsVisibleTo"
AlIdMigrationCatalogispublicbecause IVT from the tools Exe assembly to the tests Exe assembly would expose the tools project's top-levelProgramand trigger CS0433 in tests (same class name, same global namespace, both visible). Public on a tools-only type is safe — the tools assembly is never packed, never referenced by any consumer.(See commit message for the architectural-decision walkthrough.)
Test plan
dotnet build tools/ANcpLua.Analyzers.DocsGenerator/ --nologo -warnaserror→ 0/0dotnet build tests/ANcpLua.Analyzers.Tests/ --nologo -warnaserror→ 0/0dotnet run --project tests/ANcpLua.Analyzers.Tests/→ 758/758 passdotnet run --project tools/ANcpLua.Analyzers.DocsGenerator/emits all 7 artifacts includingdocs/migration-catalog.md--checkclean (drift gate works)--auditshows 89 renames × 9 bands matching renumber-plan §1grep -c AL00xx docs/ANcpLua.Analyzers.md→ 0grep -cE '^\| \AL0' docs/migration-catalog.md` → 89🤖 Generated with Claude Code