ActivatorUtilities - matching preferred constructor depends on the constructors order

[ktyl]

Description

Selecting the preferred constructor by ActivatorUtilitiesConstructor attribute works differently depending on the order of constructors.

Reproduction Steps

Let's consider the following class:

public class Foo 
{
    public Foo(Bar bar)
    {
        this.Bar = bar;
    }

    public Foo(Baz bar)
    {
        this.Baz = bar;
    } 
 
    public Bar Bar { get; }
    public Baz Baz { get; }
}

public class Bar {}
public class Baz {}

Let's assume that I want to instantiate this class using the Dependency Injection container:

var serviceCollection = new ServiceCollection();
serviceCollection.AddTransient<Foo>();
serviceCollection.AddTransient<Bar>();
serviceCollection.AddTransient<Baz>();

var serviceProvider = serviceCollection.BuildServiceProvider();

var foo = serviceProvider.GetService<Foo>();

This obviously fails because ActivatorUtilities class finds two matching constructors.
Fortunately there is a [ActivatorUtilitiesConstructorAttribute](https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilitiesConstructorAttribute.cs) so let's adjust the class Foo:

public class Foo 
{
    [ActivatorUtilitiesConstructor]
    public Foo(Bar bar)
    {
        this.Bar = bar;
    }

    public Foo(Baz bar)
    {
        this.Baz = bar;
    } 
 
    public Bar Bar { get; }
    public Baz Baz { get; }
}

// the same wiring as before
var foo = serviceProvider.GetService<Foo>();

This still throws the same error.

Expected behavior

If [ActivatorUtilitiesConstructor] is used the constructor should be prioritized above all other constructors if all its dependencies are satisfied, regardless of the order of the constructors in the source code.

Actual behavior

I have digged in the source code and I think the reason is a mistake in ActivatorUtilities class.
The following snippet of code (https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs#L95)

if (isPreferred)
{
    if (seenPreferred)
    {
        ThrowMultipleCtorsMarkedWithAttributeException();
    }

    if (length == -1)
    {
        ThrowMarkedCtorDoesNotTakeAllProvidedArguments();
    }
}

if (isPreferred || bestLength < length)
{
    bestLength = length;
    bestMatcher = matcher;
    multipleBestLengthFound = false;
}
else if (bestLength == length)
{
    multipleBestLengthFound = true;
}

isPreferred is properly set to true based on the existance of the attribute on the first constructor. But when the second constructor (public Foo(Baz baz)) is analyzed the condition in line 108 (if (isPreferred || bestLength < length)) is false, but the else statement in line 114 (else if (bestLength == length)) is evaluated to true and therefore the constructor is also considered to be the best candidate.

If the constructors appeared in an opposite order the condition in line 108 would be true and the code would have worked properly.

Regression?

No response

Known Workarounds

Add the constructor decorated with ActivatorUtilitiesConstructorAttribute after all other constructors in your class.

Configuration

No response

Other information

No response

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Selecting the preferred constructor by ActivatorUtilitiesConstructor attribute works differently depending on the order of constructors.

Reproduction Steps

Let's consider the following class:

public class Foo 
{
    public Foo(Bar bar)
    {
        this.Bar = bar;
    }

    public Foo(Baz bar)
    {
        this.Baz = bar;
    } 
 
    public Bar Bar { get; }
    public Baz Baz { get; }
}

public class Bar {}
public class Baz {}

Let's assume that I want to instantiate this class using the Dependency Injection container:

var serviceCollection = new ServiceCollection();
serviceCollection.AddTransient<Foo>();
serviceCollection.AddTransient<Bar>();
serviceCollection.AddTransient<Baz>();

var serviceProvider = serviceCollection.BuildServiceProvider();

var foo = serviceProvider.GetService<Foo>();

This obviously fails because ActivatorUtilities class finds two matching constructor.
Fortunately there is a [ActivatorUtilitiesConstructorAttribute](https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilitiesConstructorAttribute.cs) so let's adjust the class Foo:

public class Foo 
{
    [ActivatorUtilitiesConstructor]
    public Foo(Bar bar)
    {
        this.Bar = bar;
    }

    public Foo(Baz bar)
    {
        this.Baz = bar;
    } 
 
    public Bar Bar { get; }
    public Baz Baz { get; }
}

// the same wiring as before
var foo = serviceProvider.GetService<Foo>();

This still throws the same error.

Expected behavior

If [ActivatorUtilitiesConstructor] is used the constructor should be prioritized above all other constructors if all its dependencies are satisfied, regardless of the order of the constructors in the source code.

Actual behavior

I have digged in the source code and I think the reason is a mistake in ActivatorUtilities class.
The following snippet of code (https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs#L95)

if (isPreferred)
{
    if (seenPreferred)
    {
        ThrowMultipleCtorsMarkedWithAttributeException();
    }

    if (length == -1)
    {
        ThrowMarkedCtorDoesNotTakeAllProvidedArguments();
    }
}

if (isPreferred || bestLength < length)
{
    bestLength = length;
    bestMatcher = matcher;
    multipleBestLengthFound = false;
}
else if (bestLength == length)
{
    multipleBestLengthFound = true;
}

isPreferred is properly set to true based on the existance of the attribute on the first constructor. But when the second constructor (public Foo(Baz baz)) is analyzed the condition in line 108 (if (isPreferred || bestLength < length)) is false, but the else statement in line 114 (else if (bestLength == length)) is evaluated to true and therefore the constructor is also considered to be the best candidate.

If the constructors appeared in an opposite order the condition in line 108 would be true and the code would have worked properly.

Regression?

No response

Known Workarounds

Add the constructor decorated with ActivatorUtilitiesConstructorAttribute after all other constructors in your class.

Configuration

No response

Other information

No response

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

[steveharter]

Verified repro on v7 - v9P1.

[ktyl]

Thanks @steveharter for verifying. Is there a chance that it can get fixed in the future releases of v8 as well? On the other side I can imagine that as this would change the behavior of the runtime it could be considered to be a breaking change in some cases...

[steveharter]

I looked at this in greater detail: Calling GetService() does not check for [ActivatorUtilitiesConstructor]. It goes through code on CreateConstructorCallSite().

Only ActivatorUtilities.CreateInstance() checks for [ActivatorUtilitiesConstructor].

However, changing your repro to use

var foo = ActivatorUtilities.CreateInstance<Foo>(serviceProvider);

instead of

var foo = serviceProvider.GetService<Foo>();

still results in the same issue - the ordering of the constructors affects the check for [ActivatorUtilitiesConstructor] which it should not.

[steveharter]

I assume the semantics of ActivatorUtilities.CreateInstance() should be:

• We always pick the constructor with the most matched parameters first.
• If there is more than one such constructor (with the most matched parameters), we select the one with [ActivatorUtilitiesConstructor] (if there is one), otherwise we throw.

@tarekgh does this sound correct? I may push a PR soon that does this.

[tarekgh]

@steveharter

Mostly correct. The following comment describing it:

https://source.dot.net/#Microsoft.Extensions.DependencyInjection.Abstractions/ActivatorUtilities.cs,75

The only difference is, when seeing a constructor with [ActivatorUtilitiesConstructor], this will be selected even if we encountered a constructor with longer parameters. But after encountering [ActivatorUtilitiesConstructor] and then getting a constructor with longer parameters, the one with the longer parameters will be selected. I am not sure if the reflection always enumerate constructors according to the parameters count or this is not guaranteed? If reflection guarantee that, then what you wrote is accurate. if not, then it is not.

CC @eerhardt

[steveharter]

I am not sure if the reflection always enumerate constructors according to the parameters count or this is not guaranteed? I

Reflection will return the constructors in the order they are defined.

However it looks like ctor lookup from GetService() does order the constructors.

But after encountering [ActivatorUtilitiesConstructor] and then getting a constructor with longer parameters, the one with the longer parameters will be selected.

So reflection ordering does matter then. That doesn't seem like a good thing IMO. In either case, I will still put up a PR to address the case where ordering with the same number of parameters doesn't always respect [ActivatorUtilitiesConstructor].

Aside, ideally, it seems like provider.GetService() should use ActivatorUtilities.CreateInstance(provider) when it is time to find and call the constructor for a service. That would also support [ActivatorUtilitiesConstructor] for both cases; today they have different semantics.