refactor: Introduce DiagnosticContext to replace Parser mutable state#57
Merged
DrBarnabus merged 2 commits intomainfrom Feb 23, 2026
Merged
refactor: Introduce DiagnosticContext to replace Parser mutable state#57DrBarnabus merged 2 commits intomainfrom
DrBarnabus merged 2 commits intomainfrom
Conversation
Replace the two mutable instance fields (_diagnostics and _location) on Parser with an explicit DiagnosticContext parameter, making diagnostic flow visible in method signatures instead of relying on closure capture. - Add DiagnosticContext class carrying CurrentLocation, Diagnostics list, and ReportDiagnostic method with existing fallback logic - Update Parser.Parse to return (GenerationSpec?, Diagnostics) tuple, eliminating mutable state from the Parser instance entirely - Pass DiagnosticContext through ParseTemplateStringIntoKeyParts and its local functions as an explicit parameter - Simplify SourceGenerator to use Parse return tuple directly - Add unit tests for DiagnosticContext covering location fallback, accumulation, and message arg passthrough
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughReplaces Parser's mutable parsing state with a new per-Parse DiagnosticContext; Parser.Parse now returns a tuple (GenerationSpec? GenerationSpec, ImmutableEquatableArray Diagnostics) and internal helpers propagate DiagnosticContext for diagnostic reporting. Adds unit tests for DiagnosticContext behaviour. Changes
Sequence Diagram(s)sequenceDiagram
participant SourceGenerator
participant Parser
participant DiagnosticContext
participant Compilation
SourceGenerator->>Parser: Parse(TypeDeclarationSyntax, SemanticModel, CancellationToken)
Parser->>DiagnosticContext: new DiagnosticContext(compilation)
Parser->>DiagnosticContext: set CurrentLocation (from target type)
Parser->>DiagnosticContext: call ReportDiagnostic(descriptor, location?, args)
DiagnosticContext->>Compilation: verify location.SourceTree ∈ Compilation?
alt location invalid or null
DiagnosticContext-->>Parser: use CurrentLocation as fallback
end
DiagnosticContext-->>Parser: record DiagnosticInfo
Parser-->>SourceGenerator: (GenerationSpec?, Diagnostics)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CompositeKey.SourceGeneration.UnitTests/DiagnosticContextTests.cs`:
- Around line 8-139: The test class DiagnosticContextTests is declared static
which prevents xUnit discovery; change the class declaration to a non-static
public class (remove the static modifier from DiagnosticContextTests) while
leaving individual [Fact] methods as static or instance methods as desired so
xUnit can run the tests; update any references relying on the class being static
if necessary.
In `@src/CompositeKey.SourceGeneration/DiagnosticContext.cs`:
- Around line 16-21: The method ReportDiagnostic currently relies on
Debug.Assert(CurrentLocation != null) which is removed in Release; add a runtime
guard that throws (e.g., InvalidOperationException or ArgumentNullException) if
CurrentLocation is null before falling back to using it, so callers can't
silently emit diagnostics without a location. Update the guard in
DiagnosticContext.ReportDiagnostic to check CurrentLocation and throw a clear
exception message (referencing CurrentLocation and ReportDiagnostic) before the
existing logic that substitutes location = CurrentLocation when location is null
or invalid.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/CompositeKey.SourceGeneration.UnitTests/DiagnosticContextTests.cssrc/CompositeKey.SourceGeneration/DiagnosticContext.cssrc/CompositeKey.SourceGeneration/Parser.cssrc/CompositeKey.SourceGeneration/SourceGenerator.cs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 87.66% 87.77% +0.11%
==========================================
Files 35 36 +1
Lines 2099 2103 +4
Branches 344 344
==========================================
+ Hits 1840 1846 +6
+ Misses 157 156 -1
+ Partials 102 101 -1 ☔ View full report in Codecov by Sentry. |
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
1 participant
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.
Summary
DiagnosticContextclass carryingCurrentLocation, aDiagnosticslist, and theReportDiagnosticmethod with existing fallback logic_diagnosticsand_location) onParser, making diagnostic flow visible in method signatures instead of relying on closure captureParser.Parseto return a(GenerationSpec?, ImmutableEquatableArray<DiagnosticInfo>)tuple, eliminating mutable state from theParserinstance entirelyDiagnosticContextas an explicit parameter throughParseTemplateStringIntoKeyPartsand its local functions (ToPropertyKeyPart,ToRepeatingPropertyKeyPart)Closes #42
Summary by CodeRabbit
Tests
Refactor