Refactor Generators: extract SourceEmitting namespace, decompose ExecutionRuntime, add 59 tests#90
Conversation
…or ExecutionRuntime - Create CSharpLiteralFormatter for value/key literal formatting - Create CSharpAccessibilityKeyword for accessibility mapping - Create CSharpTypeKeyword for type kind mapping - Create PartialMethodEmitData record and PartialMethodSourceEmitter - Create DummyImplementationEmitter with data records - Create RoslynSymbolDataMapper to bridge Roslyn types to emitter records - Create GeneratorAssemblyExecutor for shared compile/load logic - Create BodyGenerationDataExtractor for reflection-based data extraction - Refactor GeneratesMethodExecutionRuntime to use new classes - Refactor GeneratesMethodPatternSourceBuilder to delegate to SourceEmitting - Add documentation comments to all internal/public methods Co-authored-by: dex3r <3155725+dex3r@users.noreply.github.com> Agent-Logs-Url: https://github.com/dex3r/EasySourceGenerators/sessions/cb205769-20ee-4e1e-9b44-3ef890163968
- CSharpLiteralFormatterTests: 16 tests for value/key literal formatting - CSharpAccessibilityKeywordTests: 10 tests for From and FromOrEmpty - CSharpTypeKeywordTests: 5 tests for type kind mapping - PartialMethodSourceEmitterTests: 12 tests for partial method source emission - DummyImplementationEmitterTests: 11 tests for dummy implementation generation Co-authored-by: dex3r <3155725+dex3r@users.noreply.github.com> Agent-Logs-Url: https://github.com/dex3r/EasySourceGenerators/sessions/cb205769-20ee-4e1e-9b44-3ef890163968
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…n DummyMethodData
Use consistent MethodModifiers pattern ("partial" / "static partial") in
DummyMethodData, matching PartialMethodEmitData approach and eliminating
the fragile trailing-space convention.
Co-authored-by: dex3r <3155725+dex3r@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dex3r/EasySourceGenerators/sessions/cb205769-20ee-4e1e-9b44-3ef890163968
| /// Handles the compilation, assembly loading, and method invocation steps | ||
| /// required to execute generator methods at compile time in an isolated context. | ||
| /// </summary> | ||
| internal static class GeneratorAssemblyExecutor |
There was a problem hiding this comment.
Moved LoadedAssemblyContext to its own file LoadedAssemblyContext.cs in 174a3e2.
| string errors = string.Join("; ", emitResult.Diagnostics | ||
| .Where(diagnostic => diagnostic.Severity == DiagnosticSeverity.Error) | ||
| .Select(diagnostic => diagnostic.GetMessage())); |
There was a problem hiding this comment.
Error joining to string should be a separate, testable helper method (in helpers class).
Also, it is too big, so it probably does too many things.
There was a problem hiding this comment.
Extracted DiagnosticMessageHelper.JoinErrorDiagnostics() as a testable helper with 5 unit tests. Also extracted AbstractionsAssemblyResolver and DataGeneratorsFactorySetup from the class to reduce its scope. Done in 174a3e2.
| /// for <see cref="Accessibility.Private"/> and unrecognized values. | ||
| /// Used in contexts where the <c>private</c> keyword is implicit (e.g., dummy implementations). | ||
| /// </summary> | ||
| internal static string FromOrEmpty(Accessibility accessibility) |
There was a problem hiding this comment.
This method should be a switch in From method, not an entire new method.
Also, the name "From" sucks, this doesnt tell me anything:
CSharpAccessibilityKeyword.From.
There was a problem hiding this comment.
Merged into a single method CSharpAccessibilityKeyword.ToKeyword(accessibility, defaultToPrivate: true). The defaultToPrivate parameter controls whether unrecognized/private values return "private" (default) or "". Done in 174a3e2.
|
|
||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
This class is still too big, meaning it probably does too many things.
There was a problem hiding this comment.
Extracted AbstractionsAssemblyResolver.Resolve(), DataGeneratorsFactorySetup.Setup(), and InvokeStaticMethod() private helper. GeneratesMethodExecutionRuntime is now a thin orchestrator — ~170 lines down from ~276. Done in 174a3e2.
…ityKeyword - Move LoadedAssemblyContext into its own file - Extract DiagnosticMessageHelper for testable error-joining - Extract AbstractionsAssemblyResolver from GeneratesMethodExecutionRuntime - Extract DataGeneratorsFactorySetup from GeneratesMethodExecutionRuntime - Merge FromOrEmpty into ToKeyword with defaultToPrivate parameter - Add DiagnosticMessageHelperTests (5 tests) - Update CSharpAccessibilityKeywordTests for renamed API (12 tests) Co-authored-by: dex3r <3155725+dex3r@users.noreply.github.com> Agent-Logs-Url: https://github.com/dex3r/EasySourceGenerators/sessions/7c930db8-e3f7-42d9-98cd-41cf86cdc442
Co-authored-by: dex3r <3155725+dex3r@users.noreply.github.com> Agent-Logs-Url: https://github.com/dex3r/EasySourceGenerators/sessions/7c930db8-e3f7-42d9-98cd-41cf86cdc442
There was a problem hiding this comment.
Pull request overview
This PR refactors generator implementation code by separating Roslyn symbol handling from C# source string emission, decomposing the compile→load→execute runtime into smaller components, and adding unit tests for the new emitters/helpers.
Changes:
- Introduce
SourceEmittingnamespace with Roslyn-free emitters/formatters plus a Roslyn→data mapper. - Split the previous monolithic execution runtime into focused helpers (assembly compile/load, abstractions resolution, factory wiring, reflection extraction, diagnostic formatting).
- Add new unit tests covering source emission utilities and diagnostic formatting.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| EasySourceGenerators.Generators/SourceEmitting/RoslynSymbolDataMapper.cs | Maps Roslyn symbols to plain emit-data records (bridge layer). |
| EasySourceGenerators.Generators/SourceEmitting/PartialMethodSourceEmitter.cs | Emits partial method implementation source from PartialMethodEmitData. |
| EasySourceGenerators.Generators/SourceEmitting/PartialMethodEmitData.cs | Data record for partial method emission. |
| EasySourceGenerators.Generators/SourceEmitting/DummyImplementationEmitter.cs | Emits dummy partial implementations for execution compilations. |
| EasySourceGenerators.Generators/SourceEmitting/CSharpTypeKeyword.cs | Maps TypeKind to declaration keyword. |
| EasySourceGenerators.Generators/SourceEmitting/CSharpLiteralFormatter.cs | Formats values/keys as C# literals for emission. |
| EasySourceGenerators.Generators/SourceEmitting/CSharpAccessibilityKeyword.cs | Maps Accessibility to C# keywords. |
| EasySourceGenerators.Generators/IncrementalGenerators/LoadedAssemblyContext.cs | Encapsulates collectible ALC + loaded assembly lifecycle. |
| EasySourceGenerators.Generators/IncrementalGenerators/GeneratorAssemblyExecutor.cs | Builds execution compilation, emits/loads assembly in isolated ALC, provides reflection helpers. |
| EasySourceGenerators.Generators/IncrementalGenerators/GeneratesMethodPatternSourceBuilder.cs | Refactors simple pattern source generation to delegate to SourceEmitting. |
| EasySourceGenerators.Generators/IncrementalGenerators/GeneratesMethodGenerator.cs | Adds docs; generator entry remains the same. |
| EasySourceGenerators.Generators/IncrementalGenerators/GeneratesMethodGenerationTargetCollector.cs | Adds docs; target collection/validation remains the same. |
| EasySourceGenerators.Generators/IncrementalGenerators/GeneratesMethodGenerationPipeline.cs | Adds docs; pipeline now calls refactored runtime/emitter components. |
| EasySourceGenerators.Generators/IncrementalGenerators/GeneratesMethodExecutionRuntime.cs | Becomes a thin orchestrator delegating to new helper classes. |
| EasySourceGenerators.Generators/IncrementalGenerators/DiagnosticMessageHelper.cs | Extracted helper for joining error diagnostics. |
| EasySourceGenerators.Generators/IncrementalGenerators/DataGeneratorsFactorySetup.cs | Wires DataGeneratorsFactory into Generate.CurrentGenerator in execution context. |
| EasySourceGenerators.Generators/IncrementalGenerators/BodyGenerationDataExtractor.cs | Extracts fluent body return data via reflection. |
| EasySourceGenerators.Generators/IncrementalGenerators/AbstractionsAssemblyResolver.cs | Resolves abstractions assembly from compilation references (file/in-memory). |
| EasySourceGenerators.GeneratorTests/PartialMethodSourceEmitterTests.cs | Tests for PartialMethodSourceEmitter. |
| EasySourceGenerators.GeneratorTests/DummyImplementationEmitterTests.cs | Tests for DummyImplementationEmitter. |
| EasySourceGenerators.GeneratorTests/DiagnosticMessageHelperTests.cs | Tests for DiagnosticMessageHelper. |
| EasySourceGenerators.GeneratorTests/CSharpTypeKeywordTests.cs | Tests for CSharpTypeKeyword. |
| EasySourceGenerators.GeneratorTests/CSharpLiteralFormatterTests.cs | Tests for CSharpLiteralFormatter. |
| EasySourceGenerators.GeneratorTests/CSharpAccessibilityKeywordTests.cs | Tests for CSharpAccessibilityKeyword. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (assemblyName.Name != null && compilationReferenceBytes.TryGetValue(assemblyName.Name, out byte[]? bytes)) | ||
| { | ||
| Assembly loaded = context.LoadFromStream(new MemoryStream(bytes)); |
There was a problem hiding this comment.
In the AssemblyLoadContext.Resolving handler, assemblies are loaded from new MemoryStream(bytes) without disposing the stream. Per AssemblyLoadContext.LoadFromStream guidance, the caller should own/dispose the stream; leaking streams here can add avoidable memory pressure when many resolutions occur. Consider wrapping the MemoryStream in a using (or reusing a pooled stream) before calling LoadFromStream.
| Assembly loaded = context.LoadFromStream(new MemoryStream(bytes)); | |
| using MemoryStream assemblyStream = new(bytes); | |
| Assembly loaded = context.LoadFromStream(assemblyStream); |
| if (context.CompilationReferenceBytes.TryGetValue(Consts.AbstractionsAssemblyName, | ||
| out byte[]? abstractionBytes)) | ||
| { | ||
| Assembly assembly = context.LoadContext.LoadFromStream(new MemoryStream(abstractionBytes)); |
There was a problem hiding this comment.
LoadFromCompilationReference loads the abstractions assembly via new MemoryStream(abstractionBytes) but never disposes the stream. Disposing the stream after LoadFromStream avoids unnecessary allocations/retained buffers during generator execution. Consider using a using around the MemoryStream.
| Assembly assembly = context.LoadContext.LoadFromStream(new MemoryStream(abstractionBytes)); | |
| using var stream = new MemoryStream(abstractionBytes); | |
| Assembly assembly = context.LoadContext.LoadFromStream(stream); |
| internal static string FormatKeyAsLiteral(object key, TypeKind? typeKind, string? typeDisplayString) | ||
| { | ||
| if (typeKind == TypeKind.Enum) | ||
| { | ||
| return $"{typeDisplayString}.{key}"; | ||
| } | ||
|
|
||
| return key switch | ||
| { | ||
| bool b => b ? "true" : "false", | ||
| // SyntaxFactory.Literal handles escaping and quoting (e.g. "hello" → "\"hello\"") | ||
| string s => SyntaxFactory.Literal(s).Text, | ||
| _ => key.ToString()! | ||
| }; |
There was a problem hiding this comment.
FormatKeyAsLiteral falls back to key.ToString() for non-bool/non-string cases. This can emit invalid C# for common key types (e.g., char becomes A instead of 'A'), and floating-point/decimal formatting can become culture-dependent (comma vs dot) leading to non-compilable generated code on some locales. Consider adding explicit handling for char (and other primitives) via SyntaxFactory.Literal(...) / invariant formatting, and guard against typeDisplayString being null in the enum branch.
| PropertyInfo? currentGeneratorProperty = generatorStaticType.GetProperty( | ||
| Consts.CurrentGeneratorPropertyName, BindingFlags.NonPublic | BindingFlags.Static); | ||
| currentGeneratorProperty?.SetValue(null, dataGeneratorsFactory); | ||
|
|
There was a problem hiding this comment.
Setup claims to return an error when required properties can't be found, but it currently silently succeeds if Generate.CurrentGenerator isn't found (currentGeneratorProperty is null) or if Activator.CreateInstance returns null. This can lead to confusing downstream failures when executing fluent generators. Consider validating that the property exists and is settable, and return a descriptive error if wiring fails.
| PropertyInfo? currentGeneratorProperty = generatorStaticType.GetProperty( | |
| Consts.CurrentGeneratorPropertyName, BindingFlags.NonPublic | BindingFlags.Static); | |
| currentGeneratorProperty?.SetValue(null, dataGeneratorsFactory); | |
| if (dataGeneratorsFactory is null) | |
| { | |
| return | |
| $"Could not create an instance of {Consts.DataGeneratorsFactoryTypeFullName}."; | |
| } | |
| PropertyInfo? currentGeneratorProperty = generatorStaticType.GetProperty( | |
| Consts.CurrentGeneratorPropertyName, BindingFlags.NonPublic | BindingFlags.Static); | |
| if (currentGeneratorProperty is null) | |
| { | |
| return | |
| $"Could not find static property {Consts.CurrentGeneratorPropertyName} on {Consts.GenerateTypeFullName}."; | |
| } | |
| if (!currentGeneratorProperty.CanWrite) | |
| { | |
| return | |
| $"Property {Consts.GenerateTypeFullName}.{Consts.CurrentGeneratorPropertyName} is not writable."; | |
| } | |
| if (!currentGeneratorProperty.PropertyType.IsAssignableFrom(dataGeneratorsFactoryType)) | |
| { | |
| return | |
| $"Property {Consts.GenerateTypeFullName}.{Consts.CurrentGeneratorPropertyName} is of type {currentGeneratorProperty.PropertyType.FullName}, which is not assignable from {Consts.DataGeneratorsFactoryTypeFullName}."; | |
| } | |
| currentGeneratorProperty.SetValue(null, dataGeneratorsFactory); |
The
IncrementalGeneratorsnamespace had monolithic classes mixing Roslyn concerns with string emission, significant duplication between execution paths, and no unit tests for source output logic.New
SourceEmittingnamespaceExtracts all C# source string-building into small, focused, Roslyn-free classes testable with plain data records:
CSharpLiteralFormatter— value/key → C# literal (string→"string",bool→true, enums →Type.Value)CSharpAccessibilityKeyword/CSharpTypeKeyword— enum → keyword mappers (ToKeyword(accessibility, defaultToPrivate)single method with parameter instead of separate methods)PartialMethodSourceEmitter— emits complete partial method source files fromPartialMethodEmitDataDummyImplementationEmitter— emits dummy partial implementations for execution compilationsRoslynSymbolDataMapper— thin bridge:INamedTypeSymbol/IMethodSymbol→ emit data recordsDecomposed
GeneratesMethodExecutionRuntimeThe 466-line class with duplicated compile→load→execute logic is split into:
GeneratorAssemblyExecutor— shared compilation,AssemblyLoadContextsetup, type/method resolutionLoadedAssemblyContext(IDisposable) — owns the isolated ALC lifecycle (own file)BodyGenerationDataExtractor— reflection-basedFluentBodyResultextractionAbstractionsAssemblyResolver— resolves the abstractions assembly from compilation references (file-based and in-memory)DataGeneratorsFactorySetup— wires up theDataGeneratorsFactorytoGenerate.CurrentGeneratorDiagnosticMessageHelper— testable helper for joining error diagnostics into stringsGeneratesMethodExecutionRuntime— now a thin orchestrator (~170 lines) delegating to the aboveTests & docs
SourceEmittingclasses andDiagnosticMessageHelper/// <summary>on all internal/public methodsOriginal prompt
📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.