-
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
Change ParameterDefaultValue to use GetUninitializedObject instead of Activator.CreateInstance #47722
Conversation
… Activator.CreateInstance Activator.CreateInstance has the ability to call a parameterless constructor (if one is defined) on a value type. This is incorrect for ParameterDefaultValue. It is more correct to call GetUninitializedObject, which is the same as using `default(T)`.
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue DetailsActivator.CreateInstance has the ability to call a parameterless constructor (if one is defined) on a value type. This is incorrect for ParameterDefaultValue. It is more correct to call GetUninitializedObject, which is the same as using
|
I think we should add a test. |
src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs
Show resolved
Hide resolved
LGTM |
I'm not sure how to add a test without dotnet/csharplang#99. I would have to write a value type in IL that defines a parameterless constructor. Maybe we could do it with |
We have a few IL test projects, e.g. runtime/src/libraries/System.Runtime/tests/TestStructs/System.TestStructs.il Lines 25 to 37 in 2ffb39e
as part of testing CreateInstance: runtime/src/libraries/System.Runtime/tests/System/ActivatorTests.Generic.cs Lines 47 to 49 in 1d9e50c
|
Thanks @stephentoub! I didn't know about those tests. I added a test using that struct. |
src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.External.Tests/Unity.cs
Show resolved
Hide resolved
Test failures are #47728 |
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 logic in ParameterDefaultValue.cs LGTM.
Activator.CreateInstance has the ability to call a parameterless constructor (if one is defined) on a value type. This is incorrect for ParameterDefaultValue. It is more correct to call GetUninitializedObject, which is the same as using
default(T)
.