ActivatorUtilities.CreateInstance depends on the order of constructors' definitions

[quixoticaxis]

Description

Current behavior

The effect of calling ActivatorUtilities.CreateInstance methods (any of the overloads) depends on the order in which the constructors of the type being created are defined. The instance of the class is created if constructors are defined in one order, but the creation throws if constructors are defined in reversed order (see the code below).

Expected behavior

The call throws because it failed to disambiguate the appropriate constructor. This behavior will be in-line with the behavior of Activator.CreateInstance.

Reproduction

using Microsoft.Extensions.DependencyInjection;

using static System.Console;

namespace AUCI
{
    public class A { }
    public class B { }
    public class C { }
    public class S { }

    public class Creatable
    {
        public Creatable(
            A a,
            B b,
            C c,
            S s)
        {
            WriteLine("Creatable.Long");
        }
        
        public Creatable(
            A a,
            C c,
            S s)
        {
            WriteLine("Creatable.Short");
        }
    }

    public class Program
    {
        public static void Main(string[] args)
        {
            var services = new ServiceCollection();
            services.AddScoped<S>();
            using var provider = services.BuildServiceProvider();

            var instance = ActivatorUtilities.CreateInstance(
                provider,
                typeof(Creatable),
                // Please, note that the order of parameters does not match the order of constructors' arguments
                new C(), new A()); 
        }       
    }
}

The code above throws when invoking ActivatorUtilities.CreateInstance, but works fine if we define the constructors in reversed order:

public class Creatable
    {        
        public Creatable(
            A a,
            C c,
            S s)
        {
            WriteLine("Creatable.Short");
        }

        public Creatable(
            A a,
            B b,
            C c,
            S s)
        {
            WriteLine("Creatable.Long");
        }
    }

It's counter-intuitive, IMHO.

Additional findings

I've looked through the implementation and debugged it:

1. the first call to ConstructorMatcher.Match returns 0 (meaning that the constructor is the worst possible match, the constructor does not match our parameters at all);
2. the second call to ConstructorMatcher.Match returns 0, so it's not better than the first one and we dismiss it;
3. we create the instance with the best match we've found (even knowing that it is ambiguous and does not match the parameters at all).

Configuration

The issue is not specific to configuration.

Regression?

Does not seem to be a regression.

Other information

It's probably a corner case, but it still would be great to remove the undefined (as I understand it) behavior.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[quixoticaxis]

@GrabYourPitchforks sorry, isn't it area-Extensions-DependencyInjection? ActivatorUtilities comes from DependencyInjection.

[GrabYourPitchforks]

@quixoticaxis You're right - apologies!

[ghost]

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

Issue Details

---

Description

Current behavior

The effect of calling ActivatorUtilities.CreateInstance methods (any of the overloads) depends on the order in which the constructors of the type being created are defined. The instance of the class is created if constructors are defined in one order, but the creation throws if constructors are defined in reversed order (see the code below).

Expected behavior

The call throws because it failed to disambiguate the appropriate constructor. This behavior will be in-line with the behavior of Activator.CreateInstance.

Reproduction

using Microsoft.Extensions.DependencyInjection;

using static System.Console;

namespace AUCI
{
    public class A { }
    public class B { }
    public class C { }
    public class S { }

    public class Creatable
    {
        public Creatable(
            A a,
            B b,
            C c,
            S s)
        {
            WriteLine("Creatable.Long");
        }
        
        public Creatable(
            A a,
            C c,
            S s)
        {
            WriteLine("Creatable.Short");
        }
    }

    public class Program
    {
        public static void Main(string[] args)
        {
            var services = new ServiceCollection();
            services.AddScoped<S>();
            using var provider = services.BuildServiceProvider();

            var instance = ActivatorUtilities.CreateInstance(
                provider,
                typeof(Creatable),
                // Please, note that the order of parameters does not match the order of constructors' arguments
                new C(), new A()); 
        }       
    }
}

The code above throws when invoking ActivatorUtilities.CreateInstance, but works fine if we define the constructors in reversed order:

public class Creatable
    {        
        public Creatable(
            A a,
            C c,
            S s)
        {
            WriteLine("Creatable.Short");
        }

        public Creatable(
            A a,
            B b,
            C c,
            S s)
        {
            WriteLine("Creatable.Long");
        }
    }

It's counter-intuitive, IMHO.

Additional findings

I've looked through the implementation and debugged it:

1. the first call to ConstructorMatcher.Match returns 0 (meaning that the constructor is the worst possible match, the constructor does not match our parameters at all);
2. the second call to ConstructorMatcher.Match returns 0, so it's not better than the first one and we dismiss it;
3. we create the instance with the best match we've found (even knowing that it is ambiguous and does not match the parameters at all).

Configuration

The issue is not specific to configuration.

Regression?

Does not seem to be a regression.

Other information

It's probably a corner case, but it still would be great to remove the undefined (as I understand it) behavior.

Author:	quixoticaxis
Assignees:	-
Labels:	area-Extensions-DependencyInjection, untriaged
Milestone:	-

[KalleOlaviNiemitalo]

This may be what causes #45119.

It isn't clear to me how this should be fixed, though. If

• ActivatorUtilities is being asked to create an instance of class A with an IServiceProvider and no other arguments;
• class A has two constructors A(B, C) and A(D, E), neither of which has ActivatorUtilitiesConstructorAttribute;
• the IServiceProvider is able to create instances of B, D, and E, but not C;

then, I think it would be best if ActivatorUtilities selected the A(D, E) constructor and requested only instances of D and E from the IServiceProvider. However, IServiceProvider does not provide a way for ActivatorUtilities to ask whether it can provide B and C, without actually creating an instance of B. Thus, the IServiceProvider might have to create an instance of B, and perhaps instances of services required by B, even though ActivatorUtilities would be unable to use them in the end.

Perhaps that would not matter in practice.

[quixoticaxis]

@KalleOlaviNiemitalo I personally would prefer the call to fail in any case of ambiguity and make using an attribute a must for the case when the class has more than one constructor and CreateInstance is invoked without arguments. IMHO, it would make users' code more robust in the long run.

Also a separate method or an overload that takes Type[] constructorSignature can probably be introduced to cover the scenarios where the constructed class cannot be edited or when the user does not want to introduce the dependency on the attribute into the code.

[tarekgh]

I personally would prefer the call to fail in any case of ambiguity and make using an attribute a must for the case when the class has more than one constructor and CreateInstance is invoked without arguments.

There is no ambiguity here. what is happening is you are providing the class with the two constructors.

        public Creatable(A a, B b, C c,S s)
        public Creatable(A a, C c,S s)

It tries to find which constructor can be used to create the object when passing the parameters new C() and new A(). In checking the constructors, it finds both constructors having types for the whole list of the parameters. So, it detects both constructors can satisfy the operation and then select the first.

When having public Creatable(A a, B b, C c,S s) as the first constructor, it will use it and try to create the object using it. The problem with that is it cannot create instance of B parameter object and that is what is causing it to fail and throw the exception.
When having public Creatable(A a, C c,S s) as the first constructor, it succeeds because you supplied the a and c parameters, and s parameter can be created using the DI because you have registered it there.

The right fix here is when searching for suitable constructor to use to create the object, it should check if all parameters of such constructor can be created and choose it if it does. Otherwise, it shouldn't select this constructor. The only thing with this is, would be there any perf concern doing that? that is the part that need farther investigation. @davidfowl do you think the perf will matter much in such cases? to be more specific:

In the line

runtime/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Line 63 in 2c8f2dc

bestMatcher = matcher;

before setting the best matcher, it should perform the following checks on all missing constructor parameters:

                        provider.GetService(_parameters[index].ParameterType) != null || 
                        ParameterDefaultValue.TryGetDefaultValue(_parameters[index], out object? defaultValue) // We may store the created object if needed so we don't have to recreate it again in `CreateInstance(IServiceProvider provider)`. or try not creating the parameter object at all.

[quixoticaxis]

@tarekgh Sorry, I probably don't understand something.

