Skip to content
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

Merged
merged 2 commits into from
Feb 5, 2021

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 1, 2021

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).

… 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)`.
@dotnet-issue-labeler
Copy link

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.

@ghost
Copy link

ghost commented Feb 1, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

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).

Author: eerhardt
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

@pakrym
Copy link
Contributor

pakrym commented Feb 1, 2021

I think we should add a test.

@pakrym
Copy link
Contributor

pakrym commented Feb 1, 2021

LGTM

@eerhardt
Copy link
Member Author

eerhardt commented Feb 1, 2021

I think we should add a test.

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 Microsoft.NET.Sdk.IL, but I'm not sure how much work that would be. My plan was to wait for dotnet/csharplang#99 to add a test. Thoughts?

@stephentoub
Copy link
Member

stephentoub commented Feb 1, 2021

We have a few IL test projects, e.g.
https://github.com/dotnet/runtime/tree/master/src/libraries/System.Runtime/tests/TestModule
https://github.com/dotnet/runtime/tree/master/src/libraries/System.Runtime/tests/TestStructs
https://github.com/dotnet/runtime/tree/master/src/libraries/System.Reflection.Metadata/tests/Resources/Interop
This one even has what you want :)

.class public sequential sealed StructWithPublicDefaultConstructor
extends [System.Runtime]System.ValueType
{
.field public initonly bool ConstructorInvoked
.method public hidebysig specialname rtspecialname instance void .ctor () cil managed
{
ldarg.0
ldc.i4.1
stfld bool System.Tests.StructWithPublicDefaultConstructor::ConstructorInvoked
ret
}
}

as part of testing CreateInstance:
[Fact]
public void CreateInstanceT_StructWithPublicDefaultConstructor_InvokesConstructor() =>
Assert.True(Activator.CreateInstance<StructWithPublicDefaultConstructor>().ConstructorInvoked);

@eerhardt
Copy link
Member Author

eerhardt commented Feb 1, 2021

Thanks @stephentoub! I didn't know about those tests. I added a test using that struct.

@eerhardt
Copy link
Member Author

eerhardt commented Feb 2, 2021

Test failures are #47728

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a 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.

@eerhardt eerhardt merged commit 21ff47c into dotnet:master Feb 5, 2021
@eerhardt eerhardt deleted the FixParameterDefaultValue branch February 5, 2021 18:00
@ghost ghost locked as resolved and limited conversation to collaborators Mar 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants