fix(mocks): escape C# keyword identifiers at all emit sites (#5679)

[thomhurst]

Summary

• Adds IdentifierEscaping.EscapeIdentifier and applies it at every emission site in MockImplBuilder, MockMembersBuilder, MockBridgeBuilder, MockEventsBuilder, and MockWrapperTypeBuilder where a member name becomes a C# identifier.
• Stored model Name values stay UNESCAPED, so engine dispatch keys, logging, and identity remain "event" / "get_class" / "namespace" rather than "@event" etc.
• Restores the SKIPPED T18_Member_Names_That_Are_Contextual_Keywords kitchen-sink test, with an IEscapedNames interface covering keyword-named property (@class), function (@record()), void method with keyword parameter (@event(@params)), and a method with two keyword-named parameters and a keyword-named method name (@namespace(@new, @static)) to exercise the joint escape paths.
• Dedupe pass: collapses the pre-existing private EscapeIdentifier helper in MemberDiscovery into the new shared utility so there is one keyword-escape implementation across the generator.

Why

Any interface or class that declared a member whose name happened to match a C# keyword (class, event, record, namespace, etc.) emitted unescaped identifiers at member declaration sites — producing a cascade of CS0539 / CS0106 / CS0066 / CS0708 / CS0542 errors and making the type unmockable. The EscapeIdentifier helper already existed at the parameter-discovery layer; this PR threads it through every member-name emission site.

Test plan

• T18 (TUnit.Mocks.Tests/KitchenSinkEdgeCasesTests.cs) passes
• TUnit.Mocks.Tests full suite (net10.0): 953/953
• TUnit.Mocks.SourceGenerator.Tests (net10.0): 45/45
• All 4 source-generator project variants build (base + Roslyn 4.14 / 4.4 / 4.7)
• Dispatch keys in generated factories remain unescaped ("event", "get_class", "namespace")

Closes #5679

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review: fix(mocks): escape C# keyword identifiers at all emit sites

Summary

The PR fixes a real compilation bug: interfaces/classes with members whose names are reserved C# keywords (e.g., class, event, namespace) produced unescaped identifiers at member-declaration sites in generated code, triggering CS0539/CS0106/CS0066/CS0708 errors. The fix extracts a shared IdentifierEscaping.EscapeIdentifier helper and applies it at every member-name emission site across all five builder classes.

---

What the PR Does Well

Correct bug fix, correctly scoped. Escaping is applied only at emission sites where a name becomes a C# identifier. String literals used as engine dispatch keys (e.g., "event", "get_class") intentionally remain unescaped — correct, since the mock engine resolves by raw symbol name.

Authoritative documentation in IdentifierEscaping.cs. The IMPORTANT paragraph makes the stored-unescaped/emit-escaped invariant explicit for future contributors — exactly the kind of comment that prevents regressions.

Clean deduplication. The pre-existing private EscapeIdentifier in MemberDiscovery was identical to the new utility. Collapsing to a single implementation via using static is the right move.

Parameter handling is self-consistent. Parameters are pre-escaped at model-creation time in MemberDiscovery, so they arrive in builders already carrying @. The builders don't double-escape — EscapeIdentifier("@event") returns "@event" unchanged because SyntaxFacts.GetKeywordKind("@event") is None.

Private field and method names are safe without escaping. _backing_{evt.Name} and Raise_{evt.Name} use the raw name as a suffix inside a larger identifier (e.g., _backing_event) — valid C# because the prefix prevents keyword collision. Correctly left unescaped.

---

Issues Found

1. Missing source-generator snapshot test (Medium)

The PR adds a runtime integration test (T18 in KitchenSinkEdgeCasesTests.cs) but adds no new snapshot test in TUnit.Mocks.SourceGenerator.Tests. The existing Interface_With_Keyword_Parameter_Names snapshot covers keyword-named parameters, but there is no snapshot for keyword-named members (the new scenario).

Without a snapshot, exact generated output for IEscapedNames is only validated at runtime. A future regression in emission would be caught only if the runtime test happens to exercise that path. A snapshot test in MockGeneratorTests.cs would pin the generated output and be far more targeted.

Suggested addition:

[Test]
public Task Interface_With_Keyword_Member_Names()
{
    var source = """
        using TUnit.Mocks;

        public interface ITest
        {
            int @class { get; }
            string @event();
            void @namespace(int @params);
        }

        public class TestUsage
        {
            void M() { var mock = Mock.Of<ITest>(); }
        }
        """;

    return VerifyGeneratorOutput(source);
}

