Type activator should be order agnostic #67493
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| // 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 Xunit; | ||
|
|
||
| namespace Microsoft.Extensions.DependencyInjection.Tests | ||
| { | ||
| public class ActivatorUtilitiesTests | ||
| { | ||
| [Fact] | ||
|
There was a problem hiding this comment. Thanks for adding this test. Can you also add one that ensures the default constructor is picked in this scenario? 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 ShouldUseTheLongestAvailableConstructor() | ||
| { | ||
| var services = new ServiceCollection(); | ||
| services.AddScoped<B>(); | ||
| 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.B); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ShouldUseTheLongestAvailableConstructorOnlyIfConstructorsHaveTheSamePriority() | ||
| { | ||
| var services = new ServiceCollection(); | ||
| services.AddScoped<B>(); | ||
| services.AddScoped<S>(); | ||
| using var provider = services.BuildServiceProvider(); | ||
| var a = new A(); | ||
| var c = new C(); | ||
|
|
||
| var instance = ActivatorUtilities.CreateInstance<Creatable>(provider, a, c); | ||
|
|
||
| Assert.Null(instance.B); | ||
|
There was a problem hiding this comment. Why should There was a problem hiding this comment. As I understand from dotnet/aspnetcore#2915, the longest available constructor should only be used if competing constructors Creatable class has two constructors
Let's look at the following 3 examples
According to the algorithm that was invented and used now and which I took as a basis, the first ctor is given score 1, the second one is given score 2 (2 given arguments match sequentially). As result the second constructor will be picked up (b is null)
The first ctor is given score 1, the second ctor is given score 1. We fall into a situation where we have competing constructors. In this case, the rule about the longest available constructor comes into play.
There was a problem hiding this comment. In the discussion in #46132, it is asking for an ambiguous exception to be thrown to be thrown in this case. Which seems like the right thing IMO. If there are multiple ctors that we can't really pick between, it is better to throw and say "use the See also all the discussion on #46132 for all the scenarios, and the intended behaviors. There was a problem hiding this comment. @mapogolions - any thoughts on this? There was a problem hiding this comment. @eerhardt I don't see a way to satisfy all the mentioned requirements (especially ambiguity detection). Feel free to close this as a dead end. There was a problem hiding this comment. What about using the new (in 6.0) If the |
||
| } | ||
|
|
||
| [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); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ShouldTryToUseAllAvailableConstructorsBeforeThrowingActivationException() | ||
| { // https://github.com/dotnet/runtime/issues/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.Same(a, instance.A); | ||
| Assert.Same(c, instance.C); | ||
| Assert.NotNull(instance.S); | ||
| Assert.Null(instance.B); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(typeof(IClassWithAttribute.FirstConstructorWithAttribute))] | ||
| [InlineData(typeof(IClassWithAttribute.LastConstructorWithAttribute))] | ||
| public void ConstructorWithAttributeShouldHaveTheHighestPriorityNoMatterDefinitionOrder(Type instanceType) | ||
| { // https://github.com/dotnet/runtime/issues/42339 | ||
| var services = new ServiceCollection(); | ||
| var a = new A(); | ||
| services.AddSingleton(a); | ||
| using var provider = services.BuildServiceProvider(); | ||
|
|
||
| var instance = (IClassWithAttribute)ActivatorUtilities | ||
| .CreateInstance(provider, instanceType, new B(), new C()); | ||
|
|
||
| Assert.Same(a, instance.A); | ||
| } | ||
| } | ||
|
|
||
| internal class A { } | ||
| internal class B { } | ||
| internal class C { } | ||
| internal class S { } | ||
|
|
||
| internal class Creatable | ||
| { | ||
| public A A { get; } | ||
| public B B { get; } | ||
| 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 interface IClassWithAttribute | ||
| { | ||
| public A A { get; } | ||
| public B B { get; } | ||
| public C C { get; } | ||
|
|
||
| public class FirstConstructorWithAttribute : IClassWithAttribute | ||
| { | ||
| public A A { get; } | ||
| public B B { get; } | ||
| public C C { get; } | ||
|
|
||
| [ActivatorUtilitiesConstructor] | ||
| public FirstConstructorWithAttribute(A a, B b, C c) | ||
| { | ||
| A = a; | ||
| B = b; | ||
| C = c; | ||
| } | ||
|
|
||
| public FirstConstructorWithAttribute(B b, C c) : this(null, b, c) { } | ||
| } | ||
|
|
||
| public class LastConstructorWithAttribute : IClassWithAttribute | ||
| { | ||
| public A A { get; } | ||
| public B B { get; } | ||
| public C C { get; } | ||
|
|
||
| public LastConstructorWithAttribute(B b, C c) : this(null, b, c) { } | ||
|
|
||
|
|
||
| [ActivatorUtilitiesConstructor] | ||
| public LastConstructorWithAttribute(A a, B b, C c) | ||
| { | ||
| A = a; | ||
| B = b; | ||
| C = c; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal class DefaultConstructorFirst | ||
| { | ||
| public A A { get; } | ||
| public B B { get; } | ||
|
|
||
| public DefaultConstructorFirst() { } | ||
|
|
||
| public DefaultConstructorFirst(A a) | ||
| { | ||
| A = a; | ||
| } | ||
|
|
||
| public DefaultConstructorFirst(A a, B b) | ||
| { | ||
| A = a; | ||
| B = b; | ||
| } | ||
| } | ||
|
|
||
| internal class DefaultConstructorLast | ||
| { | ||
| public A A { get; } | ||
| public B B { get; } | ||
|
|
||
| public DefaultConstructorLast(A a, B b) | ||
| { | ||
| A = a; | ||
| B = b; | ||
| } | ||
|
|
||
| public DefaultConstructorLast(A a) | ||
| { | ||
| A = a; | ||
| } | ||
|
|
||
| public DefaultConstructorLast() { } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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:
Maybe if we do (2), then special-casing (1) becomes unnecessary.