Conversation
…built-in type arguments (#5403) The mock source generator produced invalid class names containing `global::` when generic type arguments were non-built-in types (enums, classes, etc.). For example, `IFoo<SomeEnum>` generated `IFoo_global::Sandbox_SomeEnum_Mock` which fails to compile. Rewrote `SanitizeIdentifier` to use a robust character-based approach that strips all `global::` prefixes and replaces any non-identifier character with `_`. Also strips same-namespace qualifications from generic type arguments for cleaner names (`IFoo_SomeEnum_Mock` instead of `IFoo_Sandbox_SomeEnum_Mock`). Added 5 snapshot tests and 8 functional tests covering enum, class, nested namespace, multiple non-built-in, and nested generic type arguments.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 minor |
| CodeStyle | 3 minor |
🟢 Metrics 17 complexity
Metric Results Complexity 17
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Code Review: fix/generic-mock-class-name-5403
Summary
Clean, well-targeted fix for #5403. The root cause diagnosis is accurate, the fix is sound, and the test coverage is excellent (5 snapshot + 8 functional tests). No blocking issues.
The Core Fix
Old SanitizeIdentifier — chain of .Replace() calls that never stripped global:: from inner type arguments, leaving identifiers like IFoo_global::Sandbox_SomeEnum_Mock (invalid C#).
New SanitizeIdentifier — single global:: strip upfront, then a character-whitelist scan with inline double-underscore collapsing. This is strictly better: more robust, handles any special character, and is easier to reason about.
GetShortSafeName change — adds name.Replace("global::" + model.Namespace + ".", "") to strip same-namespace qualifications from generic type args. Correct and produces readable names (IFoo_SomeEnum_Mock instead of IFoo_Sandbox_SomeEnum_Mock).
Observations
1. Trailing underscore from > is intentional (not a bug)
SanitizeIdentifier("IFoo<Bar>") produces IFoo_Bar_ (trailing _ from the >). The calling code then appends Mock directly, yielding IFoo_Bar_Mock — confirmed correct in snapshots. This is fine; just noting it since it's non-obvious.
2. Double underscore in extension class names is pre-existing
The MockMemberExtensions class names like Sandbox_IFoo_Sandbox_Bar__MockMemberExtensions have a double underscore because GetCompositeSafeName ends with _ (from >) and the appended suffix starts with _Mock. This wasn't introduced by this PR (the old code had the same trailing-underscore pattern), but it is still there. Worth a note for future cleanup if the naming convention is ever revisited.
3. Namespace-strip heuristic: potential (theoretical) collision
The replacement:
name = name.Replace("global::" + model.Namespace + ".", "");strips all occurrences of the outer namespace prefix. This means a type in a sub-namespace also gets partially stripped — e.g., global::Sandbox.Sub.Bar → Sub.Bar when namespace is Sandbox. That produces IFoo_Sub_Bar_Mock, which is still distinct and correct.
The edge case worth noting: if two types share the same unqualified name but live in different sub-namespaces of the same outer namespace — e.g., Sandbox.Alpha.Widget and Sandbox.Beta.Widget — both strip to Widget, producing colliding mock names IFoo_Widget_Mock in the same generated namespace. This is unlikely in practice (the compiler itself would usually force disambiguation), but it's a latent correctness gap if someone has that naming structure.
A more defensive alternative would be to only strip when the type is directly in the interface's namespace (i.e., require no additional dots after stripping), but the current behaviour is probably the right trade-off for readable names.
4. System type names stay verbose
IProvider<List<Item>> → IProvider_System_Collections_Generic_List_Item_Mock. Not wrong, and consistent with keeping cross-namespace qualifications, but verbose. If there's future appetite for improving this, well-known system prefixes (System.Collections.Generic., System.Threading.Tasks., etc.) could be stripped or aliased. Out of scope for this fix, but the architecture supports it.
Test Coverage
Snapshot tests cover all the cases described in the PR table. Functional tests exercise the full mock lifecycle (creation → setup → invocation → verification). Regression baseline of 756 existing tests passing gives high confidence.
Verdict
Approve. The fix is correct, well-tested, and the implementation is cleaner than what it replaces. The observations above are minor and don't block merging.
There was a problem hiding this comment.
Clean, well-targeted fix. The character-whitelist SanitizeIdentifier is strictly better than the old chain of replaces, and the same-namespace stripping in GetShortSafeName correctly produces readable names. Test coverage is excellent (5 snapshot + 8 functional tests, 756 existing passing).
Summary
Fixes #5403
SanitizeIdentifierdidn't handleglobal::prefixes inside generic type arguments. ForIFoo<SomeEnum>, the FQNglobal::Sandbox.IFoo<global::Sandbox.SomeEnum>had only the leadingglobal::stripped — inner ones survived into class names asIFoo_global::Sandbox_SomeEnum_Mock, which is invalid C#.SanitizeIdentifierto use a character-based whitelist approach (letters, digits,_) that handles any special character. Also strips same-namespace qualifications from generic type arguments for cleaner names.IFoo_SomeEnum_Mockinstead ofIFoo_global::Sandbox_SomeEnum_MockIFoo<SomeEnum>IFoo_global::Sandbox_SomeEnum_MockIFoo_SomeEnum_MockIFoo<Bar>IFoo_global::Sandbox_Bar_MockIFoo_Bar_MockIMapper<Entity, Status>IMapper_global::…_Entity_global::…_Status_MockIMapper_Entity_Status_MockIService<Outer.Inner.Config>global::)IService_Outer_Inner_Config_MockIRepository<string>IRepository_string_MockIRepository_string_Mock(unchanged)Test plan