Skip to content

Commit dcc9698

Browse files
Copilotericstj
Copilot
and
ericstj
authored
Fix PropertyPolicies to consider both getter and setter accessibility (#116080)
* Initial plan for issue * Fix PropertyPolicies to consider both getter and setter accessibility Co-authored-by: ericstj <[email protected]> * Refactor PropertyPolicies to combine IsMoreAccessible logic into GetMostAccessibleAccessor Co-authored-by: ericstj <[email protected]> * Simplify GetMostAccessibleAccessor by moving MethodAttributes calculation into GetAccessibilityRank Co-authored-by: ericstj <[email protected]> * Address feedback * Fix whitespace * Update comment and include private property in test * Remove GetAccessibilityRank --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: ericstj <[email protected]> Co-authored-by: Eric StJohn <[email protected]>
1 parent 088856e commit dcc9698

File tree

6 files changed

+129
-2
lines changed

6 files changed

+129
-2
lines changed

‎src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public sealed override IEnumerable<PropertyInfo> CoreGetDeclaredMembers(RuntimeT
3333

3434
public sealed override void GetMemberAttributes(PropertyInfo member, out MethodAttributes visibility, out bool isStatic, out bool isVirtual, out bool isNewSlot)
3535
{
36-
MethodInfo? accessorMethod = GetAccessorMethod(member);
36+
MethodInfo? accessorMethod = GetMostAccessibleAccessor(member);
3737
if (accessorMethod == null)
3838
{
3939
// If we got here, this is a inherited PropertyInfo that only had private accessors and is now refusing to give them out
@@ -99,5 +99,20 @@ public sealed override bool OkToIgnoreAmbiguity(PropertyInfo m1, PropertyInfo m2
9999

100100
return accessor;
101101
}
102+
103+
private static MethodInfo? GetMostAccessibleAccessor(PropertyInfo property)
104+
{
105+
MethodInfo? getter = property.GetMethod;
106+
MethodInfo? setter = property.SetMethod;
107+
108+
if (getter == null)
109+
return setter;
110+
if (setter == null)
111+
return getter;
112+
113+
// Return the setter if it's more accessible, otherwise return the getter.
114+
// MethodAttributes acessibility values are higher for more accessible methods: private (1) --> public (6).
115+
return (setter.Attributes & MethodAttributes.MemberAccessMask) > (getter.Attributes & MethodAttributes.MemberAccessMask) ? setter : getter;
116+
}
102117
}
103118
}

‎src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ public sealed override IEnumerable<PropertyInfo> CoreGetDeclaredMembers(RuntimeT
2525

2626
public sealed override void GetMemberAttributes(PropertyInfo member, out MethodAttributes visibility, out bool isStatic, out bool isVirtual, out bool isNewSlot)
2727
{
28-
MethodInfo? accessorMethod = GetAccessorMethod(member);
28+
MethodInfo? accessorMethod = GetMostAccessibleAccessor(member);
2929
if (accessorMethod == null)
3030
{
3131
// If we got here, this is a inherited PropertyInfo that only had private accessors and is now refusing to give them out
@@ -83,5 +83,20 @@ public sealed override bool OkToIgnoreAmbiguity(PropertyInfo m1, PropertyInfo m2
8383

8484
private static MethodInfo? GetAccessorMethod(PropertyInfo property) =>
8585
property.GetMethod ?? property.SetMethod;
86+
87+
private static MethodInfo? GetMostAccessibleAccessor(PropertyInfo property)
88+
{
89+
MethodInfo? getter = property.GetMethod;
90+
MethodInfo? setter = property.SetMethod;
91+
92+
if (getter == null)
93+
return setter;
94+
if (setter == null)
95+
return getter;
96+
97+
// Return the setter if it's more accessible, otherwise return the getter.
98+
// MethodAttributes acessibility values are higher for more accessible methods: private (1) --> public (6).
99+
return (setter.Attributes & MethodAttributes.MemberAccessMask) > (getter.Attributes & MethodAttributes.MemberAccessMask) ? setter : getter;
100+
}
86101
}
87102
}

‎src/libraries/System.Reflection.MetadataLoadContext/tests/System.Reflection.MetadataLoadContext.Tests.csproj

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,10 @@
7373
<Compile Include="src\TestUtils\TypeWrapper.cs" />
7474
</ItemGroup>
7575

76+
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
77+
<Compile Include="$(CoreLibSharedDir)System\Runtime\CompilerServices\IsExternalInit.cs" Link="Common\System\Runtime\CompilerServices\IsExternalInit.cs" />
78+
</ItemGroup>
79+
7680
<ItemGroup>
7781
<ProjectReference Include="..\src\System.Reflection.MetadataLoadContext.csproj" />
7882
</ItemGroup>

‎src/libraries/System.Reflection.MetadataLoadContext/tests/src/SampleMetadata/SampleMetadata.cs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,33 @@ public class PropertyHolder1<T>
526526

527527
public class DerivedFromPropertyHolder1<T> : PropertyHolder1<T> { }
528528

529+
public class PropertyWithNonPublicAccessors
530+
{
531+
// Property with public setter but private getter - should be considered public
532+
public string? PublicSetterPrivateGetter { private get; set; }
533+
534+
// Property with both public getter and setter - should be considered public
535+
public string? PublicGetterPublicSetter { get; set; }
536+
537+
// Property with only public setter (no getter) - should be considered public
538+
public string? OnlyPublicSetter { set { } }
539+
540+
// Property with both private getter and setter - should not be considered public
541+
private string? PrivateGetterPrivateSetter { get; set; }
542+
543+
// Property with both internal getter and setter - should not be considered public
544+
internal string? InternalGetterInternalSetter { get; set; }
545+
546+
// Property with public getter but internal setter - should be considered public
547+
public string? PublicGetterInternalSetter { get; internal set; }
548+
549+
// Property with internal getter but public setter - should be considered public
550+
public string? InternalGetterPublicSetter { internal get; set; }
551+
552+
// Property with internal getter but public init - should be considered public (just like public setter as they are the same from Reflection's perspective)
553+
public string? InternalGetterPublicInit { internal get; init; }
554+
}
555+
529556
public class EventHolder1<T>
530557
{
531558
public event Action<T> MyEvent { add { throw null!; } remove { throw null!; } }

‎src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Property/PropertyTests.cs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,5 +239,40 @@ public static unsafe void TestCustomModifiers1()
239239
TestUtils.AssertNewObjectReturnedEachTime(() => p.GetOptionalCustomModifiers());
240240
}
241241
}
242+
243+
[Fact]
244+
public static void TestPropertiesWithNonPublicAccessors()
245+
{
246+
Type t = typeof(PropertyWithNonPublicAccessors).Project();
247+
248+
// Get all public instance properties
249+
PropertyInfo[] properties = t.GetProperties(BindingFlags.Instance | BindingFlags.Public);
250+
251+
// Should include properties with public setters even if getters are private
252+
Assert.Contains(properties, p => p.Name == nameof(PropertyWithNonPublicAccessors.PublicSetterPrivateGetter));
253+
Assert.Contains(properties, p => p.Name == nameof(PropertyWithNonPublicAccessors.PublicGetterPublicSetter));
254+
Assert.Contains(properties, p => p.Name == nameof(PropertyWithNonPublicAccessors.OnlyPublicSetter));
255+
Assert.Contains(properties, p => p.Name == nameof(PropertyWithNonPublicAccessors.PublicGetterInternalSetter));
256+
Assert.Contains(properties, p => p.Name == nameof(PropertyWithNonPublicAccessors.InternalGetterPublicSetter));
257+
258+
// Should not include properties where both accessors are non-public
259+
Assert.DoesNotContain(properties, p => p.Name == "PrivateGetterPrivateSetter");
260+
Assert.DoesNotContain(properties, p => p.Name == "InternalGetterInternalSetter");
261+
262+
// Verify specific property details
263+
PropertyInfo publicSetterPrivateGetterProp = t.GetProperty(nameof(PropertyWithNonPublicAccessors.PublicSetterPrivateGetter))!;
264+
Assert.NotNull(publicSetterPrivateGetterProp);
265+
Assert.True(publicSetterPrivateGetterProp.CanWrite);
266+
Assert.True(publicSetterPrivateGetterProp.CanRead); // Can read even though getter is private
267+
Assert.NotNull(publicSetterPrivateGetterProp.GetSetMethod(false)); // Public setter
268+
Assert.Null(publicSetterPrivateGetterProp.GetGetMethod(false)); // Private getter
269+
270+
PropertyInfo onlySetterProp = t.GetProperty(nameof(PropertyWithNonPublicAccessors.OnlyPublicSetter))!;
271+
Assert.NotNull(onlySetterProp);
272+
Assert.True(onlySetterProp.CanWrite);
273+
Assert.False(onlySetterProp.CanRead); // No getter at all
274+
Assert.NotNull(onlySetterProp.GetSetMethod(false)); // Public setter
275+
Assert.Null(onlySetterProp.GetGetMethod(false)); // No getter
276+
}
242277
}
243278
}

‎src/libraries/System.Runtime/tests/System.Reflection.Tests/PropertyInfoTests.cs

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,29 @@ public void SetValue_Invalid(Type type, string name, object obj, object value, o
160160
Assert.Throws(exceptionType, () => PropertyInfo.SetValue(obj, value, index));
161161
}
162162

163+
[Theory]
164+
[InlineData(nameof(PropertyInfoMembers.PublicGetIntProperty))]
165+
[InlineData(nameof(PropertyInfoMembers.PublicGetPublicSetStringProperty))]
166+
[InlineData(nameof(PropertyInfoMembers.PublicGetDoubleProperty))]
167+
[InlineData(nameof(PropertyInfoMembers.PublicGetFloatProperty))]
168+
[InlineData(nameof(PropertyInfoMembers.PublicGetEnumProperty))]
169+
[InlineData("PrivateGetPrivateSetIntProperty", false)]
170+
[InlineData(nameof(PropertyInfoMembers.PublicGetPrivateSetProperty))]
171+
[InlineData(nameof(PropertyInfoMembers.PrivateGetPublicSetProperty))]
172+
[InlineData(nameof(PropertyInfoMembers.PrivateGetPublicInitProperty))]
173+
public static void GetPublicProperties(string name, bool isPublic = true)
174+
{
175+
PropertyInfo property = typeof(PropertyInfoMembers).GetTypeInfo().GetProperty(name, BindingFlags.Public | BindingFlags.Instance);
176+
if (isPublic)
177+
{
178+
Assert.NotNull(property);
179+
}
180+
else
181+
{
182+
Assert.Null(property);
183+
}
184+
}
185+
163186
[Theory]
164187
[InlineData(nameof(PropertyInfoMembers.PublicGetIntProperty))]
165188
[InlineData(nameof(PropertyInfoMembers.PublicGetPublicSetStringProperty))]
@@ -168,6 +191,8 @@ public void SetValue_Invalid(Type type, string name, object obj, object value, o
168191
[InlineData(nameof(PropertyInfoMembers.PublicGetEnumProperty))]
169192
[InlineData("PrivateGetPrivateSetIntProperty")]
170193
[InlineData(nameof(PropertyInfoMembers.PublicGetPrivateSetProperty))]
194+
[InlineData(nameof(PropertyInfoMembers.PrivateGetPublicSetProperty))]
195+
[InlineData(nameof(PropertyInfoMembers.PrivateGetPublicInitProperty))]
171196
public static void GetRequiredCustomModifiers_GetOptionalCustomModifiers(string name)
172197
{
173198
PropertyInfo property = typeof(PropertyInfoMembers).GetTypeInfo().GetProperty(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
@@ -184,6 +209,8 @@ public static void GetRequiredCustomModifiers_GetOptionalCustomModifiers(string
184209
[InlineData(nameof(PropertyInfoMembers.PublicGetEnumProperty), 2, 2)]
185210
[InlineData("PrivateGetPrivateSetIntProperty", 0, 2)]
186211
[InlineData(nameof(PropertyInfoMembers.PublicGetPrivateSetProperty), 1, 2)]
212+
[InlineData(nameof(PropertyInfoMembers.PrivateGetPublicSetProperty), 1, 2)]
213+
[InlineData(nameof(PropertyInfoMembers.PrivateGetPublicInitProperty), 1, 2)]
187214
public static void GetAccessors(string name, int accessorPublicCount, int accessorPublicAndNonPublicCount)
188215
{
189216
PropertyInfo pi = typeof(PropertyInfoMembers).GetTypeInfo().GetProperty(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
@@ -200,6 +227,8 @@ public static void GetAccessors(string name, int accessorPublicCount, int access
200227
[InlineData(nameof(PropertyInfoMembers.PublicGetEnumProperty), true, true, true, true)]
201228
[InlineData("PrivateGetPrivateSetIntProperty", false, true, false, true)]
202229
[InlineData(nameof(PropertyInfoMembers.PublicGetPrivateSetProperty), true, true, false, true)]
230+
[InlineData(nameof(PropertyInfoMembers.PrivateGetPublicSetProperty), false, true, true, true)]
231+
[InlineData(nameof(PropertyInfoMembers.PrivateGetPublicInitProperty), false, true, true, true)]
203232
public static void GetGetMethod_GetSetMethod(string name, bool publicGet, bool nonPublicGet, bool publicSet, bool nonPublicSet)
204233
{
205234
PropertyInfo pi = typeof(PropertyInfoMembers).GetTypeInfo().GetProperty(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
@@ -528,6 +557,8 @@ public class PropertyInfoMembers
528557
public PublicEnum PublicGetEnumProperty { get; set; }
529558
private int PrivateGetPrivateSetIntProperty { get; set; }
530559
public int PublicGetPrivateSetProperty { get; private set; }
560+
public int PrivateGetPublicSetProperty { private get; set; }
561+
public int PrivateGetPublicInitProperty { private get; init; }
531562
}
532563
}
533564
}

0 commit comments

Comments
 (0)