-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Type activator should be order agnostic #67493
Conversation
…constructor is marked
…re it even starts
Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection Issue DetailsIf we have a type with some number of constructors, then the activator always finds the best constructor to activate the instance. This is independent of the order in which these constructors were defined. class ValidationResult
{
public ValidationResult(ValidationStatus status, string description,
IReadOnlyDictionary<string, object> data)
{
Status = status;
Description = description;
Data = data;
}
public ValidationResult(string description, IReadOnlyDictionary<string, object> data)
: this(ValidationStatus.Valid, description, data) { }
public ValidationStatus Status { get; }
public string Description { get; }
public IReadOnlyDictionary<string, object> Data { get; }
} var instance = ActivatorUtilities.CreateInstance<ValidationResult>(serviceProvider, "description", data); The activator will select the second constructor. (i.g. the statement But if we start using the
|
if (isPreferred) | ||
if (preferredCtors.Length == 1) | ||
{ | ||
bestMatcher = new ConstructorMatcher(preferredCtors[0]); |
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.
It is wasteful to construct an array, then only use its first element and throw away the rest. This block can be structured like this:
ConstructorInfo? constructorInfo = null;
foreach (ConstructorInfo? ctor in ctors)
{
if (ctor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute)))
{
if (constructorInfo is not null)
{
ThrowMultipleCtorsMarkedWithAttributeException();
}
constructorInfo = ctor;
}
}
if (constructorInfo is not null)
{
ConstructorMatcher bestMatcher = new(constructorInfo);
bestLength = bestMatcher.Match(parameters);
if (bestLength == -1)
{
ThrowMarkedCtorDoesNotTakeAllProvidedArguments();
}
}
This will also make using System.Linq;
unnecessary.
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.
@am11 Thanks for review. Good point
{ | ||
bestLength = length; | ||
bestMatcher = matcher; | ||
var matcher = new ConstructorMatcher(constructor); |
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.
var matcher = new ConstructorMatcher(constructor); | |
if (constructor is null) | |
{ | |
continue; | |
} | |
var matcher = new ConstructorMatcher(constructor); |
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.
@am11 What do you think if we just replace ConstructorInfo?
with the ConstructorInfo
in the foreach-loop foreach (
ConstructorInfo? ConstructorInfo constructor in constructors)
? According to documentation the GetConstructors()
call should not return collection with nullable elements. https://docs.microsoft.com/en-us/dotnet/api/system.type.getconstructors?view=net-6.0#system-type-getconstructors
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.
Yes, makes sense. Based on the usage elsewhere in the repo, null is not checked. GetConstructor (singular), however, returns nullable object,
Thanks @mapogolions. Can you also ensure the scenarios listed in #46132 are addressed here as well? Likewise, while we are fixing this bug, we should ensure the .NET Maui scenario is fixed as well, where the order of constructors looks like this: public LoginView(RandomItem item)
{
//this is constructor is invoked by code and RandomItem is not registered in DI
//therefore it should be skipped by ActivatorUtilities.CreateInstance
InitializeComponent();
}
// since there are no other valid constructors, ActivatorUtilities.CreateInstance should pick the default ctor
public LoginView()
{
InitializeComponent();
} |
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.
Thanks for getting this fix up @mapogolions. It looks like a great start.
FYI - @davidfowl @halter73 @Tratcher - in case you want to take a look and have any feedback.
{ | ||
public class ActivatorUtilitiesTests | ||
{ | ||
[Fact] |
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.
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() {}
}
@@ -336,10 +346,16 @@ public ConstructorMatcher(ConstructorInfo constructor) | |||
_parameterValues = new object?[_parameters.Length]; | |||
} | |||
|
|||
public int ApplyExectLength { get; private set; } = -1; |
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.
public int ApplyExectLength { get; private set; } = -1; | |
public int ApplyExactLength { get; private set; } = -1; |
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.
Maybe picking a better name here would help. How about
public int ApplyExectLength { get; private set; } = -1; | |
public int MatchedLength { get; private set; } = -1; |
{ | ||
foreach (ConstructorInfo? constructor in instanceType.GetConstructors()) | ||
if (constructor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute))) |
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.
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.
if (constructor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute))) | |
if (constructor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute), false)) |
} | ||
|
||
return bestMatcher.CreateInstance(provider); | ||
var matchers = new ConstructorMatcher[constructors.Length]; |
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:
- When there is only a single constructor, just use it.
- 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.
{ | ||
ConstructorInfo constructor = constructors[i]; | ||
var matcher = new ConstructorMatcher(constructor); | ||
_ = matcher.Match(parameters); |
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.
Might as well make the Match
method return void
since no one is consuming the return value anymore.
Prior related discussion: dotnet/aspnetcore#2871 |
@eerhardt |
…the same priority
...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs
Outdated
Show resolved
Hide resolved
...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs
Outdated
Show resolved
Hide resolved
|
||
var instance = ActivatorUtilities.CreateInstance<Creatable>(provider, a, c); | ||
|
||
Assert.Null(instance.B); |
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.
Why should B
be null here? There is a scoped service for B
, so shouldn't the ctor that takes a B
be picked?
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.
As I understand from dotnet/aspnetcore#2915, the longest available constructor should only be used if competing constructors
have the same priority/score (the value of the MatchedLength property reflects it)
Creatable class has two constructors
- Ctor(A a, B b, C c, S s)
- Ctor(A a, C c, S s) : this(a, null, c, s)
Let's look at the following 3 examples
ActivatorUtilities.CreateInstace(provider, new A(), new C());
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)
ActivatorUtilities.CreateInstance(provider, new A())
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.
ActivatorUtilities.CreateInstance(provider, new C(), new A())
orActivatorUtilities.CreateInstance(provider)
Same as above, except for the fact that competing constructors are given score 0.
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.
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 ActivatorUtilitiesConstructorAttribute
to disambiguate". It is really hard to define perfect behavior here when the "given arguments" and the "services available" can intermix.
See also all the discussion on #46132 for all the scenarios, and the intended behaviors.
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.
@mapogolions - any thoughts on this?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using the new (in 6.0) IServiceProviderIsService
interface to test if a Type
is available as a service in the IServiceProvider
?
Lines 8 to 19 in b098b6f
/// <summary> | |
/// Optional service used to determine if the specified type is available from the <see cref="IServiceProvider"/>. | |
/// </summary> | |
public interface IServiceProviderIsService | |
{ | |
/// <summary> | |
/// Determines if the specified service type is available from the <see cref="IServiceProvider"/>. | |
/// </summary> | |
/// <param name="serviceType">An object that specifies the type of service object to test.</param> | |
/// <returns>true if the specified service is a available, false if it is not.</returns> | |
bool IsService(Type serviceType); | |
} |
If the IServiceProvider
doesn't support this new interface, then using the algorithm proposed here?
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated
Show resolved
Hide resolved
…I.Tests/ActivatorUtilitiesTests.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
…I.Tests/ActivatorUtilitiesTests.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
…tions/src/ActivatorUtilities.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Closing as per #67493 (comment). Will either re-open or open a new PR to fix this issue. |
If we have a type with some number of constructors, then the activator always finds the best constructor to activate the instance. This is independent of the order in which these constructors were defined.
Let's look at the following class
The activator will select the second constructor. (i.g. the statement
instance.Status is ValidationStatus.Valid
will be true)But if we start using the
ActivatorUtilitiesConstructor
attribute, then the order in which constructors are defined starts to affect the final result.Please see unit tests for more details