2. Test name is semantically misleading (Low)

The test method is named T18_Member_Names_That_Are_Contextual_Keywords, but the keywords exercised are:

• class, event, namespace — reserved keywords (SyntaxFacts.GetKeywordKind returns non-None)
• record — a contextual keyword (GetKeywordKind returns None; it doesn't need @)

Contextual keywords (record, async, var, get, set, add, remove, init, etc.) are valid C# identifiers without the @ prefix — EscapeIdentifier correctly leaves them alone. Naming the test "contextual keywords" inverts the semantic truth and could mislead future contributors into thinking contextual keywords require escaping.

Suggested rename: T18_Member_Names_That_Are_Reserved_Keywords

3. IdentifierEscaping doc comment should note contextual keyword exclusion (Low)

The implementation correctly uses only SyntaxFacts.GetKeywordKind (not GetContextualKeywordKind), but a future maintainer might wonder "what about record?" and add unnecessary escaping. Adding a note would prevent that:

// Intentionally uses GetKeywordKind only (reserved keywords).
// Contextual keywords such as record, async, var, get, set are valid C# identifiers
// without @ and must NOT be escaped.

---

TUnit-Specific Concerns

• Dual-mode: Changes are isolated to TUnit.Mocks.SourceGenerator. Reflection path is unaffected. ✅
• AOT compatibility: No new reflection, no DynamicallyAccessedMembers concerns. ✅
• Performance: SyntaxFacts.GetKeywordKind is a pure enum lookup — O(1), allocation-free. ✅
• No .received.txt files in diff. ✅

---

Verdict

The core fix is correct and complete. Every member-name emission site across all five builder files has been updated, the implementation is self-consistent, and the dispatch-key invariant is preserved throughout.

Primary request before merging: Add a source-generator snapshot test (Interface_With_Keyword_Member_Names) to lock in the generated output — this is the standard TUnit verification pattern for source-gen changes and should not be skipped. The test name correction and doc-comment improvement are lower priority but worth addressing to avoid future confusion.

[thomhurst]

Addressed review feedback in f0cf2b4 (new commit on top, no amends):

1. Added snapshot test — Interface_With_Keyword_Member_Names in TUnit.Mocks.SourceGenerator.Tests/MockGeneratorTests.cs. Mixed coverage: keyword-named property (@class), keyword-named methods (@event, @namespace, plus contextual record), and keyword-named parameters (@params, @new, @static). New .verified.txt snapshot accepted; no .received.txt committed.

2. Renamed T18 to T18_Member_Names_That_Are_Reserved_Keywords in TUnit.Mocks.Tests/KitchenSinkEdgeCasesTests.cs. Kept record on IEscapedNames with a one-line comment noting it's a contextual keyword and is there to confirm contextual keywords pass through EscapeIdentifier unchanged.

3. Documented IdentifierEscaping — added a WHY-only comment above EscapeIdentifier explaining why only GetKeywordKind (reserved) is used, and that escaping contextual keywords like record/async/var/get/set would change identifier text and break explicit interface impl matching.

Test status (net10.0):

• TUnit.Mocks.SourceGenerator.Tests: 46/46 passed
• TUnit.Mocks.Tests (incl. T18 renamed): 953/953 passed

[claude]

Code Review: fix(mocks): escape C# keyword identifiers at all emit sites (#5679)

Summary

This is a well-executed, targeted fix. The core design decision — store model Name values unescaped (for dispatch keys, logging, and identity), and apply @-escaping only at code-generation emission sites — is the right architecture. Centralising the rule in IdentifierEscaping.EscapeIdentifier and threading it through all five builder classes is correct and complete.

---

Good

• Clean separation of concerns. Keeping Name on the models as-is (e.g. "event") and escaping only at the CodeWriter call sites is the correct invariant. Snapshot keys like "get_class", "namespace", and "event" must stay unescaped so the dispatch table is stable. The PR achieves this consistently.

• Deduplication. Removing the private EscapeIdentifier in MemberDiscovery and replacing it with the shared utility eliminates a real divergence risk. A future bug fix in one copy previously wouldn't have reached the other.

• Test coverage is comprehensive. The snapshot test provides a generator-level regression guard and the integration test (T18) proves end-to-end compile + dispatch correctness. Covering a joint keyword-named method + two keyword-named parameters (@namespace(@new, @static)) exercises the most complex joint-escape path.

• Contextual-keyword reasoning is correct and documented. The WHY only GetKeywordKind (reserved) comment in IdentifierEscaping.cs explains the important distinction between reserved keywords (event, class, namespace) and contextual keywords (record, async, get, set). The snapshot confirms record() is correctly left unescaped. This is a subtle point that's easy to get wrong.

---

Issue: EscapeIdentifier visibility mismatch

File: TUnit.Mocks.SourceGenerator/IdentifierEscaping.cs, line 28

// current
public static string EscapeIdentifier(string name) => ...

// preferred
internal static string EscapeIdentifier(string name) => ...

The enclosing class is internal static class IdentifierEscaping, so declaring the method public is misleading — the effective visibility is already internal. Since every caller uses using static TUnit.Mocks.SourceGenerator.IdentifierEscaping; within the same assembly, there's no functional difference, but the mismatch between the class modifier and the member modifier creates a minor readability debt. internal better expresses the intended scope.

---

Observation: GetWrapperName intentionally embeds the unescaped name in a type name

File: TUnit.Mocks.SourceGenerator/Builders/MockMembersBuilder.cs, line 138

private static string GetWrapperName(string safeName, MockMemberModel method)
    => $"{safeName}_{method.Name}_M{method.MemberId}_MockCall";

This produces class names like IEscapedNames_event_M2_MockCall — where event appears as a non-terminal substring of a larger token. The resulting identifier is valid C# because only standalone event is a keyword. This is intentionally not escaped and is correct. The snapshot confirms it compiles. Worth a comment noting the deliberate decision for the next reader, since it looks like a missed call site.

---

Observation: record as explicit interface implementation name

The snapshot output at the explicit interface proxy:

string global::IEscapedNames.record() => Object.record();

Per the C# spec, record is contextual — valid as a member name. Your tests confirm this compiles today. One thing to be aware of: if a future C# version promotes record to a reserved keyword (unlikely but not impossible), this will silently break. The IdentifierEscaping comment already documents the current decision. No action required, just noting the known future risk the PR is accepting.

---

Overall

Solid fix with the right architecture. The single change I'd suggest before merge is making EscapeIdentifier internal to match the class it lives in. The rest is clean.

[thomhurst]

Round-2 review feedback applied (commit 9fd285d3)

1. Visibility mismatch — applied.
IdentifierEscaping.EscapeIdentifier is now internal (was misleadingly public inside an internal static class).

2. GetWrapperName deliberate raw-name embedding — documented.
Added a WHY comment above MockMembersBuilder.GetWrapperName clarifying that method.Name is intentionally embedded raw: the result is a compound identifier like IFoo_event_M2_MockCall, where a keyword as a non-terminal substring of a larger token is valid C#. EscapeIdentifier is only required when the name would stand alone as a complete identifier in the emitted source.

3. record future-keyword risk — no action (flagged by reviewer as informational only).

Verification (net10.0)

• T18_Member_Names_That_Are_Reserved_Keywords — 1/1 passed
• TUnit.Mocks.Tests — 953/953 passed
• TUnit.Mocks.SourceGenerator.Tests — 46/46 passed
• No snapshot drift (no .received.txt produced).

[claude]

Code Review: fix(mocks): escape C# keyword identifiers at all emit sites (#5679)

Prior Review Follow-up

Both previous review rounds raised three items. All have been addressed in the current state:

• Snapshot test missing → Added as Interface_With_Keyword_Member_Names.verified.txt ✅
• Test name misleading ("contextual" vs "reserved") → Now correctly T18_Member_Names_That_Are_Reserved_Keywords ✅
• EscapeIdentifier visibility mismatch (class internal / method public) → Method is now internal static ✅
• GetWrapperName missing comment → Explanatory comment added above the method ✅
• Contextual-keyword doc comment → The WHY only GetKeywordKind (reserved) block in IdentifierEscaping.cs covers this ✅

---

What This PR Does Well

The core design is sound: keep model Name values unescaped (engine dispatch keys, logging, identity stay as "event", "get_class", "namespace") and escape only at the point where a name becomes a standalone C# identifier in generated source. Centralising the rule in IdentifierEscaping.EscapeIdentifier and threading it via using static across all five builder classes is the right move. The snapshot test locks in the exact generated output; the integration test (T18) proves end-to-end compile + dispatch correctness.

---

Remaining Observation

GetSafeMemberName now applies EscapeIdentifier to results fed into GetWrapperName

// MockMembersBuilder.cs
private static string GetSafeMemberName(string name)
    => EscapeIdentifier(MockMemberNames.Contains(name) ? name + _ : name);

private static string GetWrapperName(string safeName, MockMemberModel method)
    => $"{safeName}_{method.Name}_M{method.MemberId}_MockCall";

If a type were named with a reserved keyword (e.g. @event), GetSafeMemberName("event") would return "@event", and GetWrapperName would produce "@event_someMethod_M0_MockCall". In C#, @ at the start of an identifier is valid (the whole token event_someMethod_M0_MockCall is the name), so this compiles. It's an extreme edge case and not a blocking concern, but the comment on GetWrapperName currently only explains why method.Name is raw, not why safeName could potentially start with @. A one-liner would complete the picture for future readers.

Not a blocker — this is a theoretical corner case, and the practical test coverage already proves the common paths work correctly.

---

TUnit-Specific Checklist

• Dual-mode (source-gen + reflection): Changes are isolated to TUnit.Mocks.SourceGenerator ✅
• Snapshot workflow: .verified.txt committed, no .received.txt in diff ✅
• AOT: No new reflection or DynamicallyAccessedMembers concerns ✅
• Performance: SyntaxFacts.GetKeywordKind is an O(1) enum lookup ✅

---

Verdict

The fix is complete, the implementation is architecturally correct, and all prior review feedback has been incorporated. Ready to merge.

[thomhurst]

Doc clarification applied in b296be2cd: extended GetWrapperName comment to note that safeName may legitimately start with @ (when the member name is a reserved keyword), and that this is valid C# — the @ prefix applies to the entire compound identifier. Documentation-only; no logic change.

[claude]

Code Review: fix(mocks): escape C# keyword identifiers at all emit sites (#5679)

Prior Review Follow-up

All items raised across three previous review rounds have been addressed:

• Snapshot test missing → Interface_With_Keyword_Member_Names test + .verified.txt added to TUnit.Mocks.SourceGenerator.Tests ✅
• Test name misleading ("contextual" vs "reserved") → Correctly renamed T18_Member_Names_That_Are_Reserved_Keywords ✅
• EscapeIdentifier visibility mismatch → Method is now internal static to match the enclosing internal static class ✅
• GetWrapperName undocumented design decision → Comment added explaining that method.Name is intentionally raw (non-terminal substring = valid C# identifier) ✅
• safeName may start with @ → GetWrapperName comment extended to cover this edge case, noting the entire compound token is valid C# ✅
• Contextual-keyword reasoning → The WHY only GetKeywordKind (reserved) block in IdentifierEscaping.cs documents the decision and its implications for record, async, get, set, etc. ✅

---

Final Assessment

The fix is architecturally correct and implementation-complete:

• Emission/storage invariant is clean: model Name values stay unescaped for engine dispatch keys and logging; @ escaping is applied only at the point where a name becomes a standalone C# identifier in generated source.
• Deduplication is complete: the private copy in MemberDiscovery has been removed; one authoritative implementation in IdentifierEscaping is used via using static across all five builder classes.
• Test coverage is thorough: snapshot test locks in exact generated output; T18 integration test proves end-to-end compile + dispatch correctness, including the joint path (@namespace(@new, @static)).
• Edge cases handled without over-engineering: _backing_{evt.Name} and Raise_{evt.Name} correctly remain unescaped since a keyword as an identifier suffix (e.g. _backing_event) is a valid C# token.

TUnit checklist:

• Dual-mode: changes isolated to TUnit.Mocks.SourceGenerator, reflection path unaffected ✅
• Snapshot workflow: .verified.txt committed, no .received.txt in diff ✅
• AOT: no new reflection or DynamicallyAccessedMembers concerns ✅
• Performance: SyntaxFacts.GetKeywordKind is an O(1) enum lookup, allocation-free ✅

Ready to merge. This is a clean, well-reviewed fix.

[thomhurst]

Merged latest origin/main (commit 1b63f88).

Conflict: single hunk in TUnit.Mocks.SourceGenerator/Builders/MockMembersBuilder.cs at GetSafeMemberName.

• main (fix(mocks): disambiguate IEquatable<T>.Equals from object.Equals (#5675) #5680) added an ObjectMemberDisambiguations lookup mapping Equals/GetHashCode/ToString to their *Of aliases.
• this branch added an EscapeIdentifier(...) wrapper so keyword-named members get the @ prefix.

Resolution: combined both — the disambiguation lookup runs first (its renamed values like EqualsOf are not C# keywords, so escaping them is unnecessary), and the fallback path keeps EscapeIdentifier(...) around the MockMemberNames/raw-name branch:

private static string GetSafeMemberName(string name)
{
    if (ObjectMemberDisambiguations.TryGetValue(name, out var renamed))
        return renamed;
    return EscapeIdentifier(MockMemberNames.Contains(name) ? name + "_" : name);
}

EscapeIdentifier audit of main's new emit sites:

• GenerateIndexerExtensionMethods (fix(mocks): implement indexer in generated mock (#5676) #5683): emits Item/SetItem (fixed identifiers, no escaping needed) and weaves in indexer.Parameters[i].Name. Those parameter names are already escaped at the model layer in MemberDiscovery.cs (lines 594/737/777/872), so no extra escaping required at the emit site.
• *Of extension method names from ObjectMemberDisambiguations: hardcoded constants, never collide with C# keywords.
• using static TUnit.Mocks.SourceGenerator.IdentifierEscaping; import preserved.

Tests (net10.0):

• TUnit.Mocks.SourceGenerator.Tests: 47/47 passed (all snapshots, including Interface_With_Keyword_Member_Names and the new SelfEquatable_Generates_EqualsOf_GetHashCodeOf_ToStringOf).
• TUnit.Mocks.Tests KitchenSinkEdgeCasesTests/* filter: 20/20 passed (T18 included).
• No .received.txt files generated.

[claude]

Code Review: fix(mocks): escape C# keyword identifiers at all emit sites (#5679)

Prior Review Follow-up

All items raised across four previous review rounds have been addressed, and the merge-from-main conflict has been resolved cleanly:

• Snapshot test missing → Interface_With_Keyword_Member_Names test + .verified.txt added ✅
• Test name misleading ("contextual" vs "reserved") → Correctly renamed T18_Member_Names_That_Are_Reserved_Keywords ✅
• EscapeIdentifier visibility mismatch → Method is now internal static to match the enclosing class ✅
• GetWrapperName undocumented design decision → Comment added explaining raw embedding is intentional ✅
• safeName may start with @ → Comment extended to cover this edge case ✅
• Contextual-keyword reasoning → WHY only GetKeywordKind (reserved) block in IdentifierEscaping.cs documents the decision ✅

---

Merge Conflict Resolution

The merge-from-main introduced ObjectMemberDisambiguations (renaming Equals → EqualsOf, GetHashCode → GetHashCodeOf, ToString → ToStringOf) alongside indexer extension method generation. The conflict in GetSafeMemberName was resolved correctly:

private static string GetSafeMemberName(string name)
{
    if (ObjectMemberDisambiguations.TryGetValue(name, out var renamed))
        return renamed;
    return EscapeIdentifier(MockMemberNames.Contains(name) ? name + "_" : name);
}

The new disambiguation lookup runs first; the EscapeIdentifier path (added by this PR) is preserved and still fires for all other names. No escaping regression introduced by the merge.

---

Final Assessment

The fix is architecturally sound and implementation-complete:

• Emission/storage invariant is preserved: model Name values stay unescaped for engine dispatch and logging; @ escaping happens only at the point a name becomes a standalone C# identifier in generated source.
• Single implementation: the private copy in MemberDiscovery is removed; IdentifierEscaping.EscapeIdentifier is the one authoritative source via using static across all five builder classes.
• Coverage is thorough: snapshot test locks in exact generated output; T18 proves end-to-end compile + dispatch correctness including the joint path (@namespace(@new, @static)).
• Edge cases handled without over-engineering: _backing_{evt.Name} and compound type names correctly remain unescaped.

TUnit checklist:

• Dual-mode: changes isolated to TUnit.Mocks.SourceGenerator, reflection path unaffected ✅
• Snapshot workflow: .verified.txt committed, no .received.txt in diff ✅
• AOT: no new reflection or DynamicallyAccessedMembers concerns ✅
• Performance: SyntaxFacts.GetKeywordKind is O(1), allocation-free ✅

Ready to merge.