fix(mocks): emit [SetsRequiredMembers] on generated mock ctor (#5678)

[thomhurst]

Summary

• Emits [SetsRequiredMembers] on every generated XxxMockImpl / XxxWrapMockImpl constructor so the factory can new them without the user having to satisfy required members at the call site.
• Restores the SKIPPED T17_Required_Property_Does_Not_Block_Mock_Instantiation and adds a multi-required shape (ref + value type) so the initializer-skip logic is exercised broadly.

Why

A class with required members (e.g. public required string Name { get; init; }) couldn't be mocked: the generated factory's new XxxMockImpl(engine) produced CS9035 because the constructor didn't declare it took responsibility for the required members. Annotating the generated ctor with [SetsRequiredMembers] tells the compiler the constructor satisfies the contract — which is appropriate for a mock that is itself the test double.

No polyfill was needed: net8.0+ ships the attribute, and the repo's Polyfill package supplies it for net472.

Test plan

• T17 single-required test passes
• T17 multi-required (ref + value) test passes
• TUnit.Mocks.Tests full suite (net10.0): 954/954
• TUnit.Mocks.SourceGenerator.Tests (net10.0): 45/45

Closes #5678

[claude]

Code Review — fix(mocks): emit [SetsRequiredMembers] on generated mock ctor (#5678)

Summary

The fix is correct and well-targeted. Annotating every generated mock constructor with [global::System.Diagnostics.CodeAnalysis.SetsRequiredMembers] is the right approach — it suppresses CS9035 at the mock-factory call site without requiring users to provide required-member values they don't care about in tests.

---

What's Good

Correctness of the attribute placement — Emitting [SetsRequiredMembers] on all generated constructors (not only when the mocked type is detected to have required members) is a defensively correct choice. It avoids the need to statically detect required members in the source generator, and since these constructors are internal to the generated mock implementation, there's no false-safety risk to callers.

No polyfill needed — Correctly noted in the PR description: net8.0+ ships the attribute natively, and the repo's Polyfill package covers net472. No extra work required.

Snapshot tests updated — All ten affected .verified.txt files correctly reflect the new attribute line. The snapshot coverage confirms the attribute appears on every ctor variant (default, parameterless, parameterized, wrap, partial).

Test design — Replacing the skipped T17 block with two concrete tests (single required string + multi-required string/int/Guid) gives good breadth. Mixing reference and value types in T17_Multiple_Required_Members_Reference_And_Value_Types is a meaningful edge-case check.

---

One Concern: Uninitialized Required Properties in Mock Instances

[SetsRequiredMembers] tells the compiler "this constructor takes responsibility for required members" — but the generated constructors don't actually assign them. The generated mock inherits the required fields from the base class, and they remain at their default values (null for strings, 0 / Guid.Empty for value types).

This is acceptable behaviour for a mock library (the mock is a test double, not a real object), but it's worth documenting or at least adding an assertion to the test to make the expectation explicit:

[Test]
public async Task T17_Required_Property_Does_Not_Block_Mock_Instantiation()
{
    var mock = Mock.Create<RequiredShape>();
    // Required members are intentionally left at their default values in mocks.
    // The attribute [SetsRequiredMembers] suppresses CS9035 at the call site.
    Assert.That(mock.Name, Is.Null); // documents the "uninitialized" contract
    await Assert.That(mock.Compute()).IsEqualTo(0);
}

This makes it clear to future contributors that null here is intentional, not a bug — and protects against a future refactor that accidentally initializes these properties to something unexpected.

---

Minor: Structural Repetition in MockImplBuilder.cs

The attribute emission appears six times (three in GenerateWrapConstructors, three in GeneratePartialConstructors), each as:

writer.WriteLine(SetsRequiredMembersAttribute);

This is not harmful — it's a single constant write — but the structural pattern of "emit attribute, then emit constructor" is repeated across all six branch paths. If this file grows with future attributes (e.g. [Obsolete], [EditorBrowsable]), the per-branch duplication will compound.

A small local helper encapsulating the ctor preamble would make future extensions cleaner:

private static void WriteConstructorPreamble(IndentedTextWriter writer)
{
    writer.WriteLine(SetsRequiredMembersAttribute);
    // future: other ctor-level attributes go here
}

This is a low-priority suggestion — the current structure is readable and the PR scope is already well-contained.

---

Verdict

The fix is sound, the snapshots are correct, and the tests cover the key shapes. The one actionable suggestion is to add an assertion that documents the "required members are left at default" contract. The structural duplication note is advisory only.

Good fix overall. The root-cause analysis in the PR description is accurate and the implementation matches it precisely.

[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.}

[thomhurst]

Review feedback applied

Applied (commit 1b0b89cc2):

• Added explicit default-state assertions to both T17 tests in TUnit.Mocks.Tests/KitchenSinkEdgeCasesTests.cs:
  • T17_Required_Property_Does_Not_Block_Mock_Instantiation: asserts mock.Object.Name is null.
  • T17_Multiple_Required_Members_Reference_And_Value_Types: asserts Title is null, Count is 0, Id is Guid.Empty.
• Added the requested one-line comment above each block: // Required members intentionally remain at default in mocks; [SetsRequiredMembers] suppresses CS9035 only.

Deferred:

• The WriteConstructorPreamble helper extraction in MockImplBuilder.cs — per reviewer's own low-priority/YAGNI note, and given the prior b6edcbc15 cleanup already hoisted the attribute string to a const.

Verification (net10.0):

• TUnit.Mocks.Tests T17 filter: 2/2 passed.
• TUnit.Mocks.Tests full suite: 954/954 passed.
• TUnit.Mocks.SourceGenerator.Tests: 45/45 passed.
• No .received.txt snapshot diffs produced (test-only change).

[claude]

Code Review — fix(mocks): emit [SetsRequiredMembers] on generated mock ctor (#5682)

Follow-up on Prior Review (2026-04-23)

The previous review raised two concerns. Checking the current state:

✅ Uninitialized required properties — addressed. The tests now include both inline comments and explicit assertions documenting the contract:

// Required members intentionally remain at default in mocks; [SetsRequiredMembers] suppresses CS9035 only.
await Assert.That(mock.Object.Name).IsNull();
await Assert.That(mock.Object.Count).IsEqualTo(0);
await Assert.That(mock.Object.Id).IsEqualTo(System.Guid.Empty);

This is exactly right. Future readers won't mistake null for a bug.

⬜ Structural repetition in MockImplBuilder.cs — still open (advisory). The six individual writer.AppendLine(SetsRequiredMembersAttribute) calls remain. Still low-priority — see below if this matters to you.

---

New Observations

Comment length exceeds project style guideline

MockImplBuilder.cs adds a 2-line comment block:

// Tells the compiler the generated ctor satisfies any `required` members on the
// mocked base type, so the factory can `new XxxMockImpl(engine)` without CS9035.

CLAUDE.md says: "Don't write multi-paragraph docstrings or multi-line comment blocks — one short line max." The WHY here is genuinely non-obvious (a C# quirk, not self-evident from the code), so a comment is warranted — but it should be a single line:

// Suppresses CS9035: constructor claims responsibility for required members so the factory can new XxxMockImpl(engine) without initializers.

Missing snapshot for a type with required members

The snapshot tests (TUnit.Mocks.SourceGenerator.Tests/Snapshots/) now all emit [SetsRequiredMembers], but none of the snapshot-test fixture types actually have required members. The runtime tests in KitchenSinkEdgeCasesTests.cs cover the compile+run path, but a dedicated snapshot — e.g. Class_With_Required_Members.verified.txt — would lock in the generated output for that exact case and make regressions immediately visible in the generator test suite. This is advisory, not blocking.

---

Overall Assessment

The fix is correct, well-scoped, and properly tested. The two new tests cover the meaningful edge cases (single required ref type; mixed ref + value types) and explicitly document the "required members remain at default" contract. Snapshot coverage is thorough for existing shapes. The main actionable items are:

1. Minor (should fix): Collapse the 2-line comment in MockImplBuilder.cs to one line.
2. Advisory: Consider adding a snapshot fixture for a type with required members.
3. Advisory: Consider a WriteConstructorPreamble helper if more ctor-level attributes are added in the future.

Good fix overall — the root cause is clearly understood and the implementation is precise.

[thomhurst]

2nd-round review applied in b80c3c5:

1. Comment tightened — MockImplBuilder.cs const-decl comment collapsed from 2 lines to a single one-liner per project style guideline:

   // Suppresses CS9035: ctor claims responsibility for required members so factory can `new XxxMockImpl(engine)` without initializers.

2. Required-members snapshot added — new Class_With_Required_Members test in MockGeneratorTests.cs covers the exact required-member scenario fixed by [Bug] TUnit.Mocks: mocking a type with required members fails with CS9035 #5678 (one ref-typed string + one ref-typed object required property on a ConfigBase partial-mock target). Verified .verified.txt shows [SetsRequiredMembers] correctly emitted on the generated impl ctor; locks in generator output as a regression guard.

3. Skipped — WriteConstructorPreamble helper (reviewer marked advisory; const-hoist already covered the duplication concern in round 1).

Tests — all green on net10.0:

• T17 required-member runtime tests in KitchenSinkEdgeCasesTests: 2/2 pass
• TUnit.Mocks.Tests: 954/954 pass
• TUnit.Mocks.SourceGenerator.Tests: 46/46 pass (45 prior + 1 new)

No .received.txt committed.

[claude]

Code Review — Follow-up (Third Pass)

Prior Feedback Status

All actionable items from the two previous reviews have been addressed:

Item	Status
Document "required members stay at default" contract in tests	✅ Addressed in commit 1b0b89cc — explicit IsNull() / IsEqualTo(0) / IsEqualTo(Guid.Empty) assertions added
Collapse 2-line comment to one line	✅ Addressed in commit b80c3c5a — now a single-line // Suppresses CS9035: ...
Add snapshot for type with required members	✅ Addressed in commit b80c3c5a — Class_With_Required_Members.verified.txt added
Hoist SetsRequiredMembersAttribute to a constant	✅ Addressed in commit b6edcbc1
Structural repetition (6 separate writer.AppendLine calls)	⬜ Still open — always advisory, no action needed unless more ctor-level attributes are added

---

Current State Assessment

The PR is in good shape. The fix is correct, the snapshot coverage is thorough, and the test comments/assertions make the "required members intentionally remain at default" contract explicit for future contributors. No outstanding actionable items remain.

The one open advisory (a WriteConstructorPreamble helper to centralise future ctor-level attribute emission) is low-priority and can be deferred until there is a second ctor-level attribute to justify the abstraction.

LGTM.

[thomhurst]

Merged latest origin/main (which now contains the merged sister PRs #5680 and #5683).

Conflict resolution (single hunk in TUnit.Mocks.SourceGenerator.Tests/MockGeneratorTests.cs):

• Both sides added a new [Test]-attributed snapshot test method right after Class_Implementing_Forwarded_Interface_Hierarchy. Resolution kept both methods: this branch's Class_With_Required_Members followed by main's SelfEquatable_Generates_EqualsOf_GetHashCodeOf_ToStringOf.
• No duplicates: 46 distinct public Task test methods after merge.

Snapshot reconciliation:

• SelfEquatable_Generates_EqualsOf_GetHashCodeOf_ToStringOf.verified.txt (added on main) needed one line — [global::System.Diagnostics.CodeAnalysis.SetsRequiredMembers] on the generated mock impl ctor — because that attribute is exactly what this PR emits for every generated impl. Verified the diff is the intentional fix from [Bug] TUnit.Mocks: mocking a type with required members fails with CS9035 #5678 and accepted the regenerated snapshot.

Verification (net10.0):

• TUnit.Mocks.SourceGenerator.Tests: 47 / 47 passed.
• TUnit.Mocks.Tests filtered to KitchenSinkEdgeCasesTests/*: 21 / 21 passed (includes T17 T17_Required_Property_Does_Not_Block_Mock_Instantiation and T17_Multiple_Required_Members_Reference_And_Value_Types).

Merge commit: cb93299da. Snapshot follow-up: 0e51359b9.

[claude]

Follow-up Review — Previous Feedback Status

This is a follow-up to the earlier review on this PR.

Previous Concerns — Addressed ✅

"Uninitialized Required Properties" concern (main feedback from prior review) — fully addressed. The tests now have both explicit assertions and explanatory comments:

// Required members intentionally remain at default in mocks; [SetsRequiredMembers] suppresses CS9035 only.
await Assert.That(mock.Object.Name).IsNull();
await Assert.That(mock.Object.Count).IsEqualTo(0);
await Assert.That(mock.Object.Id).IsEqualTo(System.Guid.Empty);

This documents the contract clearly — future contributors won't mistake null for a bug.

Structural repetition note (advisory only, from prior review) — not acted upon, which is fine. The six direct writer.AppendLine(SetsRequiredMembersAttribute) calls remain. Still low-priority; acceptable as-is.

---

Fresh Assessment

The implementation is correct and complete:

• Attribute placement is unconditional — applied to all generated constructors regardless of whether the base type has required members. This is the right call: it avoids adding static detection of required members to the source generator, and since these constructors are internal to the generated file sealed class, there's no false-safety exposure.
• Interface mock constructors also receive the attribute (see Interface_With_Obsolete_Members.verified.txt). Interfaces can't declare required members, so the attribute is technically a no-op there, but it's harmless and keeps the generator code unconditionally consistent — no special-casing needed.
• Snapshot coverage is thorough: all ten affected .verified.txt files are updated, covering default ctors, parameterless base ctors, parameterized ctors, partial mocks, and wrap mocks.
• Test breadth is good: single required string, and multi-required mixing string / int / Guid (ref + value types).

No new concerns. This is a clean, minimal, correct fix.