fix(mocks): respect generic type argument accessibility (#5453)#5460
fix(mocks): respect generic type argument accessibility (#5453)#5460
Conversation
Extends the effective-accessibility check from #5426 to also walk generic type arguments and array element types. Previously, mocking a public generic interface closed over an internal type argument (e.g. Mock.Of<ILogger<InternalClass>>()) emitted a public wrapper whose base signature leaked the internal type, producing CS9338/CS0051. IsEffectivelyPublic now switches on the type kind and recurses into TypeArguments/ElementType, collapsing the previous two helpers into one.
There was a problem hiding this comment.
Code Review
This is a clean, well-scoped fix for the generic type argument accessibility leak. The analysis is correct and the implementation handles the full problem well.
What the fix does right
The original IsEffectivelyPublic only walked the containing-type chain, so ILogger<InternalClass> would pass (the interface itself is public), while the bound type argument InternalClass was never inspected. The new approach recursively descends all structural positions — containing types, type arguments, array elements — which is the correct complete model of effective public accessibility.
The collapse from a helper pair into one IsEffectivelyPublic(ITypeSymbol) switch is a good simplification; the single-entry-point recursive approach is easier to reason about and extend.
One architectural observation worth considering
The ITypeParameterSymbol → true branch is defensively correct (open generics can't be mocked directly at a call site), but the comment could be tightened: if an unbound generic were somehow discovered, emitting a public wrapper for an unconstrained T is only safe because Roslyn won't bind a generic type argument to a non-public symbol in a context visible to the generator. This is a reasonable assumption but might be worth a one-liner note so a future reader understands the invariant, not just the decision.
Edge case coverage looks solid
A quick mental walkthrough of harder cases:
IReadOnlyDictionary<string, IEnumerable<InternalClass>>→ descends two levels, correctly returnsfalse✓- Jagged arrays
InternalClass[][]→ recursion through nestedIArrayTypeSymbol→INamedTypeSymbol→false✓ Nullable<InternalStruct>→Nullable<T>is public, descends toInternalStruct→false✓
Test file
The three test cases are well-chosen:
- Basic internal type argument
partialmodifier (preserves traceability to the original report)Task<T>return path (different generated extension overload)
The compile-time-failure-as-regression-test pattern is the right call here — if the generator regresses, you find out at build time, not at runtime.
One cosmetic note: the nested classes InternalConsumer / InternalPartialConsumer could sit in a dedicated Issue5453TestTypes static class at the top to make the internal visibility unambiguous (nested classes inside a public outer class are internal-by-default in C# but readers occasionally have to stop and think about it). Not a blocking concern.
Summary
Correct fix, correct scope, good tests. No issues blocking merge.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 2 minor |
🟢 Metrics 4 complexity
Metric Results Complexity 4
TIP This summary will be updated as you push new changes. Give us feedback
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.29.0 to 1.30.0. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.30.0 <!-- Release notes generated using configuration in .github/release.yml at v1.30.0 --> ## What's Changed ### Other Changes * perf: eliminate locks from mock invocation and verification hot paths by @thomhurst in thomhurst/TUnit#5422 * feat: TUnit0074 analyzer for redundant hook attributes on overrides by @thomhurst in thomhurst/TUnit#5459 * fix(mocks): respect generic type argument accessibility (#5453) by @thomhurst in thomhurst/TUnit#5460 * fix(mocks): skip inaccessible internal accessors when mocking Azure.Response by @thomhurst in thomhurst/TUnit#5461 * fix: apply CultureAttribute and STAThreadExecutorAttribute to hooks (#5452) by @thomhurst in thomhurst/TUnit#5463 ### Dependencies * chore(deps): update tunit to 1.29.0 by @thomhurst in thomhurst/TUnit#5446 * chore(deps): update react to ^19.2.5 by @thomhurst in thomhurst/TUnit#5457 * chore(deps): update opentelemetry to 1.15.2 by @thomhurst in thomhurst/TUnit#5456 * chore(deps): update dependency qs to v6.15.1 by @thomhurst in thomhurst/TUnit#5458 **Full Changelog**: thomhurst/TUnit@v1.29.0...v1.30.0 Commits viewable in [compare view](thomhurst/TUnit@v1.29.0...v1.30.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
publicmock wrappers and extension classes for public generic interfaces closed over a non-public type argument (e.g.Mock.Of<ILogger<InternalClass>>()), producing CS9338/CS0051 inconsistent-accessibility errors.IsEffectivelyPublic(ITypeSymbol)that switches on type kind.TUnit.Mocks.Tests/Issue5453Tests.cs. If the generator regresses, the test project will not build.Test plan
Issue5453Tests(3 tests, including a separateinternal sealed partialcase matching the original report) passTUnit.Mocks.TestspassTUnit.Mocks.SourceGenerator.Testssnapshot tests pass — no snapshot churnTUnit.Mocks.Analyzers.Testspass