refactor: Extract Parser and Emitter to internal top-level classes#56
Merged
DrBarnabus merged 1 commit intomainfrom Feb 23, 2026
Merged
Conversation
Move Parser and Emitter from private nested classes within SourceGenerator to internal top-level classes, enabling direct unit testing of each class in isolation. - Extract Parser to internal sealed class in Parser.cs - Extract Emitter to internal sealed class in Emitter.cs - Add InternalsVisibleTo for CompositeKey.SourceGeneration.UnitTests - Exclude PolySharp ModuleInitializerAttribute polyfill to avoid type conflicts exposed by InternalsVisibleTo - Resolve CompositeKeyAttribute assembly reference by name to avoid ambiguity between CompositeKey and CompositeKey.SourceGeneration - Remove partial modifier from SourceGenerator class
WalkthroughThis PR extracts nested Parser and Emitter classes from SourceGenerator into standalone internal top-level classes, enabling direct unit testing. Adds InternalsVisibleTo declaration and updates configuration to support test project visibility. Moves ~1,700 lines of code from nested to top-level scope without altering functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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: 1
🧹 Nitpick comments (4)
src/CompositeKey.SourceGeneration/Parser.cs (3)
28-36:ReportDiagnosticispublicon aninternalclass — considerinternalvisibility.Since
Parserisinternal sealed,publicmethods on it are effectively internal. However, marking itinternal(or leaving it at the default for an internal class context) would be more intentional about the intended access scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration/Parser.cs` around lines 28 - 36, The ReportDiagnostic method on the internal sealed Parser class is declared public; change its visibility to internal to match the class scope and make intent explicit. Update the ReportDiagnostic method declaration (method name: ReportDiagnostic in Parser.cs) from public to internal, keeping the existing parameters and body unchanged, so the method is only visible within the assembly and consistent with the internal sealed Parser class.
577-624:ParseCompositeKeyAttributeValuesiterates all attributes but does not break after finding a match.The
Debug.Assertat line 585 will fire in debug builds if there are somehow twoCompositeKeyAttributeinstances, but in release builds the loop will silently overwriteattributeValues. Since the attribute should haveAllowMultiple = false, this is unlikely, but an earlybreakafter the match would make the intent clearer.♻️ Suggested change
if (SymbolEqualityComparer.Default.Equals(attributeData.AttributeClass, _knownTypeSymbols.CompositeKeyAttributeType)) + { attributeValues = TryExtractAttributeValues(attributeData); + break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration/Parser.cs` around lines 577 - 624, The loop in ParseCompositeKeyAttributeValues may continue after finding a matching CompositeKeyAttribute and overwrite attributeValues in release builds; after assigning attributeValues = TryExtractAttributeValues(attributeData) inside the foreach where SymbolEqualityComparer.Default.Equals(attributeData.AttributeClass, _knownTypeSymbols.CompositeKeyAttributeType) is true, add an immediate break to exit the loop (preserving the existing Debug.Assert) so the first-found attribute value is retained and no subsequent attributes can overwrite it.
15-15: Consider makingCompositeKeyAttributeValuesinternal.This record is only consumed by
Parser(internal) and is within an analyzer assembly (shipped withPrivateAssets=all), sopublicvisibility is broader than necessary. Changing tointernalwould reduce the public API surface of the analyzer assembly.♻️ Suggested change
-public sealed record CompositeKeyAttributeValues(string TemplateString, char? PrimaryKeySeparator, bool InvariantCulture); +internal sealed record CompositeKeyAttributeValues(string TemplateString, char? PrimaryKeySeparator, bool InvariantCulture);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration/Parser.cs` at line 15, The CompositeKeyAttributeValues record is unnecessarily public; change its declaration from public to internal so it becomes internal sealed record CompositeKeyAttributeValues(string TemplateString, char? PrimaryKeySeparator, bool InvariantCulture); and ensure any usages (e.g., in the internal Parser type) still compile—adjust accessibility only on the CompositeKeyAttributeValues declaration to reduce the analyzer assembly's public API surface.src/CompositeKey.SourceGeneration/Emitter.cs (1)
374-381: Hardcodedstackalloc Range[128]limits repeating property collections to ~128 items.When a collection has more than 128 items and uses the same separator as key delimiters,
ReadOnlySpan<char>.Splitwill pack the remainder into the last range. This leads to a confusingFormatExceptionrather than a clear error. While 128 is generous for typical usage, this is worth being aware of.If this is an intentional design constraint, consider adding a comment documenting the limit. If it should be unbounded, consider an
ArrayPool<Range>-based approach.Also applies to: 560-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration/Emitter.cs` around lines 374 - 381, The stackalloc fixed buffer (stackalloc Range[128]) can overflow for large repeating collections; replace it with a rented array from ArrayPool<Range> (e.g., var rented = ArrayPool<Range>.Shared.Rent(capacity); try { Span<Range> {partRangesVariableName} = rented; int {partCountVariableName} = {inputName}.{method}(...); ... } finally { ArrayPool<Range>.Shared.Return(rented); }) so the split can handle >128 parts; apply the same change for the other occurrence (lines ~560-568) and ensure the rented array is returned in a finally block and the Span is created from the rented array before calling the split.
🤖 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/Parser.cs`:
- Around line 532-540: The loop that checks sequential enum member values uses
Convert.ToUInt64(members[i].Value) which throws OverflowException for negative
underlying enum values; change the check to guard against negatives by either
testing if Convert.ToInt64(members[i].Value) < 0 or wrapping the
Convert.ToUInt64 call in a try/catch that catches OverflowException and sets
isSequentialFromZero = false and breaks; also compare against (ulong)i instead
of (uint)i for type consistency. Locate the loop using members[i].Value and the
isSequentialFromZero variable in Parser.cs and apply the defensive check so the
source generator degrades gracefully for negative enum values.
---
Nitpick comments:
In `@src/CompositeKey.SourceGeneration/Emitter.cs`:
- Around line 374-381: The stackalloc fixed buffer (stackalloc Range[128]) can
overflow for large repeating collections; replace it with a rented array from
ArrayPool<Range> (e.g., var rented = ArrayPool<Range>.Shared.Rent(capacity); try
{ Span<Range> {partRangesVariableName} = rented; int {partCountVariableName} =
{inputName}.{method}(...); ... } finally {
ArrayPool<Range>.Shared.Return(rented); }) so the split can handle >128 parts;
apply the same change for the other occurrence (lines ~560-568) and ensure the
rented array is returned in a finally block and the Span is created from the
rented array before calling the split.
In `@src/CompositeKey.SourceGeneration/Parser.cs`:
- Around line 28-36: The ReportDiagnostic method on the internal sealed Parser
class is declared public; change its visibility to internal to match the class
scope and make intent explicit. Update the ReportDiagnostic method declaration
(method name: ReportDiagnostic in Parser.cs) from public to internal, keeping
the existing parameters and body unchanged, so the method is only visible within
the assembly and consistent with the internal sealed Parser class.
- Around line 577-624: The loop in ParseCompositeKeyAttributeValues may continue
after finding a matching CompositeKeyAttribute and overwrite attributeValues in
release builds; after assigning attributeValues =
TryExtractAttributeValues(attributeData) inside the foreach where
SymbolEqualityComparer.Default.Equals(attributeData.AttributeClass,
_knownTypeSymbols.CompositeKeyAttributeType) is true, add an immediate break to
exit the loop (preserving the existing Debug.Assert) so the first-found
attribute value is retained and no subsequent attributes can overwrite it.
- Line 15: The CompositeKeyAttributeValues record is unnecessarily public;
change its declaration from public to internal so it becomes internal sealed
record CompositeKeyAttributeValues(string TemplateString, char?
PrimaryKeySeparator, bool InvariantCulture); and ensure any usages (e.g., in the
internal Parser type) still compile—adjust accessibility only on the
CompositeKeyAttributeValues declaration to reduce the analyzer assembly's public
API surface.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/CompositeKey.SourceGeneration.UnitTests/CompilationHelper.cssrc/CompositeKey.SourceGeneration/CompositeKey.SourceGeneration.csprojsrc/CompositeKey.SourceGeneration/Emitter.cssrc/CompositeKey.SourceGeneration/Parser.cssrc/CompositeKey.SourceGeneration/SourceGenerator.Emitter.cssrc/CompositeKey.SourceGeneration/SourceGenerator.Parser.cssrc/CompositeKey.SourceGeneration/SourceGenerator.cs
💤 Files with no reviewable changes (2)
- src/CompositeKey.SourceGeneration/SourceGenerator.Emitter.cs
- src/CompositeKey.SourceGeneration/SourceGenerator.Parser.cs
DrBarnabus
deleted the
refactor/extract-parser-emitter-to-internal-classes
branch
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
=======================================
Coverage 87.66% 87.66%
=======================================
Files 35 35
Lines 2099 2099
Branches 344 344
=======================================
Hits 1840 1840
Misses 157 157
Partials 102 102 ☔ 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
ParserandEmitterfromprivate sealednested classes withinSourceGeneratortointernal sealedtop-level classes, enabling direct unit testing in isolationInternalsVisibleTofor the test project with necessary PolySharp polyfill exclusion and assembly reference disambiguationChanges
Parser.cs— Extracted fromSourceGenerator.Parser.csasinternal sealed class Parserat namespace levelEmitter.cs— Extracted fromSourceGenerator.Emitter.csasinternal sealed class Emitterat namespace levelSourceGenerator.cs— Removedpartialmodifier, movedCompositeKeyAttributeFullNameconstant inlineCompositeKey.SourceGeneration.csproj— AddedInternalsVisibleTofor UnitTests project; excluded PolySharpModuleInitializerAttributepolyfill to prevent type conflicts exposed viaInternalsVisibleToCompilationHelper.cs— ResolvedCompositeKeyAttributeassembly reference by name to avoid CS0433 ambiguity betweenCompositeKeyandCompositeKey.SourceGenerationassembliesCloses #41
Summary by CodeRabbit