Round-2 shared-code extraction for the CLI generators#2431
Closed
Sergio0694 wants to merge 15 commits into
Closed
Conversation
'WinRT.Impl.Generator' and 'WinRT.Projection.Generator' each ship a copy of 'SignatureComparerExtensions' that has zero call sites in their own project. The real consumer is 'WinRT.Interop.Generator', which has its own (larger, strict-superset) copy at 'src/WinRT.Interop.Generator/Extensions/SignatureComparerExtensions.cs'. The two dead copies appear to have been left behind during earlier refactors. Delete them. No behavioral change is possible since they had no callers; the Interop copy is untouched and continues to serve its local consumers. Net diff: -141 LOC across 2 files. Both projects build with 0 warnings in Release. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extend 'DebugReproPacker' with three orchestration helpers that capture
the preamble/tail boilerplate that all 5 generators' 'SaveDebugRepro'
and 'UnpackDebugRepro' methods used to repeat verbatim:
* 'BeginSave<TError>(directory, toolName, archiveFileName)' validates
the user-provided directory exists (throws the per-tool
'TError.DebugReproDirectoryDoesNotExist' via the shared
'IGeneratorErrorFactory' contract), builds the target '.zip' path
and creates a fresh '{toolName}-debug-repro-{Guid}' staging dir.
* 'FinalizeSave(tempDirectory, zipPath)' deletes any pre-existing
archive, zips the staging dir, and deletes the staging dir.
* 'CreateUnpackTempDirectory(toolName)' creates a fresh
'{toolName}-debug-repro-unpack-{Guid}' directory.
Each '*Generator.DebugRepro.cs' now calls these helpers in place of
the inlined boilerplate. Per-tool concerns stay per-tool: subdirectory
layout, file-copy strategy, '.rsp' build, and JSON path-map writes
are all unchanged. Per-tool exception identity is fully preserved
because every 'BeginSave<TError>' call binds to the tool's own
'WellKnown*Exceptions' static-abstract factory.
Net diff: 6 files, +103 / -140 LOC (-37 LOC). All 5 generators build
clean and pass end-to-end debug-repro validation (ref-gen: 8/8 0 diffs,
WinMD: 16-byte MVID-only delta, proj-gen: 327/327 0 diffs, impl-gen:
0 byte diffs, interop-gen: smoke OK).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
'Impl', 'Interop', and 'WinMD' each shipped their own copy of 'RuntimeContextExtensions.LoadModule', with bodies that are character-for-character identical (only the receiver parameter name differed). Impl had only the 'PEImage' overload, Interop had only the 'string' overload, WinMD had both. Consolidate to a single file under 'WinRT.Generator.Core' that carries both overloads. All three consumer projects pick up the overload they already used; no behavioral change. This adds an 'AsmResolver.DotNet' package reference to 'WinRT.Generator.Core' (all 5 generators already reference AsmResolver directly). Net diff: 6 files, +63 / -126 LOC (-63 LOC). All 5 generators build with 0 warnings and pass end-to-end debug-repro validation (ref-gen: 8/8 0 diffs, WinMD: 16-byte MVID-only delta, proj-gen: 327/327 0 diffs, impl-gen: 0 byte diffs, interop-gen: smoke OK). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Every generator's 'Run' method historically opens with a single
'GeneratorHost.Prepare<TArgs>' call (which handles the shared
unpack/parse/save preamble) and then proceeds through a series of
phases (discovery, processing, emit, ...). Each phase was wrapped in
an identical:
try
{
ConsoleApp.Log("...");
DoPhase(args);
}
catch (Exception e) when (!e.IsWellKnown)
{
throw new UnhandledXxxException("phase-name", e);
}
That is now expressed as a single 'runner.RunPhase' call. The
'GeneratorPhaseRunner' readonly struct captures the per-tool
'wrapUnhandled' + 'log' delegates once (it is returned bound to them
by 'GeneratorHost.Prepare') and offers four overloads ('Action' /
'Func<T>', each with or without a log message). All four invariably
route through the per-tool 'wrapUnhandled' delegate so each tool
keeps throwing its own 'UnhandledXxxException' with the right phase
name.
'GeneratorHost.Prepare<TArgs>' now returns '(TArgs Args,
GeneratorPhaseRunner Runner)' instead of just 'TArgs'. The 5 'Run'
methods deconstruct the tuple and use the runner for each phase.
Impl's 'LoadOutputModule(args, out runtimeContext, out outputModule)'
becomes a tuple-return 'LoadOutputModule(args) -> (RuntimeContext,
ModuleDefinition)' so it fits the 'Func<T>' overload cleanly.
'ThrowIfCancellationRequested' calls stay per-tool at the call sites
(the pattern isn't uniform: some phases use 'args.Token', WinMD uses
'token', and the last phase typically has no check). ReferenceProjection's
second phase wraps to a well-known 'CsWinRTProcessError' rather than
the 'Unhandled' factory, so it's left as an inline try/catch.
Net diff across modified files: -84 LOC. Including the new
'GeneratorPhaseRunner.cs' (~123 LOC, mostly XML docs), the overall
file diff is +39 LOC but with a much lower density of error-handling
boilerplate at the per-tool call sites. All 5 generators build with
0 warnings and pass end-to-end debug-repro validation (ref-gen: 8/8
0 diffs, WinMD: 16-byte MVID-only delta, proj-gen: 327/327 0 diffs,
impl-gen: 0 byte diffs, interop-gen: smoke OK).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…orMessages
Each per-tool 'WellKnown*Exceptions' factory implements the same 6
logical errors from 'IGeneratorErrorFactory' with byte-identical
message text (5 of 6 strings are character-for-character identical
across all 5 tools; the sixth, 'ResponseFileReadError', only varies
by the embedded tool name).
Move the message templates to a new shared 'WellKnownGeneratorMessages'
class in 'WinRT.Generator.Core' and have each per-tool factory call
through to it. Per-tool error ID prefixes (e.g. 'CSWINRTIMPLGEN0001'
vs 'CSWINRTINTEROPGEN0028'), per-tool concrete exception types
('WellKnownImplException' / 'WellKnownInteropException' / ...) and
per-tool numeric IDs are all unchanged.
While here, replace the per-tool '<summary>' doc on each of these 6
methods with '<inheritdoc cref="IGeneratorErrorFactory.X(...)"/>'. The
per-tool summaries were byte-identical to the interface summaries, so
this is no info loss; it also avoids future drift.
Net diff: 6 files (5 modified + 1 new), -60 LOC across the per-tool
factories and +70 LOC for the new central messages file. The real win
is the divergence-protection: any future message tweak now lives in
one place. All 5 generators build clean and pass end-to-end debug-repro
validation (ref-gen: 8/8 0 diffs, WinMD: 16-byte MVID-only delta,
proj-gen: 327/327 0 diffs, impl-gen: 0 byte diffs, interop-gen: smoke OK).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both Impl and Interop shipped their own 'MvidGenerator' with
complementary overloads ('CreateMvid(Guid, Guid)' for Impl;
'CreateMvid(params IEnumerable<string>)' for Interop). Interop's
overload also depended on a local 'IncrementalHashExtensions.AppendData(Stream)'
extension whose only consumer was that one method.
Move both 'MvidGenerator' overloads into a single file in 'WinRT.Generator.Core'
and relocate 'IncrementalHashExtensions' alongside it. Each consumer continues
to call the exact overload it already used, with no behavioral change. The
impl-gen end-to-end output is byte-identical (257536 bytes, 0 byte diffs)
which proves the 'Guid+Guid' MVID computation is preserved exactly.
Net diff: 5 files changed, 2 new + 3 deleted. All 5 generators build with
0 warnings and pass end-to-end debug-repro validation (ref-gen: 8/8 0 diffs,
WinMD: 16-byte MVID-only delta, proj-gen: 327/327 0 diffs, impl-gen:
0 byte diffs, interop-gen: smoke OK).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 160-byte CsWinRT 3.0 strong-name public keys lived in several
inconsistent places across the generators:
* 'ImplValues.PublicKeyData' (Impl) - the SDK projection key, used 3x,
but the type/property name suggested it was a generic 'PublicKey'.
* 'InteropValues.WindowsSdkProjectionPublicKey[Data]' (Interop) -
the same SDK projection key, dead code (0 callers).
* 'InteropValues.CsWinRTPublicKey[Data]' (Interop) - the real CsWinRT
runtime key, used 1x.
* 'ProjectionGenerator.Emit.CsWinRTPublicKey' (Projection) - the SDK
projection key again, used 1x, but misnamed as the CsWinRT key.
The const-string forms ('PublicKey', 'CsWinRTPublicKey',
'WindowsSdkProjectionPublicKey') had 0 callers anywhere - leftover
dead code. The same 'B5FC90E7...' byte sequence appeared 3 times,
which is exactly the kind of duplication that risks accidental drift.
Consolidate into a single 'WellKnownPublicKeys' in 'WinRT.Generator.Core':
* 'WindowsSdkProjection' - the precompiled SDK projection assembly key
(used by Impl forwarder refs and by Projection's Roslyn delay-signing).
* 'CsWinRT' - the real CsWinRT 3.0 runtime release key (used by Interop
when emitting refs to 'WinRT.Runtime.dll').
Each live caller is repointed; Projection wraps the 'byte[]' as
'ImmutableArray<byte>' on the fly via 'ImmutableCollectionsMarshal'
for Roslyn's API. The dead-code declarations are deleted, including
the now-empty 'ImplValues' and 'InteropValues' files entirely.
The public key tokens ('Interop\WellKnownPublicKeyTokens.cs' with
6 entries + 'WinMD\WellKnownPublicKeyTokens.cs' with 1 entry) are
unioned into 'WinRT.Generator.Core\References\WellKnownPublicKeyTokens.cs'
(7 entries total: MSCorLib + the 6 from Interop).
Net diff: 10 modified files + 2 new + 4 deleted (16 files total),
-22 LOC. Removes the dead 'WindowsSdkProjectionPublicKey[Data]'
declarations from 'InteropValues' (~31 LOC), fixes the
'PublicKey'/'CsWinRTPublicKey'/'WindowsSdkProjection' naming
inconsistency, and centralizes the canonical byte sequences in one
file. The byte-identical impl-gen output is the strongest possible
confirmation that the consolidated keys match the originals exactly.
All 5 generators build with 0 warnings and pass end-to-end debug-repro
validation (ref-gen: 8/8 0 diffs, WinMD: 16-byte MVID-only delta,
proj-gen: 327/327 0 diffs, impl-gen: 0 byte diffs, interop-gen: smoke OK).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
'GeneratorHost.Prepare<TArgs>' previously returned a tuple
'(TArgs Args, GeneratorPhaseRunner Runner)', and each per-tool 'Run'
method captured the 'args' local in a closure for every phase body
('runner.RunPhase("phase", () => DoPhase(args, ...))').
Make 'GeneratorPhaseRunner' generic in 'TArgs', have it capture the
parsed args directly, and pass them to every body delegate. The
'RunPhase' overloads now take 'Action<TArgs>' / 'Func<TArgs, T>'
instead of 'Action' / 'Func<T>'. Per-tool exception identity is fully
preserved because the per-tool 'wrapUnhandled' delegate is still
invoked unchanged.
Phase bodies that only need 'args' now bind to a method group (e.g.
'body: Discover' instead of 'body: () => Discover(args)') and allocate
zero closures per invocation. Five phases across the five generators
benefit from this: WinMD 'Discover', Interop 'Discover', Projection
'ProcessReferences', ProjectionRef 'BuildWriterOptions', and Impl
'LoadOutputModule'. The remaining eight phases (which need additional
state besides 'args') still use lambdas, but with 'args' as an
explicit parameter rather than a captured local.
Rename 'GeneratorHost.Prepare<TArgs>' to 'GeneratorHost.CreateRunner<TArgs>'
since the method now exclusively returns the runner (no longer a
preamble-only operation that needs a separate "prepare" verb).
Net diff: 7 files, +83 / -67 LOC. All 5 generators build with
0 warnings and pass end-to-end debug-repro validation (ref-gen: 8/8
0 diffs, WinMD: 16-byte MVID-only delta, proj-gen: 327/327 0 diffs,
impl-gen: 0 byte diffs, interop-gen: smoke OK).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refactor response-file parsing and related generator messaging: - Consolidate and simplify WellKnownGeneratorMessages (make ResponseFileReadError a constant and clean up XML docs) and update per-tool exception factories to use the new message form. - Restructure ResponseFileParser: split line parsing into a map builder and a Populate method, adjust DynamicallyAccessedMembers annotations, remove nullable-inspection/NullabilityInfo usage, and simplify default-application logic. - Tidy ResponseFileBuilder formatting and annotations; minor comment and formatting cleanups. - Remove an unused PathExtensions.Normalize(ReadOnlySpan<char>) overload and add small GeneratorHost/GeneratorPhaseRunner code cleanups (target-typed new, pragma for warnings, reorder assignments). These changes reduce duplication, simplify trimming/reflection annotations for the linker, and clarify parsing behavior without changing external behavior.
Rename the UnhandledGeneratorException property from GeneratorDescription to GeneratorName and update the standardized ToString message to use the new name ("The CsWinRT {GeneratorName} generator..."). Adjust all per-tool unhandled exception types to provide GeneratorName values. Remove explicit generic type args from several GeneratorHost.CreateRunner calls (relying on inference). Also apply minor XML-doc and cref cleanups (simplify exception/type cref qualifications and projection writer cref) and a small doc fix in WindowsRuntimeExceptionExtensions.
The 'InternalsVisibleTo' items in 'src/WinRT.Generator.Core/Properties/AssemblyInfo.cs' are emitted by the .NET SDK directly from MSBuild '<InternalsVisibleTo>' items when they're declared in the project file, so there is no need for a hand-written 'AssemblyInfo.cs' to host them. Move the 5 entries (one per consuming generator) into 'WinRT.Generator.Core.csproj' and delete the now-empty 'Properties/AssemblyInfo.cs' (plus its empty parent folder). All 5 generators build with 0 warnings, confirming the SDK-emitted attributes still grant the same internals access. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Simplify references by removing unnecessary qualification prefixes: replace System.StringComparison with StringComparison and global::WindowsRuntime.ProjectionWriter.ProjectionWriter.Run with ProjectionWriter.ProjectionWriter.Run. This is a code cleanup across generator tasks (RunCsWinRTWinMDGenerator.cs, ProjectionGenerator.Generate.cs, ReferenceProjectionGenerator.cs) with no behavior change.
Convert GeneratorPhaseRunner<TArgs> to use C# primary-constructor syntax, removing explicit private fields and the manual constructor. Replace uses of _wrapUnhandled and _log with the positional parameters wrapUnhandled and log, and make Args return the positional args field. Add XML param docs for the new parameters. This is a refactor to reduce boilerplate without changing behavior.
Every per-tool 'Run' method previously placed a manual
'args.Token.ThrowIfCancellationRequested()' between consecutive
phases - 8 explicit calls across the 5 generators.
Bake that check into 'GeneratorPhaseRunner<TArgs>.RunPhase' (all 4
overloads): after the body completes successfully, the runner calls
'args.Token.ThrowIfCancellationRequested()' before returning. The
check sits OUTSIDE the 'try'/'catch' so the resulting
'OperationCanceledException' propagates without going through the
per-tool 'wrapUnhandled' delegate, which is already a no-op for it
('OperationCanceledException' is in 'IsWellKnown').
Per-tool 'Run' methods drop the 8 redundant
'runner.Args.Token.ThrowIfCancellationRequested()' calls; the last
phase of each generator now does one harmless extra cancellation
check before the success log message.
Net diff: 6 files, +18 / -18 LOC. All 5 generators build with 0 warnings
and pass end-to-end debug-repro validation (ref-gen: 8/8 0 diffs,
WinMD: 16-byte MVID-only delta, proj-gen: 327/327 0 diffs, impl-gen:
0 byte diffs, interop-gen: smoke OK).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename src/WinRT.Generator.Core/GeneratorPhaseRunner.cs to src/WinRT.Generator.Core/GeneratorPhaseRunner{TArgs}.cs. No source changes were made; this update clarifies the file name to reflect the generic TArgs type parameter.
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
Round-2 shared-code extraction for the CsWinRT CLI generators: pull
seven more shared helpers into
WinRT.Generator.Coreand tighten theGeneratorHostAPI. Driven by the validated merged report from amulti-agent analysis (5 agents: Opus 4.8, Opus 4.7 1M, Opus 4.6,
GPT-5.5, GPT-5.3-Codex).
Motivation
After the initial 6-phase
WinRT.Generator.Coreextraction, severalhelpers remained duplicated across the 5 generator projects, with a
few cases of outright dead code and naming inconsistencies that
invited future drift (most notably the same 160-byte SDK projection
strong-name key declared in 3 different files under 3 different names).
This PR cleans those up.
The per-tool
Runmethods also still carried significanttry/catch boilerplate around every phase. A shared
GeneratorPhaseRunner<TArgs>collapses each phase to a singleRunPhasecall while preserving per-tool exception identity exactly(same
Unhandled*Exceptionsubtype, same phase tag, same per-toolfactory).
Every commit was validated end-to-end via the debug-repro round-trip
harness, with byte-identical outputs everywhere a deterministic
comparison is possible.
Changes
Delete dead SignatureComparerExtensions copies in Impl + Projection:remove the two unused copies in
WinRT.Impl.GeneratorandWinRT.Projection.Generator(the real consumer is Interop, whichhas its own larger copy). Net -140 LOC.
Add debug-repro orchestration helpers to DebugReproPacker:extend
DebugReproPackerwith three orchestration helpers thatcapture the preamble/tail boilerplate every
SaveDebugReproandUnpackDebugRepropreviously repeated verbatim:BeginSave<TError>,FinalizeSave,CreateUnpackTempDirectory.Per-tool exception identity preserved via
IGeneratorErrorFactory.Move RuntimeContextExtensions.LoadModule to Core:consolidate Impl, Interop, and WinMD copies into a single Core file
with both overloads. Adds
AsmResolver.DotNetas a Core packagereference.
Add GeneratorPhaseRunner to wrap per-phase try/catch + logandMake GeneratorPhaseRunner generic in TArgs and capture the args itself:new
GeneratorPhaseRunner<TArgs>struct holds the parsed argsplus the per-tool
wrapUnhandledandlogdelegates. Each phasecall site collapses from a 6-9 line
try/catchblock to asingle
runner.RunPhase("phase", body)call.GeneratorHost.Prepareis renamed to
CreateRunnerand now returns just the runner.Phases that only need
argsbind to a method group with zeroclosure allocations.
Centralize IGeneratorErrorFactory message strings in WellKnownGeneratorMessages:the 6 shared logical errors defined by
IGeneratorErrorFactory(response-file parsing, debug-repro packaging, etc.) had byte-identical
message text across all 5 per-tool factories. Extract the templates
to a shared
WellKnownGeneratorMessagesand use<inheritdoc/>for the per-tool docs. Per-tool error IDs and concrete exception
types unchanged.
Consolidate MvidGenerator + IncrementalHashExtensions in Core:Impl and Interop each shipped complementary
MvidGeneratoroverloads; merge both into a single Core file. Interop's
IncrementalHashExtensions(sole consumer was that one MVIDmethod) moves to Core alongside it. Impl-gen end-to-end output stays
byte-identical (257536 bytes, 0 byte diffs).
Consolidate WellKnownPublicKeys + WellKnownPublicKeyTokens in Core:the same 160-byte
B5FC90E7...SDK projection strong-name keywas duplicated 3 times across
ImplValues, deadInteropValues.WindowsSdkProjectionPublicKeyData, and inline inProjectionGenerator.Emit. Consolidate into a singleWellKnownPublicKeysin Core, deleting the now-emptyImplValuesandInteropValuesfiles entirely. Public keytokens (previously split across Interop's 6-entry file and WinMD's
1-entry file) are unioned into a single Core file.
Validation
Every commit passes the end-to-end debug-repro round-trip:
ref-gen: 8/8 .cs files, 0 diffsWinMD: 5632/5632 bytes, 16-byte MVID-only delta (expected)proj-gen: 327/327 .cs files, 0 diffsimpl-gen: 257536/257536 bytes, 0 byte diffsinterop-gen:--helpsmoke test passesAll 5 generators build with 0 warnings (Release) and AOT-publish
cleanly for
win-arm64.Net impact
40 files changed, +559 / -787 LOC at the per-tool call sites
(net -228 LOC), with
WinRT.Generator.Corepicking up:DebugRepro/DebugReproPacker.cs(extended with 3 orchestration helpers)Errors/WellKnownGeneratorMessages.cs(new)Extensions/IncrementalHashExtensions.cs(moved from Interop)Extensions/RuntimeContextExtensions.cs(moved from WinMD)GeneratorHost.cs(renamedPrepare→CreateRunner, returns runner)GeneratorPhaseRunner.cs(new generic struct)Helpers/MvidGenerator.cs(moved + merged from Impl + Interop)References/WellKnownPublicKeyTokens.cs(union of Interop + WinMD)References/WellKnownPublicKeys.cs(new, replaces 3 duplicated declarations)