fix(mocks): propagate [Obsolete] and null-forgiving raise dispatch (#5626)

[JohnVerheij]

Description

The mock generator's generated forwards and overrides did not carry the source member's [Obsolete] attribute, causing CS0612/CS0618 on Object.X() call sites and CS0672 on partial-mock overrides. This PR captures the [Obsolete] attribute (with message and IsError flag) during member discovery and emits it on every generated forward and override, including static abstract DIM emissions and per-accessor attributes for properties that mark only a getter or setter. Extension methods on Mock<T> are intentionally not marked so consumer test code remains warning-clean.

Additionally fixes CS8600/CS8604 in multi-arg event raise emission: the cast (T)__argArray[i] was missing the null-forgiving operator that the single-arg case at the same method already uses. Non-nullable event parameter types in multi-arg events triggered CS8600 at the cast and CS8604 at the downstream handler invocation. This PR makes the multi-arg case consistent with the single-arg case.

Related Issue

Fixes #5626

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Performance improvement
• Refactoring (no functional changes)

Checklist

Required

• I have read the Contributing Guidelines
• If this is a new feature, I started a discussion first and received agreement
• My code follows the project's code style (modern C# syntax, proper naming conventions)
• I have written tests that prove my fix is effective or my feature works

TUnit-Specific Requirements

• Dual-Mode Implementation: If this change affects test discovery/execution, I have implemented it in BOTH:
  • Source Generator path (TUnit.Core.SourceGenerator)
  • Reflection path (TUnit.Engine)
• Snapshot Tests: If I changed source generator output or public APIs:
  • I ran TUnit.Core.SourceGenerator.Tests and/or TUnit.PublicAPI tests
  • I reviewed the .received.txt files and accepted them as .verified.txt
  • I committed the updated .verified.txt files
• Performance: If this change affects hot paths (test discovery, execution, assertions):
  • I minimized allocations and avoided LINQ in hot paths
  • I cached reflection results where appropriate
• AOT Compatibility: If this change uses reflection:
  • I added appropriate [DynamicallyAccessedMembers] annotations
  • I verified the change works with dotnet publish -p:PublishAot=true

Testing

• All existing tests pass (dotnet test)
• I have added tests that cover my changes
• I have tested both source-generated and reflection modes (if applicable)

Additional Notes

This PR only touches TUnit.Mocks.SourceGenerator. Dual-mode parity does not apply — mocks are always source-generated, there is no reflection-mode mocking path. AOT does not apply — the generator emits static code at compile time, no runtime reflection added.

Snapshot tests Interface_With_Obsolete_Members and Interface_FluentUI_Shape_Nullable_Warnings cover the regression surface; a new AssertGeneratedCodeHasNoNullableWarnings helper in SnapshotTestBase acts as a forward-looking guard for CS86xx-family diagnostics.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

Summary

This PR fixes two bugs in : (1) attributes on interface/class members were not propagated to generated forwards and overrides, causing CS0612/CS0618/CS0672 to bleed into consumer builds, and (2) multi-arg event raise dispatch was missing null-forgiving operators, causing CS8600/CS8604 under nullable context. Both are real correctness issues that could block TreatWarningsAsErrors users.

The overall implementation is solid and well-scoped. A few points to consider:

---

Positive Observations

Asymmetric accessor handling is correct. The GetAccessorObsoleteAttributeSyntax helper correctly avoids double-emitting when the property itself is already marked [Obsolete], and only emits per-accessor attributes for the asymmetric case ({ [Obsolete] get; set; }). The snapshot in Interface_With_Obsolete_Members.verified.txt confirms this produces the right output for all three cases (both, get-only, set-only).

String escaping is tested. The WithTrickyChars member exercising embedded quotes and backslashes is a good regression guard for the message-literal path.

Test infrastructure improvement. Enabling NullableContextOptions.Enable on the test compilation (previously omitted) means the CS8600/CS8604 bug class is detectable in tests going forward, not just by accident.

---

Issue 1: No direct assertion that CS0612/CS0618/CS0672 are suppressed

The new AssertGeneratedCodeHasNoNullableWarnings helper guards against CS86xx nullable warnings, but the original reported bug was CS0612/CS0618/CS0672 (obsolete warnings). The test Interface_With_Obsolete_Members calls AssertGeneratedCodeHasNoNullableWarnings — which does not check for CS06xx.

The snapshot approach gives indirect coverage (if the [Obsolete] attribute is emitted, the compiler won't warn), but the warning is never asserted absent in a compile-time sense. Since GetGeneratedCompilationErrors filters to errors only, a CS0618 regression (which is a warning by default) would silently slip through.

Consider adding a parallel helper:

protected static void AssertGeneratedCodeHasNoObsoleteWarnings(
    string source,
    IEnumerable<MetadataReference>? additionalReferences = null,
    CSharpParseOptions? parseOptions = null)
{
    var (_, diagnostics) = RunGeneratorWithCompilationDiagnostics(source, additionalReferences, parseOptions);
    var obsoleteWarnings = diagnostics
        .Where(d => d.Severity >= DiagnosticSeverity.Warning
                 && (d.Id is "CS0612" or "CS0618" or "CS0672"))
        .ToList();
    if (obsoleteWarnings.Count > 0)
    {
        var messages = string.Join(Environment.NewLine, obsoleteWarnings.Select(d => d.ToString()));
        throw new InvalidOperationException(
            $"Generated code emits {obsoleteWarnings.Count} obsolete warning(s):{Environment.NewLine}{messages}");
    }
}

This would turn the regression test from "the snapshot has the right content" into a hard compile-time assertion, which is a stronger guarantee and better documents what the fix actually prevents.

---

Issue 2: C# 10+ named parameters on are silently dropped

The GetObsoleteAttributeSyntax method reconstructs the attribute from ConstructorArguments only. In C# 10+, [Obsolete] gained named parameters:

[Obsolete("Use V2", DiagnosticId = "CUSTOM001", UrlFormat = "https://...")]

These live in attr.NamedArguments, not ConstructorArguments, so they are silently dropped. The generated forward would emit [global::System.Obsolete("Use V2")], losing the custom diagnostic ID and URL.

In practice, DiagnosticId/UrlFormat are rare in production interfaces, but it is worth noting because a consumer relying on a custom diagnostic ID to suppress warnings (e.g., #pragma warning disable CUSTOM001) would find their suppression not working in the mock.

If out of scope for this PR, a note in the method's XML doc would be useful.

---

Minor note on helper name

AssertGeneratedCodeHasNoNullableWarnings is clear, but given it checks a specific CS86xx range rather than all nullable-related diagnostics, AssertGeneratedCodeHasNoCS86xxWarnings would be more precise and consistent if a sibling CS06xx helper is added. Either is acceptable.

---

Overall this is a good, well-contained fix with thorough test coverage for the targeted scenarios. The suggestions above would strengthen the regression guarantees rather than change the correctness of the fix itself.

[claude]

Code Review

Summary

This PR fixes two bugs in TUnit.Mocks.SourceGenerator: (1) [Obsolete] attributes on interface/class members were not propagated to generated forwards and overrides, causing CS0612/CS0618/CS0672 to bleed into consumer builds, and (2) multi-arg event raise dispatch was missing null-forgiving operators, causing CS8600/CS8604 under nullable context. Both are real correctness issues that could block TreatWarningsAsErrors users.

The overall implementation is solid and well-scoped. A few points to consider:

---

Positive Observations

Asymmetric accessor handling is correct. The GetAccessorObsoleteAttributeSyntax helper correctly avoids double-emitting when the property itself is already marked [Obsolete], and only emits per-accessor attributes for the asymmetric case ({ [Obsolete] get; set; }). The snapshot in Interface_With_Obsolete_Members.verified.txt confirms this produces the right output for all three cases (both, get-only, set-only).

String escaping is tested. The WithTrickyChars member exercising embedded quotes and backslashes is a good regression guard for the message-literal path.

Test infrastructure improvement. Enabling NullableContextOptions.Enable on the test compilation (previously omitted) means the CS8600/CS8604 bug class is detectable in tests going forward, not just by accident.

---

Issue 1: No direct assertion that CS0612/CS0618/CS0672 are suppressed

The new AssertGeneratedCodeHasNoNullableWarnings helper guards against CS86xx nullable warnings, but the original reported bug was CS0612/CS0618/CS0672 (obsolete warnings). The test Interface_With_Obsolete_Members calls AssertGeneratedCodeHasNoNullableWarnings — which does not check for CS06xx.

The snapshot approach gives indirect coverage (if the [Obsolete] attribute is emitted, the compiler won't warn), but the warning is never asserted absent in a compile-time sense. Since GetGeneratedCompilationErrors filters to errors only, a CS0618 regression (which is a warning by default) would silently slip through.

Consider adding a parallel helper:

protected static void AssertGeneratedCodeHasNoObsoleteWarnings(
    string source,
    IEnumerable<MetadataReference>? additionalReferences = null,
    CSharpParseOptions? parseOptions = null)
{
    var (_, diagnostics) = RunGeneratorWithCompilationDiagnostics(source, additionalReferences, parseOptions);
    var obsoleteWarnings = diagnostics
        .Where(d => d.Severity >= DiagnosticSeverity.Warning
                 && (d.Id is "CS0612" or "CS0618" or "CS0672"))
        .ToList();
    if (obsoleteWarnings.Count > 0)
    {
        var messages = string.Join(Environment.NewLine, obsoleteWarnings.Select(d => d.ToString()));
        throw new InvalidOperationException(
            $"Generated code emits {obsoleteWarnings.Count} obsolete warning(s):{Environment.NewLine}{messages}");
    }
}

This would turn the regression test from "the snapshot has the right content" into a hard compile-time assertion — a stronger guarantee that directly validates what the fix prevents.

---

Issue 2: C# 10+ named parameters on [Obsolete] are silently dropped

The GetObsoleteAttributeSyntax method reconstructs the attribute from ConstructorArguments only. In C# 10+, [Obsolete] gained named parameters:

[Obsolete("Use V2", DiagnosticId = "CUSTOM001", UrlFormat = "https://...")]

These live in attr.NamedArguments, not ConstructorArguments, so they are silently dropped. The generated forward would emit [global::System.Obsolete("Use V2")], losing the custom diagnostic ID and URL.

In practice, DiagnosticId/UrlFormat are rare in production interfaces, but a consumer relying on a custom diagnostic ID to suppress warnings (e.g., #pragma warning disable CUSTOM001) would find their suppression not working on the mock. If out of scope for this PR, a note in the method's XML doc would be useful.

---

Minor note on helper naming

AssertGeneratedCodeHasNoNullableWarnings is clear, but given it checks a specific CS86xx range, a sibling AssertGeneratedCodeHasNoObsoleteWarnings would complete the picture. Whether you rename to AssertGeneratedCodeHasNoCS86xxWarnings for symmetry is stylistic.

---

Overall this is a good, well-contained fix with thorough test coverage for the targeted scenarios. The suggestions above would strengthen the regression guarantees rather than change the correctness of the fix itself.

[claude]

Code Review: fix(mocks): propagate [Obsolete] and null-forgiving raise dispatch (#5626)

This is a solid, well-motivated fix. The approach is correct and the regression coverage is strong. I have a few issues worth addressing — one of which is a correctness concern for incremental generators.

---

Critical: GetHashCode not updated in either model

Both MockMemberModel.GetHashCode() and MockEventModel.GetHashCode() are not updated to include the new ObsoleteAttribute fields, even though Equals is. For an incremental source generator, the pipeline uses both Equals and GetHashCode to determine cache validity. If two models hash to the same value but differ only in their ObsoleteAttribute (e.g., a member gains [Obsolete] between runs), the incremental pipeline may incorrectly skip regeneration.

// MockMemberModel.GetHashCode() should include new fields:
public override int GetHashCode()
{
    var hash = HashCode.Combine(..., SpanReturnElementType);
    hash = HashCode.Combine(hash, ObsoleteAttribute, GetterObsoleteAttribute, SetterObsoleteAttribute);
    return hash;
}

Same applies to MockEventModel.GetHashCode().

---

Design: Pre-rendered string in the model couples rendering to discovery

Storing ObsoleteAttribute as a pre-rendered C# string like "[global::System.Obsolete(\"msg\", true)]" in the model (a Roslyn data structure) couples the code-emission concern to the discovery phase. The rest of the model stores typed data (strings for names, booleans for flags, etc.) and delegates rendering to builders.

A typed ObsoleteInfo record would be a better fit:

internal sealed record ObsoleteInfo(string? Message, bool IsError, string? DiagnosticId, string? UrlFormat);

Then EmitObsoleteAttribute(writer, ObsoleteInfo? info) in the builders handles rendering. This makes the model testable without string-matching against C# fragments, keeps equality comparison on typed fields, and avoids the anti-pattern of storing pre-generated source snippets in model records. Worth considering for a follow-up if not now.

---

Important: GetObsoleteAttributeSyntax(property) called redundantly

In CreatePropertyModel, GetObsoleteAttributeSyntax(property) is called once for ObsoleteAttribute, then called again inside GetAccessorObsoleteAttributeSyntax for each accessor — 3× total for a property with both accessors. Pass the already-computed result in:

var propertyObsolete = GetObsoleteAttributeSyntax(property);
// ...
ObsoleteAttribute = propertyObsolete,
GetterObsoleteAttribute = GetAccessorObsoleteAttributeSyntax(propertyObsolete, property.GetMethod),
SetterObsoleteAttribute = GetAccessorObsoleteAttributeSyntax(propertyObsolete, property.SetMethod)

private static string GetAccessorObsoleteAttributeSyntax(string propertyObsolete, IMethodSymbol? accessor)
{
    if (accessor is null || propertyObsolete.Length > 0) return "";
    return GetObsoleteAttributeSyntax(accessor);
}

---

Minor: EscapeStringLiteral doesn't handle all escape sequences

The implementation only escapes \\ and ". An [Obsolete] message with \n, \r, \t, or NUL would produce syntactically invalid generated C#. These are rare in practice but worth a defensive fix:

private static string EscapeStringLiteral(string value)
    => "\"" + value
        .Replace("\\", "\\\\")
        .Replace("\"", "\\\"")
        .Replace("\n", "\\n")
        .Replace("\r", "\\r")
        .Replace("\t", "\\t")
        + "\"";

---

What's done well

• The core approach — copying the attribute onto every generated forward/override — is correct and handles all four code paths (method, property, event, static abstract DIM) consistently.
• Asymmetric accessor handling with GetterObsoleteAttribute/SetterObsoleteAttribute is clean and correct.
• Preserving C# 10+ DiagnosticId/UrlFormat named arguments is thoughtful and important for consumer suppression patterns.
• AssertGeneratedCodeHasNoObsoleteWarnings and AssertGeneratedCodeHasNoNullableWarnings are excellent regression guards — they prove the attribute placement actually suppresses warnings, not just appears in snapshot text.
• The ! null-forgiving fix for multi-arg event dispatch is minimal, correct, and well-justified.
• Snapshot tests cover tricky edge cases (embedded quotes/backslashes, DiagnosticId round-trip, asymmetric accessors).

The GetHashCode gap is the one item I'd block on before merging; the rest can be addressed at your discretion.

[claude]

Code Review: fix(mocks): propagate [Obsolete] and null-forgiving raise dispatch (#5626)

Overall this is a clean, focused bug fix with good test coverage. The approach is sound and the snapshot tests + compile-time assertion guards are the right way to prevent regressions here.

---

Correctness

The core logic is correct:

• GetObsoleteAttributeSyntax properly handles all constructor-argument shapes (no args, message only, message+isError) and the C# 10 named arguments.
• Per-accessor asymmetry ({ [Obsolete] get; set; }) is handled correctly by GetAccessorObsoleteAttributeSyntax — it only emits per-accessor attributes when the property itself is not already obsolete, avoiding duplicates.
• The ! null-forgiving fix in GenerateRaiseEventDispatch is consistent with the single-argument path and directly addresses the CS8600/CS8604 symptoms.

---

Design Concern: Pre-rendered strings in the model

The model stores already-rendered C# syntax:

// MockMemberModel.cs
public string ObsoleteAttribute { get; init; } = "";
// stored as e.g. "[global::System.Obsolete(\"Use V2\", false)]"

This couples discovery (knowing a member is obsolete) with emission (how to render it). A structured intermediate would be cleaner:

// Alternative
public ObsoleteInfo? Obsolete { get; init; }
record ObsoleteInfo(string? Message, bool IsError, string? DiagnosticId, string? UrlFormat);

In practice the string approach works fine since [Obsolete] is simple and stable, but if emission ever needs to change (conditional formatting, Roslyn-version gating), you'd touch every builder. Not a blocker — worth noting for future model evolution.

---

Silent drop of unknown named arguments

GetObsoleteAttributeSyntax only preserves DiagnosticId and UrlFormat from named arguments. System.ObsoleteAttribute has no other named args today, so this is safe. However, if a future .NET version adds one (as happened when DiagnosticId/UrlFormat were added in .NET 5), the drop would be silent. A non-string future named argument would also be silently skipped due to the is not string strValue guard. A brief comment marking this intentional would help:

// Only string-valued named args (DiagnosticId, UrlFormat) are preserved.
// Non-string or unrecognised named args are intentionally dropped — [Obsolete]
// has no others today, but if .NET adds some the generated attribute still
// compiles; it just won't carry the extra metadata.

---

Minor: AssertGeneratedCodeHasNoNullableWarnings prefix filter

d.Id.StartsWith("CS86", StringComparison.Ordinal) catches all CS86xx diagnostics. The doc comment lists specific IDs (CS8600, CS8602, CS8603, CS8604, CS8618, CS8625), so the broader prefix acts as a forward-looking guard. The comment says "CS86xx-family" which communicates this intent clearly enough.

---

Inline accessor attributes in MockWrapperTypeBuilder

var getterAttr = prop.GetterObsoleteAttribute.Length > 0 ? prop.GetterObsoleteAttribute + " " : "";
var getter = prop.HasGetter ? $"{getterAttr}get => {target}.{prop.Name}; " : "";

This generates { [global::System.Obsolete] get => ...; set => ...; } on one line. Technically valid C#, fine for generated code. It differs stylistically from the multi-line per-accessor emission in MockImplBuilder, which could surprise future maintainers. Not a bug.

---

Testing quality

The test additions are thorough:

• Interface_With_Obsolete_Members covers methods, generics, events, symmetric/asymmetric accessor attributes, string escaping, and DiagnosticId/UrlFormat.
• Interface_FluentUI_Shape_Nullable_Warnings reproduces the exact real-world trigger shape from the issue.
• AssertGeneratedCodeHasNoObsoleteWarnings / AssertGeneratedCodeHasNoNullableWarnings compile the output and assert at the diagnostic level, not just snapshot-text level — this is the right pattern for a regression guard.
• The refactor of GetGeneratedCompilationErrors into a shared private RunGeneratorWithCompilationDiagnostics is a clean extraction and enabling nullable context for all compilations is the correct call.

---

Summary

The fix is correct and well-tested. Two items worth addressing before merge (or as tracked follow-ups):

1. Silent drop of unrecognised named arguments — add a comment noting this is intentional and when it would need revisiting.
2. Pre-rendered string model — acceptable tradeoff for now, but a comment or TODO would help if the team anticipates [Obsolete] handling will need to evolve.

Neither is a blocker. Good work closing this one out.

[thomhurst]

Thanks for this!