-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
GetType: keep throwing same exceptions as .NET 8 #114228
Conversation
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.
Pull Request Overview
This PR ensures that the exception behavior stays consistent with .NET 8 by differentiating between the exceptions thrown when using Type.GetType versus Assembly.GetType for invalid assembly names. Key changes include:
- Updating tests in GetTypeTests.cs to validate that FileLoadException is always thrown by Type.GetType regardless of throwOnError and that Assembly.GetType behaves as specified.
- Modifying TypeNameParser.cs to check if a top-level assembly was provided and to throw either FileLoadException or ArgumentException based on that.
- Adding and propagating the TopLevelAssemblyWasProvided property in TypeNameResolver.cs and TypeNameResolver.CoreCLR.cs.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
tests/System.Reflection.Tests/GetTypeTests.cs | Updated tests to assert correct exceptions for invalid assembly names. |
System/Reflection/Metadata/TypeNameParser.cs | Adjusted parsing logic and exception throwing based on assembly name validity and top-level assembly presence. |
System/Reflection/TypeNameResolver.cs | Added a TopLevelAssemblyWasProvided property to support the new validation logic. |
System/Reflection/TypeNameResolver.CoreCLR.cs | Propagated the TopLevelAssemblyWasProvided flag to the parser call. |
Tagging subscribers to this area: @dotnet/area-system-reflection |
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.
Thank you!
src/coreclr/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.CoreCLR.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Kotas <[email protected]>
/ba-g known issues |
Looks like this PR forgot to adjust the Native AOT implementation as well |
Lots of the failures in this build were in the test touched https://dev.azure.com/dnceng-public/public/_build/results?buildId=1005140&view=results /Users/runner/work/1/s/.packages/microsoft.dotnet.helix.sdk/10.0.0-beta.25203.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(44,5): error : Test System.Reflection.Tests.GetTypeTests.GetType_InvalidAssemblyName has failed. Check the Test tab or this console log: https://helix.dot.net/api/2019-06-17/jobs/0e93cafc-9702-4366-9278-169703aa02b7/workitems/System.Reflection.Tests/console |
The build analysis gave up on parsing all test failures. The ones that it did parse were all known except one that I have looked at and matched with a known issue. |
Keep throwing the same exceptions as we did in .NET 8. Namely:
Type.GetType
, always throwFileLoadException
for invalid assembly names no matter whatthrowOnError
is set to.Assembly.GetType
, throw only whenthrowOnError
is set, andArgumentException
(notFileLoadException
)fixes #113534
FWIW The app I've used for testing: