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

Type activator should be order agnostic #67493

Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
6d88e34
Type activator depends on constructors definition order if preferred …
mapogolions Apr 2, 2022
4381820
constructor marked as preferred should be considered the first option
mapogolions Apr 2, 2022
a859ea8
prevent parameter matching in case it is known that it will fail befo…
mapogolions Apr 2, 2022
6f74f65
break loop if a perfect constructor is found
mapogolions Apr 2, 2022
ed5655f
add empty line
mapogolions Apr 2, 2022
2491f50
replace the wasteful array construction operation
mapogolions Apr 3, 2022
5dab06b
rename local variable
mapogolions Apr 3, 2022
9cdbf31
fix indent, rename unit test
mapogolions Apr 3, 2022
d9c5458
remove nullable operator
mapogolions Apr 3, 2022
d646ee1
reproduce issue #46132
mapogolions Apr 3, 2022
92f188b
fix issue 46132
mapogolions Apr 3, 2022
1012cf0
remove prioritized constructor matcher
mapogolions Apr 3, 2022
fb7264a
specify issue
mapogolions Apr 3, 2022
9beb319
refactoring
mapogolions Apr 3, 2022
d93c558
fix sorting
mapogolions Apr 4, 2022
a935ddc
add additional checks
mapogolions Apr 4, 2022
6d8fdc2
chooses default constructor no matter order
mapogolions Apr 29, 2022
fe1c38e
type activator uses longest available constructor when multiple const…
mapogolions Apr 29, 2022
7e4a363
refactoring
mapogolions Apr 29, 2022
7aabe2c
update method signature
mapogolions Apr 29, 2022
9927510
should use the longest available constructor among constructors with …
mapogolions Apr 29, 2022
68ff999
Update src/libraries/Microsoft.Extensions.DependencyInjection/tests/D…
mapogolions Apr 29, 2022
175c637
Update src/libraries/Microsoft.Extensions.DependencyInjection/tests/D…
mapogolions Apr 29, 2022
e03f785
Update src/libraries/Microsoft.Extensions.DependencyInjection.Abstrac…
mapogolions Apr 29, 2022
35f0d21
remove dead code
mapogolions Apr 29, 2022
d9052f1
fix typo
mapogolions Apr 29, 2022
9ca1980
Merge branch 'main' into type-activator-should-be-order-agnostic
mapogolions Apr 29, 2022
219e21b
rename test cases, remove useless code
mapogolions Apr 30, 2022
686d660
remove assert
mapogolions Apr 30, 2022
3facdfe
add extra test for the longest available constructor rule
mapogolions Apr 30, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq.Expressions;
Expand Down Expand Up @@ -31,49 +32,58 @@ public static object CreateInstance(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type instanceType,
params object[] parameters)
{
int bestLength = -1;
bool seenPreferred = false;
string? message = $"A suitable constructor for type '{instanceType}' could not be located. Ensure the type is concrete and all parameters of a public constructor are either registered as services or passed as arguments. Also ensure no extraneous arguments are provided.";
if (instanceType.IsAbstract)
{
throw new InvalidOperationException(message);
}

ConstructorMatcher bestMatcher = default;
ConstructorInfo[] constructors = instanceType.GetConstructors();
if (constructors.Length == 0)
{
throw new InvalidOperationException(message);
}

if (!instanceType.IsAbstract)
ConstructorInfo? pereferredConstructor = null;
mapogolions marked this conversation as resolved.
Show resolved Hide resolved
foreach (ConstructorInfo constructor in constructors)
{
foreach (ConstructorInfo? constructor in instanceType.GetConstructors())
if (constructor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing code passes false in for inherit. The new code uses a different overload that passes true for inherit. It's possible that this doesn't really matter for constructors, but I'd prefer to limit the changes to only what is necessary to fix the issue.

Suggested change
if (constructor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute)))
if (constructor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute), false))

{
var matcher = new ConstructorMatcher(constructor);
bool isPreferred = constructor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute), false);
int length = matcher.Match(parameters);

if (isPreferred)
if (pereferredConstructor is not null)
{
if (seenPreferred)
{
ThrowMultipleCtorsMarkedWithAttributeException();
}

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

if (isPreferred || bestLength < length)
{
bestLength = length;
bestMatcher = matcher;
ThrowMultipleCtorsMarkedWithAttributeException();
}

seenPreferred |= isPreferred;
pereferredConstructor = constructor;
}
}

if (bestLength == -1)
if (pereferredConstructor is not null)
{
string? message = $"A suitable constructor for type '{instanceType}' could not be located. Ensure the type is concrete and all parameters of a public constructor are either registered as services or passed as arguments. Also ensure no extraneous arguments are provided.";
throw new InvalidOperationException(message);
var matcher = new ConstructorMatcher(pereferredConstructor);
if (matcher.Match(parameters) == -1)
{
ThrowMarkedCtorDoesNotTakeAllProvidedArguments();
}
return matcher.CreateInstance(provider, throwIfFailed: true)!;
}

return bestMatcher.CreateInstance(provider);
var matchers = new ConstructorMatcher[constructors.Length];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about 2 optimizations here:

  1. When there is only a single constructor, just use it.
  2. When there are more than one, is there a way to not allocate an array here? Possibly we could stackalloc up to a reasonable count? Maybe 5 or 10? If the Type has more than that, then allocating an array seems OK since it won't be that common.

Maybe if we do (2), then special-casing (1) becomes unnecessary.

for (int i = 0; i < constructors.Length; i++)
{
ConstructorInfo constructor = constructors[i];
var matcher = new ConstructorMatcher(constructor);
_ = matcher.Match(parameters);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well make the Match method return void since no one is consuming the return value anymore.

matchers[i] = matcher;
}
Array.Sort(matchers, (a, b) => b.ApplyExectLength - a.ApplyExectLength);
object? instance = null;
for (int i = 0; i < matchers.Length; i++)
{
ConstructorMatcher matcher = matchers[i];
if (matcher.ApplyExectLength == -1) break;
bool lastChance = i == matchers.Length - 1;
instance = matcher.CreateInstance(provider, throwIfFailed: lastChance);
if (instance is not null) return instance;
}
throw new InvalidOperationException(message);
}

/// <summary>
Expand Down Expand Up @@ -336,10 +346,16 @@ public ConstructorMatcher(ConstructorInfo constructor)
_parameterValues = new object?[_parameters.Length];
}

public int ApplyExectLength { get; private set; } = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public int ApplyExectLength { get; private set; } = -1;
public int ApplyExactLength { get; private set; } = -1;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe picking a better name here would help. How about

Suggested change
public int ApplyExectLength { get; private set; } = -1;
public int MatchedLength { get; private set; } = -1;


public int Match(object[] givenParameters)
{
if (givenParameters.Length > _parameters.Length)
{
return ApplyExectLength;
}
int applyIndexStart = 0;
int applyExactLength = 0;
ApplyExectLength = 0;
for (int givenIndex = 0; givenIndex != givenParameters.Length; givenIndex++)
{
Type? givenType = givenParameters[givenIndex]?.GetType();
Expand All @@ -357,21 +373,22 @@ public int Match(object[] givenParameters)
applyIndexStart++;
if (applyIndex == givenIndex)
{
applyExactLength = applyIndex;
ApplyExectLength = applyIndex;
}
}
}
}

if (givenMatched == false)
{
return -1;
ApplyExectLength = -1;
return ApplyExectLength;
}
}
return applyExactLength;
return ApplyExectLength;
}

public object CreateInstance(IServiceProvider provider)
public object? CreateInstance(IServiceProvider provider, bool throwIfFailed = false)
{
for (int index = 0; index != _parameters.Length; index++)
{
Expand All @@ -382,7 +399,11 @@ public object CreateInstance(IServiceProvider provider)
{
if (!ParameterDefaultValue.TryGetDefaultValue(_parameters[index], out object? defaultValue))
{
throw new InvalidOperationException($"Unable to resolve service for type '{_parameters[index].ParameterType}' while attempting to activate '{_constructor.DeclaringType}'.");
if (throwIfFailed)
{
throw new InvalidOperationException($"Unable to resolve service for type '{_parameters[index].ParameterType}' while attempting to activate '{_constructor.DeclaringType}'.");
}
return null;
}
else
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using Xunit;

namespace Microsoft.Extensions.DependencyInjection.Tests
{
public class ActivatorUtilitiesTests
{
[Fact]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this test. Can you also add one that ensures the default constructor is picked in this scenario?

dotnet/maui#4318 (comment)

Basically:

        [Theory]
        [InlineData(typeof(DefaultConstructorFirst)]
        [InlineData(typeof(DefaultConstructorLast)]
        public void ChoosesDefaultConstructorNoMatterOrder(Type instanceType)
        {
            var services = new ServiceCollection();
            using var provider = services.BuildServiceProvider();

            var instance = ActivatorUtilities.CreateInstance(provider, instanceType);

            Assert.NotNull(instance);
        }

public class DefaultConstructorFirst
{
    public A A { get; }
    public B B { get; }

	public DefaultConstructorFirst() {}

    public DefaultConstructorFirst(ClassA a)
    {
        A = a;
    }

    public DefaultConstructorFirst(ClassA a, ClassB b)
    {
        A = a;
        B = b;
    }
}

public class DefaultConstructorLast
{
    public A A { get; }
    public B B { get; }

    public DefaultConstructorLast(ClassA a, ClassB b)
    {
        A = a;
        B = b;
    }

    public DefaultConstructorLast(ClassA a)
    {
        A = a;
    }

	public DefaultConstructorLast() {}
}

public void ShouldFixIssue_46132()
{
var services = new ServiceCollection();
services.AddScoped<S>();
using var provider = services.BuildServiceProvider();
var a = new A();
var c = new C();

var instance = ActivatorUtilities.CreateInstance<Creatable>(
provider, c, a);

Assert.NotNull(instance);
Assert.Same(a, instance.A);
Assert.Same(c, instance.C);
Assert.NotNull(instance.S);
Assert.Null(instance.B);
}

[Theory]
[MemberData(nameof(ActivatorUtilitiesData))]
mapogolions marked this conversation as resolved.
Show resolved Hide resolved
public void ShouldFixIssue_42339(Type instanceType)
{
var data = new Dictionary<string, object>();
var serviceProvider = new FakeServiceProvider(t =>
{
if (t == typeof(FakeValidationStatus)) return FakeValidationStatus.Invalid;
throw new NotImplementedException();
});

var instance = (IFakeValidationResult)ActivatorUtilities
.CreateInstance(serviceProvider, instanceType, "description", data);

Assert.Equal(FakeValidationStatus.Invalid, instance.Status);
}

public static TheoryData<Type> ActivatorUtilitiesData
{
get => new TheoryData<Type>
{
typeof(FakeValidationResult),
typeof(FakeValidationResultOps)
};
}
}

internal class A { }
internal class B { }
internal class C { }
internal class S { }

internal class Creatable
{
public A A { get; }
public B? B { get; }
mapogolions marked this conversation as resolved.
Show resolved Hide resolved
public C C { get; }
public S S { get; }
public Creatable(A a, B b, C c, S s)
{
A = a;
B = b;
C = c;
S = s;
}

public Creatable(A a, C c, S s) : this (a, null, c, s) { }
}

internal class FakeServiceProvider : IServiceProvider
{
private readonly Func<Type, object?> _factory;

public FakeServiceProvider(Func<Type, object?> factory)
{
_factory = factory;
}

public object? GetService(Type serviceType) => _factory(serviceType);
}

internal interface IFakeValidationResult
{
FakeValidationStatus Status { get; }
string Description { get; }
IReadOnlyDictionary<string, object> Data { get; }
}

internal class FakeValidationResult : IFakeValidationResult
{
[ActivatorUtilitiesConstructor]
public FakeValidationResult(FakeValidationStatus status, string description,
IReadOnlyDictionary<string, object> data)
{
Status = status;
Description = description;
Data = data;
}

public FakeValidationResult(string description, IReadOnlyDictionary<string, object> data)
: this(FakeValidationStatus.Valid, description, data) { }

public FakeValidationStatus Status { get; }
public string Description { get; }
public IReadOnlyDictionary<string, object> Data { get; }
}

internal class FakeValidationResultOps : IFakeValidationResult
{
public FakeValidationResultOps(string description, IReadOnlyDictionary<string, object> data)
: this(FakeValidationStatus.Valid, description, data) { }

[ActivatorUtilitiesConstructor]
public FakeValidationResultOps(FakeValidationStatus status, string description,
IReadOnlyDictionary<string, object> data)
{
Status = status;
Description = description;
Data = data;
}

public FakeValidationStatus Status { get; }
public string Description { get; }
public IReadOnlyDictionary<string, object> Data { get; }
}

internal enum FakeValidationStatus
{
Valid,
Invalid
}
}