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

Unity GetService for Struct Type with a default constructor calls the constructor #87

Open
eerhardt opened this issue Feb 1, 2021 · 9 comments

Comments

@eerhardt
Copy link

eerhardt commented Feb 1, 2021

I'm adding a test for dotnet/runtime#47722. In doing so, I created a Struct type that has a default constructor (note you can only do this in IL today, until dotnet/csharplang#99 is implemented).

Calling ActivatorUtilities.CreateInstance(provider, typeof(ClassWithOptionalArgsCtorWithStructs)) for a class whose constructor is defined like the following:

public struct StructWithPublicDefaultConstructor
{
    public readonly bool ConstructorInvoked;

    public StructWithPublicDefaultConstructor() // note can't do this in C# yet
    {
        ConstructorInvoked = true;
    }
}

public class ClassWithOptionalArgsCtorWithStructs
{
    public StructWithPublicDefaultConstructor StructWithConstructor { get; }

    public ClassWithOptionalArgsCtorWithStructs(
        StructWithPublicDefaultConstructor structWithConstructor = default
    )
    {
        StructWithConstructor = structWithConstructor;
    }
}

Fails the following test:

Assert.False(class.StructWithConstructor.ConstructorInvoked);

The constructor shouldn't have been invoked because I didn't register StructWithPublicDefaultConstructor as a service. However, debugging into it, the Unit ServiceProvider is finding the ConstructorInfo on the ValueType and invoking it.

Unity is the only DI provider with this behavior. The other providers return null for this Type, and the test passes.

I needed to special case Unity's behavior in this test. So I logged an issue to ensure this is the behavior we are expecting for Unity.

@ENikS
Copy link
Contributor

ENikS commented Feb 2, 2021

I am not sure, is this incorrect behavior?

@eerhardt
Copy link
Author

eerhardt commented Feb 2, 2021

is this incorrect behavior?

It is different behavior than what I would get if I would call new ClassWithOptionalArgsCtorWithStructs().

var c1 = new ClassWithOptionalArgsCtorWithStructs();
Console.WriteLine(c1.StructWithConstructor.ConstructorInvoked); // prints "false"

var c2 = ActivatorUtilities.CreateInstance(unityServiceProvider, typeof(ClassWithOptionalArgsCtorWithStructs));
Console.WriteLine(c2.StructWithConstructor.ConstructorInvoked); // prints "true"

@ENikS
Copy link
Contributor

ENikS commented Feb 2, 2021

Unity compiles pipeline as xyz = new StructWithPublicDefaultConstructor(); for dependencies.

It is concerning that you are using ActivatorUtilities.CreateInstance in so many places. For example, previous version allowed to create Startup using external DI, such as Unity, resolving dependencies from Unity. Now it is not possible and breaks code where dependencies are configured with Unity.

@eerhardt
Copy link
Author

eerhardt commented Feb 2, 2021

It is concerning that you are using ActivatorUtilities.CreateInstance in so many places.

I'm unit testing the behavior of ActivatorUtilities.CreateInstance because I am changing the code in it. 😉 See dotnet/runtime#47722.

@ENikS
Copy link
Contributor

ENikS commented Feb 2, 2021

I understand, the remark was just a general rant.

If I understand correctly, the behavior is not wrong and there is nothing required on my part to change it?

@eerhardt
Copy link
Author

eerhardt commented Feb 2, 2021

the behavior is not wrong and there is nothing required on my part to change it?

The behavior using a Unity ServiceProvider is different than all the other DI providers being tested in dotnet/runtime. If you are OK with having different behavior that's fine with me. I just opened this issue to raise awareness that this behavior is different, and ensure this behavior difference in Unity is expected and acceptable.

@ENikS
Copy link
Contributor

ENikS commented Feb 2, 2021

It would be rather hard to change. Unity creates pipeline like this:

return new ClassWithOptionalArgsCtorWithStructs( new StructWithPublicDefaultConstructor() );

I am not sure how this could be replaced with new ClassWithOptionalArgsCtorWithStructs( null ); and why. Could you please provide more context?

Optional or not, the struct is still allocated on the stack, so it is required to be initialized, what am I missing?

@eerhardt
Copy link
Author

eerhardt commented Feb 2, 2021

Could you please provide more context?

The reason Unity's behavior is different in ActivatorUtilities.CreateInstance is because the following code in ActivatorUtilities.CreateInstance:

            public object CreateInstance(IServiceProvider provider)
            {
                for (int index = 0; index != _parameters.Length; index++)
                {
                    if (_parameterValues[index] == null)
                    {
                        object? value = provider.GetService(_parameters[index].ParameterType);
                        if (value == null)
                        {
                            if (!ParameterDefaultValue.TryGetDefaultValue(_parameters[index], out object? defaultValue))

ActivatorUtilities tries calling provider.GetService(type) for each parameter in the constructor. No one has registered a service for StructWithPublicDefaultConstructor. However, the Unity service provider is still returning a boxed StructWithPublicDefaultConstructor object, which it created using the default constructor. Thus that boxed object is passed into the ClassWithOptionalArgsCtorWithStructs. All the other DI providers return null when asked for provider.GetService(typeof(StructWithPublicDefaultConstructor)) because no services for that type have been registered.

Optional or not, the struct is still allocated on the stack, so it is required to be initialized, what am I missing?

Check out dotnet/csharplang#99 for more info. But when you have a Struct Type that defines a parameterless constructor, the following lines of code do different things:

StructWithPublicDefaultConstructor c1 = default;
StructWithPublicDefaultConstructor c2 = new StructWithPublicDefaultConstructor();

c1 won't have its parameterless constructor called. c2 will. This is also a difference between calling GetUnitializedObject and Activator.CreateInstance. GetUnitializedObject won't invoke the constructor. Activator.CreateInstance will. That's why I am changing ActivatorUtilities' ParameterDefaultValue.TryGetDefaultValue method to use GetUnitializedObject - so the parameterless constructor doesn't get invoked. To align the behavior with = default.

@ENikS
Copy link
Contributor

ENikS commented Feb 2, 2021

No one has registered a service for StructWithPublicDefaultConstructor. However, the Unity service provider is still returning a boxed StructWithPublicDefaultConstructor object, which it created using the default constructor

By original design Unity attempts to create an instance even if it is not registered. It does not care if it is a class or struct as long as it could be instantiated. I will do some research but I am afraid this 'feature' can not be changed.

A type (event struct) could be annotated with attributes for initialization: fields, properties, methods and can not be created with default. A proper constructor must be invoked.

Unity already allows most of these, just with DefaultValue and other attributes: Allow no-arg constructor and field initializers...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants