-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix PropertyPolicies to consider both getter and setter accessibility #116080
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: ericstj <[email protected]>
TaskHostFactory
does not work with public properties with private get accessor as task parameters
...ion.MetadataLoadContext/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs
Outdated
Show resolved
Hide resolved
…ostAccessibleAccessor Co-authored-by: ericstj <[email protected]>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:If you need me to access, download, or install something from one of these locations, you can either:
|
...ion.MetadataLoadContext/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs
Outdated
Show resolved
Hide resolved
…tion into GetAccessibilityRank Co-authored-by: ericstj <[email protected]>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, lets move this out of draft. @jkotas @AaronRobinsonMSFT would you like to review?
...ion.MetadataLoadContext/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Property/PropertyTests.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Property/PropertyTests.cs
Outdated
Show resolved
Hide resolved
...ion.MetadataLoadContext/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs
Outdated
Show resolved
Hide resolved
.../System.Private.CoreLib/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.MetadataLoadContext/tests/src/SampleMetadata/SampleMetadata.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See feedback above
.../System.Private.CoreLib/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs
Outdated
Show resolved
Hide resolved
I think this is ready for another pass when folks are ready. |
return GetAccessibilityRank(setter) > GetAccessibilityRank(getter) ? setter : getter; | ||
|
||
// Define accessibility ranking | ||
static int GetAccessibilityRank(MethodInfo method) | ||
{ | ||
MethodAttributes access = method.Attributes & MethodAttributes.MemberAccessMask; | ||
return access switch | ||
{ | ||
MethodAttributes.Public => 4, | ||
MethodAttributes.Family => 3, // protected | ||
MethodAttributes.Assembly => 3, // internal | ||
MethodAttributes.FamORAssem => 3, // protected internal | ||
MethodAttributes.FamANDAssem => 2, // protected and internal | ||
MethodAttributes.Private => 1, | ||
_ => 0 | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return GetAccessibilityRank(setter) > GetAccessibilityRank(getter) ? setter : getter; | |
// Define accessibility ranking | |
static int GetAccessibilityRank(MethodInfo method) | |
{ | |
MethodAttributes access = method.Attributes & MethodAttributes.MemberAccessMask; | |
return access switch | |
{ | |
MethodAttributes.Public => 4, | |
MethodAttributes.Family => 3, // protected | |
MethodAttributes.Assembly => 3, // internal | |
MethodAttributes.FamORAssem => 3, // protected internal | |
MethodAttributes.FamANDAssem => 2, // protected and internal | |
MethodAttributes.Private => 1, | |
_ => 0 | |
}; | |
} | |
return (setter.Attributes & MethodAttributes.MemberAccessMask) > (getter.Attributes & MethodAttributes.MemberAccessMask) ? setter : getter; |
I do see the rationale behind the ranking. Why is FamANDAssem rank 2, and all Family, Assembly and FamORAssem share a rank 3?
I think it can be simplified down to just comparing the MemberAccess value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let me also add tests for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this only ever considers Private vs Public, so it's not possible test the difference between family and assembly -
Lines 132 to 142 in 624737e
if (inBaseClass && visibility == MethodAttributes.Private) | |
continue; | |
if (numCandidatesInDerivedTypes != 0 && policies.IsSuppressedByMoreDerivedMember(member, queriedMembers._members, startIndex: 0, endIndex: numCandidatesInDerivedTypes)) | |
continue; | |
BindingFlags allFlagsThatMustMatch = default; | |
allFlagsThatMustMatch |= (isStatic ? BindingFlags.Static : BindingFlags.Instance); | |
if (isStatic && inBaseClass) | |
allFlagsThatMustMatch |= BindingFlags.FlattenHierarchy; | |
allFlagsThatMustMatch |= ((visibility == MethodAttributes.Public) ? BindingFlags.Public : BindingFlags.NonPublic); |
I appreciate more what you're suggesting around unifying these implementations - this query abstraction ends up doing more work than really necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks
Fixes an issue where
PropertyPolicies.GetMemberAttributes
in MetadataLoadContext only considered getter accessibility when determining property visibility, causing properties with public setters but private getters to be incorrectly excluded fromGetProperties(BindingFlags.Public)
.Problem
TaskHostFactory
does not work with public properties that have private get accessors as task parameters. For example:When MSBuild's TaskHostFactory uses MetadataLoadContext to discover task properties, the S1 property above would not be found because
PropertyPolicies.GetMemberAttributes
only looked at the getter accessibility (private) rather than considering that the setter is public.Root Cause
The issue was in
PropertyPolicies.GetAccessorMethod()
which returnedproperty.GetMethod ?? property.SetMethod
, prioritizing the getter. When the getter was private but setter was public, the property was marked as private and excluded from public property enumeration.Solution
Modified
PropertyPolicies.GetMemberAttributes
to useGetMostAccessibleAccessor()
instead ofGetAccessorMethod()
Added
GetMostAccessibleAccessor()
method that:Applied fix to both implementations:
src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/BindingFlagSupport/PropertyPolicies.cs
Testing
Added comprehensive tests that verify:
Example
Before this fix:
After this fix:
This ensures MetadataLoadContext follows the same property visibility semantics as regular .NET reflection, where a property is considered public if any accessor is public.
Fixes #116012.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
badhost
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile System.Net.Http.Functional.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.2-beta.25260.104/build/../tools/net/xunit.console.dll System.Net.Http.Functional.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
(dns block)does.not.exist.sorry
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest <SendAsync_ConnectionFailure_RecordsActivitiesWithCorrectErrorInfo>g__RunTest|18_0 /tmp/yinewg3b.bbx 1.1 False dns
(dns block)/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest <SendAsync_ConnectionFailure_RecordsActivitiesWithCorrectErrorInfo>g__RunTest|18_0 /tmp/ycho5lgn.idp 1.1 True dns
(dns block)/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest <SendAsync_ConnectionFailure_RecordsActivitiesWithCorrectErrorInfo>g__RunTest|18_0 /tmp/y0we1k00.s3k 2.0 True dns
(dns block)nosuchhost.invalid
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile System.Net.Http.Functional.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.2-beta.25260.104/build/../tools/net/xunit.console.dll System.Net.Http.Functional.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
(dns block)/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest+<>c <SendAsync_ExpectedDiagnosticExceptionLogging>b__9_0 /tmp/2ckzwn2w.1tl 1.1 True
(dns block)/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.DiagnosticsTest+<>c <SendAsync_ExpectedDiagnosticExceptionActivityLogging>b__23_0 /tmp/nx20jrpq.dco 1.1 False
(dns block)server
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig System.DirectoryServices.Protocols.Tests.runtimeconfig.json --depsfile System.DirectoryServices.Protocols.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.2-beta.25260.104/build/../tools/net/xunit.console.dll System.DirectoryServices.Protocols.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
(dns block)www.microsoft.com
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/System.Net.Http.Functional.Tests.deps.json /home/REDACTED/work/runtime/runtime/artifacts/bin/System.Net.Http.Functional.Tests/Debug/net10.0-linux/Microsoft.DotNet.RemoteExecutor.dll System.Net.Http.Functional.Tests, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Net.Http.Functional.Tests.HttpClientHandler_ServerCertificates_Test+<>c <HttpClientUsesSslCertEnvironmentVariables>b__26_0 /tmp/tf3iqxu1.dyy 1.1 True
(dns block)www.some.example
/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net10.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Net.Http.Functional.Tests.runtimeconfig.json --depsfile System.Net.Http.Functional.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.2-beta.25260.104/build/../tools/net/xunit.console.dll System.Net.Http.Functional.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing
(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.