There is no ambiguity here. what is happening is you are providing the class with the two constructors.
…
So, it detects both constructors can satisfy the operation and then select the first.

You say that there is no ambiguity, then say that the code picks the first of the constructors. Is there any notion of constructor/member ordering in C#?
I've never seen any specification on member ordering, and while I can imagine it for classes defined in a single source file, I'm not certain what would be the "first constructor" for the partial class.

[tarekgh]

Is there any notion of constructor/member ordering in C#?

Yes, we use reflection and it give us the constructors (and methods) in specific order. Usually, we encourage to not taking dependency on such order. but for ActivatorUtlities just go through the list we get from the reflection.

I've never seen any specification on member ordering, and while I can imagine it for classes defined in a single source file, I'm not certain what would be the "first constructor" for the partial class.

Yes, you shouldn't assume any order in general. but at the end if you are requesting the list of constructors, would be the reflection to decide which order will give to you. If you look at https://docs.microsoft.com/en-us/dotnet/api/system.type.getconstructors?view=net-5.0 it is clearly state in the Remarks section The GetConstructors method does not return constructors in a particular order, such as declaration order. Your code must not depend on the order in which constructors are returned, because that order varies. but this doesn't mean when you change the order in the source code will not affect the order returned from the reflection. it is up to the reflection to return the list in the order it desire.

You may try manually run

foreach (ConstructorInfo? constructor in instanceType.GetConstructors())

runtime/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Line 41 in 2c8f2dc

foreach (ConstructorInfo? constructor in instanceType.GetConstructors())

And look at the order in your case and when you switch the order in the source code too.

[davidfowl]

This is problematic, there's another issue for this somewhere.

[KalleOlaviNiemitalo]

@davidfowl might you mean #46272 for GetProperties?

[quixoticaxis]

@tarekgh this is what I was speaking about. As far as the documented contract goes, it does not even guarantee the ordering being stable between two calls to the API. Maybe we treat the term "ambiguous" differently, but I suppose that the choice between two equally suitable variants based on implementation details can be called "ambiguous" (or "undefined", but I cannot see any meaningful way to define the current behavior.)

[tarekgh]

@quixoticaxis we must consider the purpose of the API. it is creating an instance of object. If the user code giving two ways (i.e. two legit constructors) and then we fail that will look weird too.

The current behavior can be easily defined as, CreateInstance tries to create instance of the object by finding any legit constructor satisfy the creation using the input parameters.

[quixoticaxis]

Sorry, misclicked "Close".

@maryamariyan could you, please, move the issue back to the 6.0.0 milestone?

[quixoticaxis]

@tarekgh I see, thank you for the explanation.
So the purpose of the API is to make object creation as easy as it can be. I personally would have preferred a more strict contract that enables usage in fallback scenarios, for example, when one needs a particular consructor, but I can see your point now.

[davidfowl]

CreateFactory and CreateInstance should behave similarly. Today they don't and that's broken

[SteveDunn]

This issue is mentioned in the work planned for .NET 7 story, but it doesn't look like it's been touched in a while. Is it still planned for .NET 7? If someone where to have a crack at it now, would it make it into .NET 7?

[eerhardt]

We've moved it from the work planned for .NET 7 story. It is no longer planned for .NET 7 (unfortunately).

[mnmr]

Seeing that this issue is still open, I have an additional concern that I'd like you to consider when tackling this.

Orleans is using ActivatorUtilities to create objects, but would prefer to have internalized attributes so that they can follow a consistent naming scheme (for ease of discovery). That is, we'd like to be able to define an OrleansConstructorAttribute and then somehow tell ActivatorUtilities that this corresponds to the ActivatorUtilitiesConstructorAttribute. ActivatorUtilities would effectively have to match attribute names against a list of possible names, rather than looking for a single name match.

Given the broad utility of ActivatorUtilities, I can see how quite a few projects could benefit from being able to reuse this logic without sacrificing discoverability of their attributes.

[eerhardt]

would prefer to have internalized attributes

@mnmr - that is a new scenario, not what this issue is tracking. Please open a new API proposal issue for your scenario.

[maryamariyan]

I've started working on this issue and looked at the above conversations, linked issues, and use cases and approaches that were considered.

Given all the information presented in this issue, here's a run down of the conclusions and plan of action and the basic idea behind what the CreateInstance API should do:
(link to current implementation)

1 - It first looks a constructor using ActivatorUtilitiesConstructorAttribute if it exists it tries to match its arguments with the given parameters, if they don't match it throws. (existing behaviour)
2 - If multiple constructors have the attribute it will throw. (existing behaviour)
3 - When no constructor has that attribute, we go with the following rule: (new behaviour)

Find longest constructor which:
(a) matches all given parameters (existing behaviour)
(b) has the most DI services registered (new behaviour: Checking using the IServiceProviderIsService API)
(c) with most default values that match (new behaviour)

Rank all the matched constructors with the number of arguments that fitted the above criteria. If we find only one constructor with the biggest rank, we pick that to instantiate the item with.
Otherwise, if more than one constructor matched the same number of arguments, then we throw with ambiguous exception.

Note about ordering:

• The order of the parameters passed to CreateInstance do not matter
• The order of registered services do not matter
• The order of constructor definitions also should not matter. (Issue being fixed here)

In a later comment I'll build up all test cases and provide cases that would end up having behaviour change due to this.

[Eilon]

@maryamariyan thanks for the write-up. This all seems reasonable to me. I'd also love to see specific examples of scenarios that: 1. didn't work before, but now work, and 2. worked before but now work differently (e.g. different overload chosen, or throws). Also my understanding is that IServiceProviderIsService isn't necessarily implemented by the provider (it's new, right?) so it would be good to note what the behavior is if that is not available.

@eerhardt @davidfowl and any other folks watching: I think it's super important for us to get feedback on whether this is overall a good change, including any breaking aspects (such as throwing on cases that would now be considered ambiguous).

[eerhardt]

I'd also love to see specific examples of scenarios that: 1. didn't work before, but now work

There are 2 instances of Maui hitting this issue that caused them to stop using ActivatorUtilities:

dotnet/maui#4318 (comment)
dotnet/maui#4280 (comment)

That first one is the canonical case for what needs to be fixed here:

Fails:

    public LoginView(RandomItem item)
    {
        //this is constructor is invoked by code and RandomItem is not registered in DI
        InitializeComponent();
    }
    public LoginView()
    {
        InitializeComponent();
    }

Works:

    public LoginView()
    {
        InitializeComponent();
    }
    public LoginView(RandomItem item)
    {
        //this is constructor is invoked by code and RandomItem is not registered in DI
        InitializeComponent();
    }

The only difference is the order in which the ctors are defined.

[maryamariyan]

Took use cases in comment #46132 (comment) and updated the expected result with latest considerations.

Key use cases to consider:

• [case 3] When a default value is specified then give it a better rank
• [case 4] When ctor(A a) and ctor(B b) are present but both A and B are registered we should throw since it is ambiguous which to choose. This is possible using IServiceProviderIsService.
• [case 7] When a specified instance provided as argument is missing then throw.

#	ctors	IServiceProviderIsService	.CreateInstance .CreateFactory args	expected result	CreateInstance	CreateFactory
1	(A) (A, B)	B	(A)	a) ✅ (A, B) b) 🟥 (A) c) 🟥 throw	(A)	🟥
2	(A) (A, B)	-	(A)	a) 🟥 (A, B) b) ✅ (A) c) 🟥 throw	(A)	🟥
3	(A) (A, B=default)	-	(A)	a) ✅ (A, B) b) 🟥 (A) c) 🟥 throw	(A)	🟥
4	(A, B) (A, C)	B, C	(A)	a) 🟥 (A, B) b) 🟥 (A, C) c) ✅ throw	(A, B)	🟥
5	(A, B) (A, C)	B	(A)	a) ✅ (A, B) b) 🟥 (A, C) c) 🟥 throw	(A, B)	🟥
6	[ActivatorUtilitiesConstructor] (A) (A, B)	B	(A)	a) ✅ (A) b) 🟥 throw	(A)	(A)
7	(A) [ActivatorUtilitiesConstructor] (A, B)	-	(A)	a) 🟥 (A, B) b) ✅ throw	🟥	🟥
8	(A, B)	-	(B, A)	a) ✅ (A, B) b) 🟥 throw	(A, B)	(A, B)
9	(A, B)(B, A)	-	(B, A)	a) 🟥 (A, B) b) 🟥 (B, A) c) ✅ throw	(B, A)	🟥
10	(A, C, B)	C	(A, B)	a) ✅ (A, C, B) b) 🟥 throw	(A, C, B)	(A, C, B)

[maryamariyan]

cc @tillig @halter73 since you both just reviewed the PR, please also consider the above use cases, of how the proposal would cause breaking change.

[halter73]

I think no matter what we do, we need to align the behaviors of CreateFactory and CreateInsatance. If we're going to have to break people no matter what to align behaviors, I think going with the stricter unambiguous CreateFactory behavior makes sense.

[eerhardt]

Agreed - where it is possible, we should have aligned behaviors between the two.

One thing to remember is that CreateInstance has more information and can make a better decision than CreateFactory. This is because CreateInstance gets access to the IServiceProvider and can query IServiceProviderIsService to see if a Type is a registered service. CreateFactory can't do that since it doesn't have access to an IServiceProvider when picking a ctor.

[maryamariyan]

I think it should be fine that ActivatorUtilities.CreateFactory is stricter than CreateInstance, CreateFactory can be easily further disambiguated by passing more types as arguments to pick the right constructor.

 CreateFactory(Type, Type[])

// Simply picks an unambiguous constructor based on the provided types

// vs.

 CreateInstance(IServiceProvider, Type, Object[])
 CreateInstance<T>(IServiceProvider, Object[])

// Uses `Object[]` instances to look for required types in a constructor (the behaviour today is buggy and inaccurate)
// Proposal:
// Use `IServiceProvider` to look for registered services
// Pick the longest constructor with all the required instances
// Pick the longest constructor with all the most registered services and including default parameters

Contrast that with CreateInstance where it not only receives IServiceProvider (helpful for finding registered services), but also it takes instances (rather than types) of constructor arguments as arguments. There is benefit in making CreateInstance more flexible because for the user to further disambiguate with CreateInstance, there is extra hassle with passing down an instance rather than a type as done for CreateFactory.

As a result, I am proposing for CreateInstance to remain more flexible than CreateFactory. CreateInstance should be able to use IServiceProvider to pick the constructor with longest acceptable arguments when there is ambiguity.

[maryamariyan]

Here's the latest consensus:

• Goal: We want to completely fix the bug with behaviour dependence on order of constructor definition.
• All the instance arguments passed in to CreateInstance were required before and should remain required after the breaking change.
• Generally we do not want CreateInstance to be as strict as CreateFactory.
• CreateInstance needs to account for the case where a container is potentially misconfigured. If for any reason IServiceProviderIsService ends returns false for a service which is actually registered, should still work today.

For example, given:

public class A
{
   A(B b) { }
}

ActivatorUtilities.CreateInstance(provider, typeof(A)) should be able to properly instantiate A if B is registered. Even if IServiceProviderIsService is not available or IServiceProviderIsService.IsService for B returns false for some reason incorrectly.

General idea:

• Try to find the longest constructor matching all parameters based on the behaviour of IServiceProviderIsService.
• If none are found, try to fallback to CreateFactory, more than one throw
• if IServiceProviderIsService is not present fallback to CreateFactory.
• If IServiceProviderIsService is configured wrong, or does not exist, it is OK for the API to function incorrectly or ambiguously.

[eerhardt]

If more than one found, try to fallback to CreateFactory, more than one throw

If none are found, try to fallback to CreateFactory, more than one throw

[maryamariyan]

This change will be in .NET 8.0 Preview 1

[Breaking change]: ActivatorUtilities.CreateInstance behaves consistently independent of ctor definition order
Please let us know if you have any concerns.