-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix CustomAttributeTypedArgument.ToString() throwing an exception on Mono #118948
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
base: main
Are you sure you want to change the base?
Conversation
The function ToString() only takes into consideration how CoreClr's CustomAttributeTypedArgument is implemented and does a cast accordingly which causes it to fail with an InvalidCastException on Mono. So the casts should be runtime specific for it to not throw an InvalidCastException.
|
Tagging subscribers to this area: @dotnet/area-system-reflection |
| if (ArgumentType.IsArray) | ||
| { | ||
| #if !MONO | ||
| IList<CustomAttributeTypedArgument> array = (IList<CustomAttributeTypedArgument>)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.
Value is documented to return ReadOnlyCollection<CustomAttributeTypedArgument> for arrays: https://learn.microsoft.com/en-us/dotnet/api/system.reflection.customattributetypedargument.value?view=net-9.0#remarks
Should this issue be fixed in the implementation of Value property on Mono instead?
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.
@jkotas I couldn't figure out where to change the implementation of Value. Could you maybe help me point to it?
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.
I would expect that the fix is needed in ves_icall_System_Reflection_RuntimeCustomAttributeData_ResolveArgumentsInternal or one of the methods that it calls.
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.
I have assigned this to copilot: #119293. Let's see whether it can figure it out.
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.
You can take a look at the commits in #119293 for some inspiration where to fix this. (The changes in that PR are not quite right - if nothing else the changes in the shared code need to be under MONO ifdef.)
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.
@jkotas I can't seem to figure out a way to get this working. The change makes mono crash irrespective of whether I do it in the C# code or C code.
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.
@jkotas Can we make the code under a MONO ifdef?
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.
The fix should be fixing the implementation of Value property to follow the documented behavior.
I am ok with some under MONO ifdef if there is no better way.
CustomAttributeTypedArgument is structured differently in Mono and CoreClr and the function ToString() only takes into consideration how CoreClr's CustomAttributeTypedArgument is implemented and does a cast accordingly which causes it to fail with an InvalidCastException on Mono. So the casts should be runtime specific for it to not throw an InvalidCastException.
For Example, this is a standalone program which throws an InvalidCastException on Mono but not on CoreClr:
Output on Mono without the fix:
Output on CoreClr:
Output on Mono with the fix: