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.

Sign up for GitHub

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

Jump to bottom

ActivatorUtilities not depending on ctor order for creating instances #75846

Merged
merged 9 commits into from
Oct 13, 2022
Merged

ActivatorUtilities not depending on ctor order for creating instances #75846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
84d9662
Fixes the bug with behaviour dependence on order of constructor defin…
maryamariyan Oct 7, 2022
a3de5c0
Apply PR feedback
maryamariyan Oct 11, 2022
fcd0f5b
Add mocked service
maryamariyan Oct 11, 2022
42152c5
mocked serviceprovider
maryamariyan Oct 11, 2022
f8d5069
Undo mocking fix NRE bug
maryamariyan Oct 11, 2022
cf36a7d
Fix NRE for both CreateInstance and CreateFactory. moving test to oth…
maryamariyan Oct 11, 2022
54b1af1
Remove unnecessary IsService check
maryamariyan Oct 11, 2022
54e17ae
Test badly formed container
maryamariyan Oct 11, 2022
5fa924c
Update tests - Apply PR feedback
maryamariyan Oct 13, 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
127 changes: 90 additions & 37 deletions ...libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,35 @@ public static object CreateInstance(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type instanceType,
params object[] parameters)
{
int bestLength = -1;
bool seenPreferred = false;
if (provider == null)
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
{
throw new ArgumentNullException(nameof(provider));
}

ConstructorMatcher bestMatcher = default;
if (instanceType.IsAbstract)
{
throw new InvalidOperationException(SR.CannotCreateAbstractClasses);
}

if (!instanceType.IsAbstract)
IServiceProviderIsService? serviceProviderIsService = provider.GetService<IServiceProviderIsService>();
halter73 marked this conversation as resolved.
Show resolved Hide resolved
// if container supports using IServiceProviderIsService, we try to find the longest ctor that
// (a) matches all parameters given to CreateInstance
// (b) matches the rest of ctor arguments as either a parameter with a default value or as a service registered
// if no such match is found we fallback to the same logic used by CreateFactory which would only allow creating an
// instance if all parameters given to CreateInstance only match with a single ctor
if (serviceProviderIsService != null)
{
int bestLength = -1;
bool seenPreferred = false;

ConstructorMatcher bestMatcher = default;
bool multipleBestLengthFound = false;

foreach (ConstructorInfo? constructor in instanceType.GetConstructors())
{
var matcher = new ConstructorMatcher(constructor);
bool isPreferred = constructor.IsDefined(typeof(ActivatorUtilitiesConstructorAttribute), false);
int length = matcher.Match(parameters);
int length = matcher.Match(parameters, serviceProviderIsService);
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved

if (isPreferred)
{
Expand All @@ -61,19 +78,37 @@ public static object CreateInstance(
{
bestLength = length;
bestMatcher = matcher;
multipleBestLengthFound = false;
}
else if (bestLength == length)
{
multipleBestLengthFound = true;
}

seenPreferred |= isPreferred;
}

if (bestLength != -1)
{
if (multipleBestLengthFound)
{
throw new InvalidOperationException(SR.Format(SR.MultipleCtorsFoundWithBestLength, instanceType, bestLength));
}

return bestMatcher.CreateInstance(provider);
}
}

if (bestLength == -1)
Type?[] argumentTypes = new Type[parameters.Length];
for (int i = 0; i < argumentTypes.Length; i++)
{
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);
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
argumentTypes[i] = parameters[i]?.GetType();
}

return bestMatcher.CreateInstance(provider);
FindApplicableConstructor(instanceType, argumentTypes, out ConstructorInfo constructorInfo, out int?[] parameterMap);
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
var constructorMatcher = new ConstructorMatcher(constructorInfo);
constructorMatcher.MapParameters(parameterMap, parameters);
return constructorMatcher.CreateInstance(provider);
}

/// <summary>
Expand All @@ -92,7 +127,7 @@ public static ObjectFactory CreateFactory(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type instanceType,
Type[] argumentTypes)
{
FindApplicableConstructor(instanceType, argumentTypes, out ConstructorInfo? constructor, out int?[]? parameterMap);
FindApplicableConstructor(instanceType, argumentTypes, out ConstructorInfo constructor, out int?[] parameterMap);

ParameterExpression? provider = Expression.Parameter(typeof(IServiceProvider), "provider");
ParameterExpression? argumentArray = Expression.Parameter(typeof(object[]), "argumentArray");
Expand Down Expand Up @@ -152,8 +187,7 @@ private static MethodInfo GetMethodInfo<T>(Expression<T> expr)
object? service = sp.GetService(type);
if (service == null && !isDefaultParameterRequired)
{
string? message = $"Unable to resolve service for type '{type}' while attempting to activate '{requiredBy}'.";
throw new InvalidOperationException(message);
throw new InvalidOperationException(SR.Format(SR.UnableToResolveService, type, requiredBy));
}
return service;
}
Expand Down Expand Up @@ -202,7 +236,7 @@ private static Expression BuildFactoryExpression(

private static void FindApplicableConstructor(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type instanceType,
Type[] argumentTypes,
Type?[] argumentTypes,
out ConstructorInfo matchingConstructor,
out int?[] matchingParameterMap)
{
Expand All @@ -212,8 +246,7 @@ private static void FindApplicableConstructor(
if (!TryFindPreferredConstructor(instanceType, argumentTypes, ref constructorInfo, ref parameterMap) &&
!TryFindMatchingConstructor(instanceType, argumentTypes, ref constructorInfo, ref parameterMap))
{
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);
throw new InvalidOperationException(SR.Format(SR.CtorNotLocated, instanceType));
}

matchingConstructor = constructorInfo;
Expand All @@ -223,7 +256,7 @@ private static void FindApplicableConstructor(
// Tries to find constructor based on provided argument types
private static bool TryFindMatchingConstructor(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type instanceType,
Type[] argumentTypes,
Type?[] argumentTypes,
[NotNullWhen(true)] ref ConstructorInfo? matchingConstructor,
[NotNullWhen(true)] ref int?[]? parameterMap)
{
Expand All @@ -233,7 +266,7 @@ private static bool TryFindMatchingConstructor(
{
if (matchingConstructor != null)
{
throw new InvalidOperationException($"Multiple constructors accepting all given argument types have been found in type '{instanceType}'. There should only be one applicable constructor.");
throw new InvalidOperationException(SR.Format(SR.MultipleCtorsFound, instanceType));
}

matchingConstructor = constructor;
Expand All @@ -253,7 +286,7 @@ private static bool TryFindMatchingConstructor(
// Tries to find constructor marked with ActivatorUtilitiesConstructorAttribute
private static bool TryFindPreferredConstructor(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type instanceType,
Type[] argumentTypes,
Type?[] argumentTypes,
[NotNullWhen(true)] ref ConstructorInfo? matchingConstructor,
[NotNullWhen(true)] ref int?[]? parameterMap)
{
Expand Down Expand Up @@ -289,7 +322,7 @@ private static bool TryFindPreferredConstructor(

// Creates an injective parameterMap from givenParameterTypes to assignable constructorParameters.
// Returns true if each given parameter type is assignable to a unique; otherwise, false.
private static bool TryCreateParameterMap(ParameterInfo[] constructorParameters, Type[] argumentTypes, out int?[] parameterMap)
private static bool TryCreateParameterMap(ParameterInfo[] constructorParameters, Type?[] argumentTypes, out int?[] parameterMap)
{
parameterMap = new int?[constructorParameters.Length];

Expand Down Expand Up @@ -336,39 +369,48 @@ public ConstructorMatcher(ConstructorInfo constructor)
_parameterValues = new object?[_parameters.Length];
}

public int Match(object[] givenParameters)
public int Match(object[] givenParameters, IServiceProviderIsService serviceProviderIsService)
{
int applyIndexStart = 0;
int applyExactLength = 0;
for (int givenIndex = 0; givenIndex != givenParameters.Length; givenIndex++)
for (int givenIndex = 0; givenIndex < givenParameters.Length; givenIndex++)
{
Type? givenType = givenParameters[givenIndex]?.GetType();
bool givenMatched = false;

for (int applyIndex = applyIndexStart; givenMatched == false && applyIndex != _parameters.Length; ++applyIndex)
for (int applyIndex = 0; applyIndex < _parameters.Length; applyIndex++)
{
if (_parameterValues[applyIndex] == null &&
_parameters[applyIndex].ParameterType.IsAssignableFrom(givenType))
{
givenMatched = true;
_parameterValues[applyIndex] = givenParameters[givenIndex];
if (applyIndexStart == applyIndex)
{
applyIndexStart++;
if (applyIndex == givenIndex)
{
applyExactLength = applyIndex;
}
}
break;
}
}

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

// confirms the rest of ctor arguments match either as a parameter with a default value or as a service registered
for (int i = 0; i < _parameters.Length; i++)
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
{
if (_parameterValues[i] == null &&
!serviceProviderIsService.IsService(_parameters[i].ParameterType))
{
if (ParameterDefaultValue.TryGetDefaultValue(_parameters[i], out object? defaultValue))
{
_parameterValues[i] = defaultValue;
}
else
{
return -1;
}
}
}

return _parameters.Length;
}

public object CreateInstance(IServiceProvider provider)
Expand All @@ -382,7 +424,7 @@ 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}'.");
throw new InvalidOperationException(SR.Format(SR.UnableToResolveService, _parameters[index].ParameterType, _constructor.DeclaringType));
}
else
{
Expand Down Expand Up @@ -411,16 +453,27 @@ public object CreateInstance(IServiceProvider provider)
return _constructor.Invoke(BindingFlags.DoNotWrapExceptions, binder: null, parameters: _parameterValues, culture: null);
#endif
}

public void MapParameters(int?[] parameterMap, object[] givenParameters)
{
for (int i = 0; i < _parameters.Length; i++)
{
if (parameterMap[i] != null)
{
_parameterValues[i] = givenParameters[(int)parameterMap[i]!];
}
}
}
}

private static void ThrowMultipleCtorsMarkedWithAttributeException()
{
throw new InvalidOperationException($"Multiple constructors were marked with {nameof(ActivatorUtilitiesConstructorAttribute)}.");
throw new InvalidOperationException(SR.Format(SR.MultipleCtorsMarkedWithAttribute, nameof(ActivatorUtilitiesConstructorAttribute)));
}

private static void ThrowMarkedCtorDoesNotTakeAllProvidedArguments()
{
throw new InvalidOperationException($"Constructor marked with {nameof(ActivatorUtilitiesConstructorAttribute)} does not accept all given argument types.");
throw new InvalidOperationException(SR.Format(SR.MarkedCtorMissingArgumentTypes, nameof(ActivatorUtilitiesConstructorAttribute)));
}
}
}
27 changes: 27 additions & 0 deletions ...ibraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,4 +140,31 @@
<value>Implementation type cannot be '{0}' because it is indistinguishable from other services registered for '{1}'.</value>
<comment>{0} = implementation type, {1} = service type</comment>
</data>
<data name="MultipleCtorsMarkedWithAttribute" xml:space="preserve">
<value>Multiple constructors were marked with {0}.</value>
<comment>{0} = attribute used with ActivatorUtilities</comment>
</data>
<data name="MarkedCtorMissingArgumentTypes" xml:space="preserve">
<value>Constructor marked with {0} does not accept all given argument types.</value>
<comment>{0} = attribute used with ActivatorUtilities</comment>
</data>
<data name="CannotCreateAbstractClasses" xml:space="preserve">
<value>Instances of abstract classes cannot be created.</value>
</data>
<data name="MultipleCtorsFoundWithBestLength" xml:space="preserve">
<value>Multiple constructors for type '{0}' were found with length {1}.</value>
<comment>{0} = instance type, {1} = best length</comment>
</data>
<data name="UnableToResolveService" xml:space="preserve">
<value>Unable to resolve service for type '{0}' while attempting to activate '{1}'.</value>
<comment>{0} = service type, {1} = required by</comment>
</data>
<data name="CtorNotLocated" xml:space="preserve">
<value>A suitable constructor for type '{0}' 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.</value>
<comment>{0} = instance type</comment>
</data>
<data name="MultipleCtorsFound" xml:space="preserve">
<value>Multiple constructors accepting all given argument types have been found in type '{0}'. There should only be one applicable constructor.</value>
<comment>{0} = instance type</comment>
</data>
</root>
Loading