-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix Type.IsPublic for pointer and byref types #65156
Conversation
Tagging subscribers to this area: @dotnet/area-system-reflection Issue DetailsFixes #12355 Type.IsPublic returns false for pointer and byref types generated from a public type. Because
|
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.
Mono changes LGTM
760ec25
to
b6aa2c8
Compare
b6aa2c8
to
11076ec
Compare
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; some minor test feedback
@buyaa-n note some CI failures with the new tests |
src/libraries/System.Runtime/tests/System/Type/TypePropertyTests.cs
Outdated
Show resolved
Hide resolved
Does System.Reflection.MetadataLoadContext need this fix as well? |
Looking, thanks
Will check, thanks |
A question about the CI test failure: for runtime/src/mono/mono/metadata/icall.c Lines 1805 to 1806 in f1337d9
I beleive we need same behavior for this, but i am not sure which value is expected, I guess it should be public, but want to confirm, please let me know what you think @steveharter @jkotas @GrabYourPitchforks @lambdageek |
MLC just returning Line 28 in ced5e7a
Line 28 in ced5e7a
|
The value of |
Right 0, so it was same as what runtime/src/coreclr/vm/runtimehandles.cpp Lines 809 to 815 in 4c2ea7e
|
8dce35f
to
0b042b8
Compare
c721a18
to
a17a004
Compare
src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.cs
Outdated
Show resolved
Hide resolved
...System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoByRefType.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.MetadataLoadContext/tests/src/SampleMetadata/SampleMetadata.cs
Outdated
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.
LGTM. Thank you!
Thank you all the your review and suggestions! Build failure unrelated, merging |
Fixes #12355
Type.IsPublic returns false for pointer and byref types generated from a public type. Because
RuntimeTypeHandle::GetAttributes
returns 0 (NotPublic) for TypeDesc types (except generic type variables). Updated the code to return attributes from underlying element type (for pointer and byref types only)