Repository navigation
Fix: generator crash when external constructor parameter names differ only by case - #30
Merged
Merged
Conversation
FactoryGenerator.csproj never set DevelopmentDependency=true, the
standard/documented setting for Roslyn analyzer/source-generator NuGet
packages. Without it, a project that only transitively depends on
FactoryGenerator (e.g. via a ProjectReference to another library that
itself references it directly) also gets the FactoryGenerator analyzer
registered on top of that project's own direct reference. Roslyn does not
deduplicate separate <Analyzer> items even when they load the same
generator, so both instances independently try to AddSource the same
generated-source hint names (e.g. FactoryGenerator.Declarations/{member}.g.cs
for an IContainer-typed constructor parameter). The second AddSource call
throws ArgumentException('...must be unique within a generator'), aborting
the whole generator run for that compilation with zero output - which is
what produces a cascading 'CS0234: ...Generated does not exist' in
consuming code, since the generator failed to emit anything.
DevelopmentDependency=true stops the analyzer from flowing to a
referencing project's own downstream consumers, regardless of whether
that project's own PackageReference sets PrivateAssets. Verified via
dotnet pack that the resulting nuspec now correctly emits
<developmentDependency>true</developmentDependency>. This setting has
never been present in this file's history (confirmed via git log), so
this is a pre-existing packaging gap, not something introduced by the
recently-merged perf work.
Also adds a regression test covering a related, previously-untested (but
safe) scenario: two separate injections both taking an IContainer-typed
external constructor parameter with the identical name - confirms that
case is correctly deduplicated within a single generator run and was not
itself the source of the crash.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CONFIRMED root cause, reproduced end-to-end against the real downstream
consumer's repository: Roslyn's SourceProductionContext.AddSource compares
hint names with StringComparer.OrdinalIgnoreCase (verified against
dotnet/roslyn's GeneratorDriver.cs and its own test suite), but
GenerateCode's declarations/�rrayDeclarations dictionaries used the
default (ordinal, case-SENSITIVE) comparer for their keys. Those keys
double as generated hint names AND valid C# identifiers - e.g. two
unrelated injections each taking an external IContainer-typed constructor
parameter named 'lifeTimeScope' and 'lifetimeScope' are both completely
valid on their own (C# identifiers are case-sensitive), so both survived
as separate dictionary entries, but then both independently tried to
become their own generated source file. Roslyn's case-insensitive
uniqueness check rejected the second one, throwing
ArgumentException('...must be unique within a generator') and aborting
the entire generator run for that compilation with zero output - the
exact reported regression (CS8785 warning, cascading CS0234 for the
missing Generated namespace).
This is a long-standing latent bug (the affected dictionaries have used
the default comparer since the project's initial commit), not something
introduced by the recently-merged perf work - it was just never
previously triggered by two same-name-different-case identifiers landing
in the same compilation.
Fix: since a partial class's members can be split across any number of
source files (or consolidated into one) with zero effect on the compiled
result, group declarations by a case-insensitive hint name and merge any
colliding entries into a single generated file instead of emitting one
AddSource call per dictionary key. This preserves every declaration (both
differently-cased members still get generated and remain independently
usable) rather than silently dropping one via a case-insensitive
dictionary overwrite, or crashing the whole generator.
Validated three ways:
1. Full test suite: 91/91 passing, including two new regression tests
(identical external-parameter names, and names differing only by
case).
2. Reproduced the exact crash live against the actual downstream
consumer's real WeConfig.Application project (confirmed two classes,
SessionFactory and Engine, with external IContainer parameters
'lifeTimeScope'/'lifetimeScope' reachable from the same compilation).
3. Patched the locally-cached FactoryGenerator analyzer DLL with this fix
and rebuilt WeConfig.Application end-to-end: build now succeeds with
0 errors (previously failed with the reported CS8785/CS0234).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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.
Update: confirmed root cause found and fixed
I initially suspected a missing
DevelopmentDependencypackaging setting (still included below as a valid best-practice fix), but I got direct access to the downstream consumer's repository and was able to reproduce the exact crash locally and confirm the true root cause end-to-end.Confirmed root cause
Roslyn's
SourceProductionContext.AddSourcecompares hint names withStringComparer.OrdinalIgnoreCase(verified directly againstdotnet/roslyn'sGeneratorDriver.csand its own test suite:Assert.Equal(expectedName, source.HintName, StringComparer.OrdinalIgnoreCase)), butGenerateCode'sdeclarations/arrayDeclarationsdictionaries used the default (ordinal, case-sensitive) comparer for their keys.Those dictionary keys double as both the generated hint name and a valid C# identifier. In the downstream consumer's real codebase, two completely unrelated classes each take an external
IContainer-typed constructor parameter:SessionFactory(..., IContainer lifeTimeScope)Engine(IContainer lifetimeScope)Both are entirely valid, independent C# — case-sensitive identifiers are allowed to differ only by case. Both survived as separate entries in the case-sensitive dictionary, and each independently tried to become its own generated source file via
context.AddSource. Roslyn's case-insensitive uniqueness check rejected the second one, throwingArgumentException("...must be unique within a generator")and aborting the entire generator run for that compilation with zero output — producing the exact reportedCS8785warning and the cascadingCS0234(missingGeneratednamespace).This is a long-standing latent bug — the affected dictionaries have used the default comparer since the project's very first commit — not something introduced by the recently-merged
perfwork. It was simply never previously triggered by two same-name-different-case identifiers landing in the same compilation.Fix
Since a partial class's members can be split across any number of source files (or consolidated into one) with zero effect on the compiled result, declarations are now grouped by a case-insensitive hint name, and any colliding entries are merged into a single generated file instead of one
AddSourcecall per dictionary key. This preserves every declaration — both differently-cased members are still generated and remain independently usable — rather than silently dropping one via a case-insensitive dictionary overwrite, or crashing the whole generator.Validation
WeConfig.Applicationproject — confirmedSessionFactory/EnginewithlifeTimeScope/lifetimeScopeare both reachable from the same compilation.WeConfig.Applicationend-to-end: build now succeeds with 0 errors (previously failed with the reportedCS8785/CS0234).Also included: analyzer packaging best practice
FactoryGenerator.csprojnever set<DevelopmentDependency>true</DevelopmentDependency>, the standard/documented setting for Roslyn analyzer/source-generator NuGet packages, which prevents the analyzer from flowing to a referencing project's own downstream consumers regardless of that project's ownPrivateAssetssetting. This wasn't the cause of this specific crash, but it's an unambiguously correct, low-risk improvement kept in this PR.