Preserve override accessibility in generated mocks#5984
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
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.
There was a problem hiding this comment.
Code Review: Preserve override accessibility in generated mocks
The core fix is correct and well-motivated. Replacing the binary IsProtected flag with OverrideAccessModifier (a string carrying the full resolved modifier) is the right abstraction, and the GetOverrideAccessModifier / HasInternalAccess logic in MemberDiscovery.cs handles all five accessibility levels correctly, including the ProtectedOrInternal → "protected" downgrade when InternalsVisibleTo isn't present.
Issue: AssertContains has a substring-match false positive risk
The new helper tests use text.Contains(expected, StringComparison.Ordinal), but "internal override string InternalMethod()" is a substring of "protected internal override string InternalMethod()". This means the test
AssertContains(generated, "internal override string InternalMethod()");would silently pass even if the generator incorrectly emitted protected internal override string InternalMethod() for a plain-internal member. The same risk applies to any assertion pair where the expected token is a suffix of a longer modifier (e.g. internal vs protected internal).
Suggested fix: Anchor assertions to a word boundary or leading whitespace, e.g.:
private static void AssertContainsLine(string text, string expected)
{
// Wrap in newlines so the modifier is not found inside a longer token.
if (!text.Contains($"\n {expected}", StringComparison.Ordinal)
&& !text.Contains($"\r\n {expected}", StringComparison.Ordinal))
{
throw new InvalidOperationException($"Expected generated output to contain: {expected}");
}
}Alternatively, use the existing VerifyGeneratorOutput snapshot approach (consistent with the surrounding test suite) so the entire generated file is validated—this would make cross-modifier confusion impossible to miss.
Minor: AssertContains / AssertDoesNotContain throw InvalidOperationException
Throwing InvalidOperationException from test helpers produces confusing stack traces and doesn't participate in test-framework output formatting. Prefer Assert.Fail(message) (or whatever the TUnit equivalent is) so failures are reported consistently alongside other test assertion failures.
Observation: CreateEventModel calls in the interface-member paths don't pass compilationAssembly (lines 152, 280)
These two call sites omit compilationAssembly, so the events always get the default OverrideAccessModifier = "public". Interface events are always public in C#, so this is harmless today, but it's inconsistent with the surrounding property/method call sites that do pass compilationAssembly. Worth making it uniform to avoid a latent bug if the code ever handles non-public default interface members.
Positive notes
- The
GetOverrideAccessModifierswitch is clean and exhaustive; the_ => "public"fallback is safe because the accessibility filter upstream already excludes non-overridable members. AccessorPrefixis a nice small helper that cleanly handles the "same modifier as property → no prefix" case.HasInternalAccesscorrectly usesGivesAccessTorather than a string comparison, so it handlesInternalsVisibleTotransparently.- The snapshot fix for
Partial_Mock_Omits_Inaccessible_Property_Setters(emittingprotected setinstead of bareset) is a real correctness improvement, not just cosmetic. - Test coverage across same-assembly, external-no-IVT, and external-with-IVT is comprehensive.
Fixes #5975.
This change preserves the effective accessibility of overridden members when generating partial and wrap mocks, so
protected internal,internal, andprivate protectedmembers emit valid overrides under the right assembly-access conditions.Validation:
dotnet build TUnit.Mocks.SourceGenerator.Tests/TUnit.Mocks.SourceGenerator.Tests.csproj -v:minimaldotnet test TUnit.Mocks.SourceGenerator.Tests/TUnit.Mocks.SourceGenerator.Tests.csproj --no-build --framework net10.0