From cf825a1ae6a42e9ce59e971bd644ca3bf860eea2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Valentin=20Breu=C3=9F?= Date: Tue, 28 Jul 2026 23:16:10 +0200 Subject: [PATCH] fix: keep the reachable accessor of a partly inaccessible property A filled base slot was dropped from the mock surface whenever either accessor was invisible to the mock's assembly, so a property like public abstract string Mixed { get; internal set; } that a derived type already overrides lost its public getter entirely. The type stayed mockable, but `Mixed` was absent from the generated surface and `mock.Setup.Mixed` failed with CS1061 and nothing explaining why. FindPartlyReachableFilledSlot now recognises that shape, so FillsInaccessibleBaseSlot keeps the base declaration and the emitter overrides only the reachable accessor; Property.Getter/Setter were already null for the other one. Those slots are threaded down as filledProperties so that ComputeHasInaccessibleRequiredMember does not count the unreachable half as an unmet obligation. Without that the type would be refused again. An unfilled mixed accessor is still refused: nothing in the chain implements the abstract inaccessible accessor, so a partial override would fail with CS0534. That distinction is why the filled-slot information has to be threaded rather than read off the base symbol. Only an override can be partial. An explicit interface implementation must supply every accessor (CS0551) and cannot reach an internal one (CS0122), so interface slots stay all-or-nothing. --- Docs/pages/08-analyzers.md | 29 +++ .../Entities/Class.cs | 69 +++++-- .../MockabilityAnalyzerAccessibilityTests.cs | 4 + .../MockTests.CrossAssemblyTests.cs | 175 +++++++++++++++++- 4 files changed, 258 insertions(+), 19 deletions(-) diff --git a/Docs/pages/08-analyzers.md b/Docs/pages/08-analyzers.md index 211cb1a6..b8c75bb6 100644 --- a/Docs/pages/08-analyzers.md +++ b/Docs/pages/08-analyzers.md @@ -39,6 +39,35 @@ assembly: for example an `internal abstract` or `private protected abstract` mem `InternalsVisibleTo`. There is no valid code a mock could emit for such a member, so the type cannot be mocked at all. +The rule only considers members the mock is still obliged to implement. If a more derived type in the +referenced assembly already overrides the inaccessible member, the obligation is discharged and the +type stays mockable: + +```csharp +// In a referenced assembly that does not grant InternalsVisibleTo: +public abstract class Dispenser +{ + internal abstract void Refill(); +} + +public abstract class ChocolateDispenser : Dispenser +{ + internal override void Refill() { } // obligation discharged +} + +// Mockolate0002 fires for Dispenser, but ChocolateDispenser mocks fine. +ChocolateDispenser sut = ChocolateDispenser.CreateMock(); +``` + +`Refill` itself is not part of the mock's surface, since your assembly cannot see it. An +`internal abstract override` re-declaration is not a discharge: it continues the obligation without +implementing it, so the rule still fires. + +The same applies per accessor. For a property whose accessors differ in accessibility, such as +`public abstract string Flavour { get; internal set; }`, an override further down discharges only the +inaccessible half. The mock then overrides the accessor it can see and leaves the other one to the +referenced assembly's implementation, so writes through the mock are not intercepted or recorded. + ## Mockolate0003 A mocked member's signature routes through the ref-struct pipeline in a way Mockolate can't diff --git a/Source/Mockolate.SourceGenerators/Entities/Class.cs b/Source/Mockolate.SourceGenerators/Entities/Class.cs index 417f9133..3711ebaa 100644 --- a/Source/Mockolate.SourceGenerators/Entities/Class.cs +++ b/Source/Mockolate.SourceGenerators/Entities/Class.cs @@ -23,7 +23,8 @@ public Class(ITypeSymbol type, List? alreadyDefinedEvents = null, List? exceptMethods = null, List? exceptProperties = null, - List? exceptEvents = null) + List? exceptEvents = null, + List? filledProperties = null) #pragma warning restore S107 { _sourceAssembly = sourceAssembly; @@ -61,9 +62,15 @@ public Class(ITypeSymbol type, List methodExceptCandidates = new(); List propertyExceptCandidates = new(); List eventExceptCandidates = new(); + List propertyFilledCandidates = new(); foreach (ISymbol member in members) { + if (FindPartlyReachableFilledSlot(member, sourceAssembly) is { } partlyReachableSlot) + { + propertyFilledCandidates.Add(new Property(partlyReachableSlot, null, sourceAssembly)); + } + if (!FillsInaccessibleBaseSlot(member, sourceAssembly)) { continue; @@ -161,15 +168,18 @@ public Class(ITypeSymbol type, exceptEvents ??= new List(); exceptEvents.AddRange(DistinctList(eventExceptCandidates)); + filledProperties ??= new List(); + filledProperties.AddRange(DistinctList(propertyFilledCandidates)); + InheritedTypes = new EquatableArray( GetInheritedTypes(type).Select(t => new Class(t, sourceAssembly, methods, properties, events, exceptMethods, exceptProperties, - exceptEvents)) + exceptEvents, filledProperties)) .ToArray()); ReservedNames = ComputeReservedNames(type); - HasInaccessibleRequiredMember = ComputeHasInaccessibleRequiredMember() || + HasInaccessibleRequiredMember = ComputeHasInaccessibleRequiredMember(filledProperties) || InheritedTypes.Any(inherited => inherited.HasInaccessibleRequiredMember); _surfaceHash = ComputeSurfaceHash(); @@ -260,18 +270,44 @@ private int ComputeSurfaceHash() /// Mockolate0002 for the same condition so the user gets a diagnostic instead of a silently /// missing mock. Keep both in sync. /// - private bool ComputeHasInaccessibleRequiredMember() + private bool ComputeHasInaccessibleRequiredMember(List filledProperties) => Methods.Any(method => method is { IsAbstract: true, IsOverridableFromMock: false, }) || - Properties.Any(property => property is { IsAbstract: true, IsOverridableFromMock: false, }) || + Properties.Any(property => property is { IsAbstract: true, IsOverridableFromMock: false, } && + !filledProperties.Contains(property, + Property.ContainingTypeIndependentEqualityComparer)) || Events.Any(@event => @event is { IsAbstract: true, IsOverridableFromMock: false, }); /// /// True when fills a base slot (by or by - /// explicit interface implementation) whose base declaration is invisible to - /// . + /// explicit interface implementation) that the mock must leave alone entirely, because the base + /// declaration or one of its accessors is invisible to . /// private static bool FillsInaccessibleBaseSlot(ISymbol member, IAssemblySymbol? sourceAssembly) - => EnumerateFilledSlots(member).Any(slot => !IsSlotReachable(slot, sourceAssembly)); + { + if (FindPartlyReachableFilledSlot(member, sourceAssembly) is not null) + { + return false; + } + + return EnumerateFilledSlots(member).Any(slot => !IsSlotReachable(slot, sourceAssembly)); + } + + /// + /// A filled slot the mock can still restate in part, as in + /// public abstract string P { get; internal set; }: it keeps the reachable accessor and + /// drops the other, which the filling member already implements. + /// + /// + /// Only an qualifies; an explicit interface implementation must supply + /// every accessor (CS0551) and cannot reach an internal one (CS0122). + /// + private static IPropertySymbol? FindPartlyReachableFilledSlot(ISymbol member, + IAssemblySymbol? sourceAssembly) + => member is IPropertySymbol { IsAbstract: false, OverriddenProperty: { } slot, } && + Helpers.IsOverridableFrom(slot, sourceAssembly) && + HasUnreachableAccessor(slot, sourceAssembly) + ? slot + : null; private static IEnumerable EnumerateFilledSlots(ISymbol member) { @@ -322,16 +358,15 @@ private static IEnumerable EnumerateFilledSlots(ISymbol member) } private static bool IsSlotReachable(ISymbol slot, IAssemblySymbol? sourceAssembly) - => slot switch - { - IPropertySymbol property => Helpers.IsOverridableFrom(property, sourceAssembly) && - IsAccessorReachable(property.GetMethod, sourceAssembly) && - IsAccessorReachable(property.SetMethod, sourceAssembly), - _ => Helpers.IsOverridableFrom(slot, sourceAssembly), - }; + => Helpers.IsOverridableFrom(slot, sourceAssembly) && + (slot is not IPropertySymbol property || !HasUnreachableAccessor(property, sourceAssembly)); + + private static bool HasUnreachableAccessor(IPropertySymbol property, IAssemblySymbol? sourceAssembly) + => IsUnreachableAccessor(property.GetMethod, sourceAssembly) || + IsUnreachableAccessor(property.SetMethod, sourceAssembly); - private static bool IsAccessorReachable(IMethodSymbol? accessor, IAssemblySymbol? sourceAssembly) - => accessor is null || Helpers.IsOverridableFrom(accessor, sourceAssembly); + private static bool IsUnreachableAccessor(IMethodSymbol? accessor, IAssemblySymbol? sourceAssembly) + => accessor is not null && !Helpers.IsOverridableFrom(accessor, sourceAssembly); /// /// Identifiers that the mock class shares its scope with but that aren't surfaced through diff --git a/Tests/Mockolate.Analyzers.Tests/MockabilityAnalyzerAccessibilityTests.cs b/Tests/Mockolate.Analyzers.Tests/MockabilityAnalyzerAccessibilityTests.cs index a6059a48..b8eaa33d 100644 --- a/Tests/Mockolate.Analyzers.Tests/MockabilityAnalyzerAccessibilityTests.cs +++ b/Tests/Mockolate.Analyzers.Tests/MockabilityAnalyzerAccessibilityTests.cs @@ -63,6 +63,10 @@ public abstract class MyExternalType : MyBaseType "internal override event System.EventHandler MyMember;")] [InlineData("public abstract string MyMember { get; internal set; }", "public override string MyMember { get => null!; internal set { } }")] + [InlineData("public abstract string MyMember { internal get; set; }", + "public override string MyMember { internal get => null!; set { } }")] + [InlineData("private protected abstract int MyMember { get; set; }", + "private protected override int MyMember { get; set; }")] public async Task WhenInaccessibleAbstractMemberIsAlreadyOverridden_ShouldNotBeFlagged( string baseMember, string derivedOverride) => await Verifier .VerifyAnalyzerWithReferencedProjectAsync( diff --git a/Tests/Mockolate.SourceGenerators.Tests/MockTests.CrossAssemblyTests.cs b/Tests/Mockolate.SourceGenerators.Tests/MockTests.CrossAssemblyTests.cs index c0b74160..7c71111d 100644 --- a/Tests/Mockolate.SourceGenerators.Tests/MockTests.CrossAssemblyTests.cs +++ b/Tests/Mockolate.SourceGenerators.Tests/MockTests.CrossAssemblyTests.cs @@ -209,8 +209,8 @@ await That(result.Sources).DoesNotContainKey("Mock.MyExternalType.g.cs") [InlineData("internal abstract int Hidden { get; set; }", "internal override int Hidden { get; set; }")] [InlineData("internal abstract event System.EventHandler Hidden;", "internal override event System.EventHandler Hidden;")] - [InlineData("public abstract string Hidden { get; internal set; }", - "public override string Hidden { get => null!; internal set { } }")] + [InlineData("private protected abstract int Hidden { get; set; }", + "private protected override int Hidden { get; set; }")] public async Task InaccessibleAbstractMemberAlreadyOverridden_ShouldStillBeMocked( string baseMember, string derivedOverride) { @@ -273,6 +273,177 @@ await That(result.Sources["Mock.MyExternalType.g.cs"]) .Because("the default implementation fills the slot, so the mock inherits it rather than restating it"); } + [Theory] + [InlineData("public abstract string Mixed { get; internal set; }", + "public override string Mixed { get => null!; internal set { } }", "internal set")] + [InlineData("public abstract string Mixed { get; private protected set; }", + "public override string Mixed { get => null!; private protected set { } }", "private protected set")] + public async Task MixedAccessorSlotAlreadyOverridden_ShouldBeMockedWithTheAccessibleAccessorOnly( + string baseMember, string derivedOverride, string inaccessibleAccessor) + { + MetadataReference external = ExternalAssembly.Compile($$""" + namespace Ext; + + public abstract class MyBaseType + { + {{baseMember}} + } + + public abstract class MyExternalType : MyBaseType + { + {{derivedOverride}} + } + """); + + GeneratorResult result = Generator.RunWithReferences(CreateMockForMyExternalType, [external,]); + + await That(result.Diagnostics).IsEmpty(); + await That(result.Sources["Mock.MyExternalType.g.cs"]) + .Contains("public override string Mixed").And + .Contains(".GetProperty").And + .DoesNotContain(inaccessibleAccessor) + .Because("the accessor is invisible to the mock's assembly, so only the reachable one may be overridden"); + } + + [Theory] + [InlineData("public abstract string Mixed { internal get; set; }", + "public override string Mixed { internal get => null!; set { } }", "internal get")] + public async Task MixedAccessorSlotAlreadyOverridden_WithOnlyTheSetterReachable_ShouldBeMockedWithTheSetter( + string baseMember, string derivedOverride, string inaccessibleAccessor) + { + MetadataReference external = ExternalAssembly.Compile($$""" + namespace Ext; + + public abstract class MyBaseType + { + {{baseMember}} + } + + public abstract class MyExternalType : MyBaseType + { + {{derivedOverride}} + } + """); + + GeneratorResult result = Generator.RunWithReferences(CreateMockForMyExternalType, [external,]); + + await That(result.Diagnostics).IsEmpty(); + await That(result.Sources["Mock.MyExternalType.g.cs"]) + .Contains("public override string Mixed").And + .Contains(".SetProperty").And + .DoesNotContain(inaccessibleAccessor) + .Because("the accessor is invisible to the mock's assembly, so only the reachable one may be overridden"); + } + + [Fact] + public async Task MixedAccessorIndexerSlotAlreadyOverridden_ShouldBeMockedWithTheAccessibleAccessorOnly() + { + MetadataReference external = ExternalAssembly.Compile(""" + namespace Ext; + + public abstract class MyBaseType + { + public abstract int this[int index] { get; internal set; } + } + + public abstract class MyExternalType : MyBaseType + { + public override int this[int index] { get => 0; internal set { } } + } + """); + + GeneratorResult result = Generator.RunWithReferences(CreateMockForMyExternalType, [external,]); + + await That(result.Diagnostics).IsEmpty(); + await That(result.Sources["Mock.MyExternalType.g.cs"]) + .Contains("public override int this[int index]").And + .DoesNotContain("internal set") + .Because("the setter is invisible to the mock's assembly, so only the getter may be overridden"); + } + + [Fact] + public async Task MixedAccessorSlotFilledByAnIntermediateType_ShouldBeMockedWithTheAccessibleAccessorOnly() + { + MetadataReference external = ExternalAssembly.Compile(""" + namespace Ext; + + public abstract class MyBaseType + { + public abstract string Mixed { get; internal set; } + } + + public abstract class MyMiddleType : MyBaseType + { + public override string Mixed { get => null!; internal set { } } + } + + public abstract class MyExternalType : MyMiddleType + { + public abstract int Visible(); + } + """); + + GeneratorResult result = Generator.RunWithReferences(CreateMockForMyExternalType, [external,]); + + await That(result.Diagnostics).IsEmpty(); + await That(result.Sources["Mock.MyExternalType.g.cs"]) + .Contains("public override string Mixed").And + .DoesNotContain("internal set") + .Because("the slot is filled further up the chain, which discharges the setter just as a direct override does"); + } + + [Fact] + public async Task MixedAccessorSlot_WithInternalsVisibleTo_ShouldBeMockedWithBothAccessors() + { + MetadataReference external = ExternalAssembly.Compile(""" + [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("TestAssembly")] + namespace Ext; + + public abstract class MyBaseType + { + public abstract string Mixed { get; internal set; } + } + + public abstract class MyExternalType : MyBaseType + { + public override string Mixed { get => null!; internal set { } } + } + """); + + GeneratorResult result = Generator.RunWithReferences(CreateMockForMyExternalType, [external,]); + + await That(result.Diagnostics).IsEmpty(); + await That(result.Sources["Mock.MyExternalType.g.cs"]).Contains("internal set"); + } + + [Fact] + public async Task MixedAccessorInterfaceSlotWithDefaultImplementation_ShouldNotBeRestated() + { + MetadataReference external = ExternalAssembly.Compile(""" + namespace Ext; + + public interface IMyBaseType + { + string Mixed { get; internal set; } + } + + public interface MyExternalType : IMyBaseType + { + string IMyBaseType.Mixed { get => null!; set { } } + int Visible(); + } + """); + + GeneratorResult result = Generator.RunWithReferences(CreateMockForMyExternalType, [external,]); + + await That(result.Diagnostics).IsEmpty(); + await That(result.Sources["Mock.MyExternalType.g.cs"]) + .Contains("public int Visible()").And + .DoesNotContain("Mixed") + .Because( + "an explicit interface implementation must supply every accessor (CS0551) and cannot reach the internal one (CS0122), so interface slots stay all-or-nothing"); + } + [Fact] public async Task InaccessibleAbstractMemberReDeclaredAsAbstract_ShouldNotBeMocked() {