Unify `ActivatorUtilities` methods `CreateInstance` and `CreateFactory` #50688

lawrence-laz · 2021-04-03T17:26:52Z

This pull request addresses issues #45119, #42339 and #46132.
For more details please look into commit messages and the discussion in #46132.

Using [ActivatorUtilitiesconstructor] attribute on a constructor should force `ActivatorUtilities.CreateInstance` to use that constructor regardless of the constructor definition order. This test shows that it is not the case in current implementaion.

Calling `ActivatorUtilities.CreateInstance` without additional arguments should prefer parameterless constructor as there are no guarantees that parameters, for which arguments were not supplied, can be resolved from the ServiceProvider.

Current test for `ActivatorUtilities.CreateInstance` checked that the first matched constructor is being used. Ctor definition order should not affect which ctor is being picked. Instead, the ctor should be picked based on how similar the provided argumetns are to the ctor's parameters. This would reuls in more predictable behaviour, where the best constructor is being chosen regardless of the class member ordering.

Previously `ActivatorUtilities.CreateInstance` and `ActivatorUtilities.CreateFactory` behaved differently: former depended on ctor definition order to disambiguate ctors, while latter failed altogether. The behavior is now unified and addresses concerns raised in dotnet#45119 dotnet#42339 and dotnet#46132. Constructors are chosen based on the following factors in a given order: 1) Preferred ctor (having [ActivatorUtilitiesConstructor] attribute) 2) Ctor with the most surjective parameters based on the given arguments. In other words constructor that has the least unresolved parameters is chosen. 3) Ctor with more resolved parameters is prefered. If two ctors have all of the supplied arguments, but one of them has additional parameters with default values, then the one with more parameters is chosen.

ghost · 2021-04-03T17:26:57Z

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

This pull request addresses issues #45119, #42339 and #46132.
For more details please look intro commit messages and the discussion in #46132.

Author:	lawrence-laz
Assignees:	-
Labels:	`area-Extensions-DependencyInjection`
Milestone:	-

dnfadmin · 2021-04-03T17:27:06Z

All CLA requirements met.

xtqqczze · 2021-04-04T08:26:17Z

src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

-            }
-
-            return bestMatcher.CreateInstance(provider);
+            Type[] argumentTypes = parameters


To avoid LINQ usage, you could replace Select and ToArray with Array.ConvertAll. For least allocations, you could use a manual for loop.

Changed this to use a simple for loop. 👍

quixoticaxis · 2021-04-04T10:10:08Z

src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

@@ -209,8 +179,9 @@ private static MethodInfo GetMethodInfo<T>(Expression<T> expr)
            ConstructorInfo? constructorInfo = null;
            int?[]? parameterMap = null;

-            if (!TryFindPreferredConstructor(instanceType, argumentTypes, ref constructorInfo, ref parameterMap) &&
-                !TryFindMatchingConstructor(instanceType, argumentTypes, ref constructorInfo, ref parameterMap))
+            if (instanceType.IsAbstract ||


Could you elaborate on the IsAbstract check? AFAIU, it was not preset in the CreateFactory flow. Was it implicitly covered by expression build failure?

Yes, in case of CreateFactory and an abstract type it used to fail with the following exception:

System.InvalidOperationException: Can't compile a NewExpression with a constructor declared on an abstract class at System.Linq.Expressions.Compiler.LambdaCompiler.EmitNewExpression(Expression expr) in C:\Git\runtime\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\Compiler\LambdaCompiler.Expressions.cs:line 632 at System.Linq.Expressions.Compiler.LambdaCompiler.EmitExpression(Expression node, CompilationFlags flags) in C:\Git\runtime\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\Compiler\LambdaCompiler.Generated.cs:line 120 at System.Linq.Expressions.Compiler.LambdaCompiler.EmitLambdaBody(CompilerScope parent, Boolean inlined, CompilationFlags flags) in C:\Git\runtime\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\Compiler\LambdaCompiler.Lambda.cs:line 232 at System.Linq.Expressions.Compiler.LambdaCompiler.EmitLambdaBody() in C:\Git\runtime\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\Compiler\LambdaCompiler.Lambda.cs:line 195 at System.Linq.Expressions.Compiler.LambdaCompiler.Compile(LambdaExpression lambda) in C:\Git\runtime\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\Compiler\LambdaCompiler.cs:line 187 at System.Linq.Expressions.Expression`1.Compile() in C:\Git\runtime\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs:line 225 at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.CreateFactory(Type instanceType, Type[] argumentTypes) in C:\Git\runtime\src\libraries\Microsoft.Extensions.DependencyInjection.Abstractions\src\ActivatorUtilities.cs:line 104 at Microsoft.Extensions.DependencyInjection.Specification.DependencyInjectionSpecificationTests.CreateInstanceFromFactory(IServiceProvider provider, Type type, Object[] args) in C:\Git\runtime\src\libraries\Microsoft.Extensions.DependencyInjection.Specification.Tests\src\ActivatorUtilitiesTests.cs:line 26 at Microsoft.Extensions.DependencyInjection.Specification.DependencyInjectionSpecificationTests.<>c__DisplayClass29_0.<AAACreateInstance_WithAbstractTypeAndPublicConstructor_ThrowsCorrectException>b__0() in C:\Git\runtime\src\libraries\Microsoft.Extensions.DependencyInjection.Specification.Tests\src\ActivatorUtilitiesTests.cs:line 384 at Xunit.Assert.RecordException(Func`1 testCode) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\Record.cs:line 50

Since abstract type is not desired in neither CreateInstance nor CreateFactory I assume it is fine to throw the same exception?

I guess so, an explicit check beats an implicit check in my book.
It's up to maintainers to decide though, 'cause some software somewhere can handle the original exception with regards to the message.

Makes sense. Let's see what others say.

quixoticaxis · 2021-04-04T10:13:37Z

.../Microsoft.Extensions.DependencyInjection.Specification.Tests/src/ActivatorUtilitiesTests.cs

@@ -198,27 +198,29 @@ public void TypeActivatorRethrowsOriginalExceptionFromConstructor(CreateInstance
        }

        [Theory]
-        [InlineData(typeof(string))]
-        [InlineData(typeof(int))]
-        public void TypeActivatorCreateFactoryDoesNotAllowForAmbiguousConstructorMatches(Type paramType)


If this is no longer true, then this PR does not fix #46132. I'm not implying it should, but this PR seems to bring breaking changes to both CreateInstance and CreateFactory.

Following @tarekgh comments:

[..] we must consider the purpose of the API. it is creating an instance of object. If the user code giving two ways (i.e. two legit constructors) and then we fail that will look weird too.

The current behavior can be easily defined as, CreateInstance tries to create instance of the object by finding any legit constructor satisfy the creation using the input parameters.

This PR tries to solve the #46132 issue by choosing the most similar ctor in terms of the ctor parameters and the supplied arguments, instead of relying on ctor definition order as it was done previously. I thought this should address #46132. I specifically tried to cover it by adding tests for this case in 7479663.

Given:

public class Foo { public Foo(A a, B b, C c, S s) { ... } public Foo(A a, C c, S s) { ... } } ActivatorUtilities.CreateInstance(provider, typeof(Foo), new C(), new A());

It would always choose the Foo(A a, C c, S s) ctor regardless of definition order, as it is closer to the supplied arguments than the other ctor.

Does this makes sense?

I described the expected behavior I want to see.
Using (A, B) arguments where (B, A) parameters are expected looks magical and throws type (signature) safety out of the window, IMHO.
It all comes to what we want. And I don't want the snippet to work. I want it to throw to keep everything simple.
Originally we have magical poorly described CreateInstance and strict, but well defined CreateFactory. I want the former to behave as the later: that way I'll have fewer surprises and hard to debug errors, I'm fine with failing fast during development/deployment though. But I'm only one user.

I see your point. It could possibly be easier to just fail when there are multiple choices for given arguments.

Although, if we had a way to check if IServiceProvider has a parameter type registered (or decided to go with .GetService to check that), then I think failing would be too strict and we should rather choose the constructor with the longest list of all resolved parameters. Looking from user point of view, giving the provider parameter implies you want to resolve additional services.

Using (A, B) arguments where (B, A) parameters are expected looks magical and throws type (signature) safety out of the >window, IMHO.

Regarding this, don't you feel this is a bit too strict for this method? It makes sense for Activator.CreateInstance to behave like that, because it has all arguments explicitly provided. In this case we expect to resolve 0..* additional arguments from a service provider and there's little guarantee in the parameter ordering, ex.:

// We start with this working example public Foo(A a, B b) { ... } ActivatorUtilities.CreateInstance(provider, typeof(Foo), new A(), new B()); // After some time someone adds an additional dependency in between the parameters public Foo(A a, X x, B b) { ... } // Should the following fail, even if `X` is available in the service provider? ActivatorUtilities.CreateInstance(provider, typeof(Foo), new A(), new B());

For me it feels like given the dynamic nature of this method the parameters should be treated more like a unordered set rather than a list.

Although, if we had a way to check if IServiceProvider has a parameter type registered

Unfortunately, the lack of this feature is a major pain for DI adaptors/implementations/wrappers/anything in itself.

src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

wzchua · 2021-04-07T00:49:53Z

.../Microsoft.Extensions.DependencyInjection.Specification.Tests/src/ActivatorUtilitiesTests.cs

+            var instance = ActivatorUtilities.CreateInstance(serviceProvider, type);
+
+            // Assert
+            Assert.Equal("Parameterless", ((ClassWithParameterlessCtor)instance).CtorUsed);


If IFakeService is in the service collection, it should be using the IFakeService constructor

Thing is we only have an IServiceProvider at this point and the only way to figure out if a type is registered in DI container is to actually create an instance of that type. Do you feel we should still try check that, despite the possible side effects?

Co-authored-by: xtqqczze <[email protected]>

xtqqczze · 2021-04-07T16:59:41Z

src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

            {
-                foreach (ConstructorInfo? constructor in instanceType.GetConstructors())
+                argumentTypes = new Type[parameters.Length];
+                for (int index = 0; index < argumentTypes.Length; index++)


Suggested change

for (int index = 0; index < argumentTypes.Length; index++)

for (int index = 0; index < parameters.Length; index++)

Fix CQ issue:

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgIRgDNoYGBvGhgU0bFySBhGAArGGAwMAsgAoADtijZ8ucVJkYA2gF0Gq9fhgYYUXAEp+gvtUFOGAFQCeymIYZqA5gFczADsMd09cAG47ZwYAS3YGFTUNc0stAF50hiD/ABtchkJCY2SzCysAOgAZGCDfDAALBkyGAAZbRxjeaK6BP0Da0I8YDNdhioBRfGUMNzCRqM6YgF8epxhc3Bg1+x2Y/uCh8ObsmAB3Mc89ExTy3Gra+oaDRd6nTihE2JC4oIATGAIE6tCK/AFAgA8PigAUO83uNTqjVB33BAGo0R03rsltjBAdBvC9KjAUYsjcymlif9SRUAOLmeaKayvPGrXFOdl7JgAdmhsMJw0iPXZ7PoTFIDAAguxyt1ceKRGIJNJZAokqYtCrdN4KakrFicb15t4CSF4ayYvFEnq7s0sjl8oVira0g8kU0Wu1uQ48fyBuahSd5pNprMLdz2b0NlsfdynGajiMTkFzpcvK7Koini944IPl8fiSgVkQWDAQwoZmEY9keWEBjDdjfX78TCA0ncNTwWSSqZ9V3iwZ6YzhszLW8o10uRzBMQ+YmI7jRUA

safern · 2021-04-22T22:19:54Z

@lawrence-laz are you still actively working on this? It seems like there is a lot of discussion going on, should we close the PR and move that discussion to an issue instead?

davidfowl · 2021-04-23T05:16:29Z

@lawrence-laz are you still actively working on this? It seems like there is a lot of discussion going on, should we close the PR and move that discussion to an issue instead?

Lets instead convert this to draft while that discussion is happening.

maryamariyan · 2021-05-06T16:23:41Z

@lawrence-laz are you still actively working on this? It seems like there is a lot of discussion going on, should we close the PR and move that discussion to an issue instead?

Friendly ping @lawrence-laz.

lawrence-laz · 2021-05-09T11:07:39Z

Sorry for not responding for a while, I'm preoccupied with some work right now.

I originally suspended work on this PR due to still ongoing discussions in #46132.

This PR has unified .CreateInstance and .CreateFactory methods and implementation should proceed once we agree on how this should behave in all cases.

I think it makes sense to convert this to draft until then, so I did that.

ghost · 2021-07-03T22:01:01Z

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

lawrence-laz added 4 commits April 3, 2021 17:47

Add test for parameterless ctor dotnet#45119

d186769

Calling `ActivatorUtilities.CreateInstance` without additional arguments should prefer parameterless constructor as there are no guarantees that parameters, for which arguments were not supplied, can be resolved from the ServiceProvider.

dotnet-issue-labeler bot added the area-Extensions-DependencyInjection label Apr 3, 2021

davidfowl added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Apr 3, 2021

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 3, 2021

xtqqczze reviewed Apr 4, 2021

View reviewed changes

Change from LINQ to simple for loop

a0f1c9e

quixoticaxis reviewed Apr 4, 2021

View reviewed changes

xtqqczze reviewed Apr 4, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs Outdated Show resolved Hide resolved

wzchua reviewed Apr 7, 2021

View reviewed changes

Optimize argumentType initialization

ef32f76

Co-authored-by: xtqqczze <[email protected]>

lawrence-laz force-pushed the unify-activatorutilities-createinstance-createfactory branch from 8de4da5 to ef32f76 Compare April 7, 2021 15:26

xtqqczze reviewed Apr 7, 2021

View reviewed changes

maryamariyan added the needs author feedback label May 6, 2021

lawrence-laz marked this pull request as draft May 9, 2021 10:59

maryamariyan removed the needs author feedback label May 20, 2021

ghost closed this Jul 3, 2021

ghost locked as resolved and limited conversation to collaborators Sep 28, 2021

ericstj removed breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Sep 29, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify `ActivatorUtilities` methods `CreateInstance` and `CreateFactory` #50688

Unify `ActivatorUtilities` methods `CreateInstance` and `CreateFactory` #50688

lawrence-laz commented Apr 3, 2021 •

edited

Loading

ghost commented Apr 3, 2021

dnfadmin commented Apr 3, 2021 •

edited

Loading

xtqqczze Apr 4, 2021

lawrence-laz Apr 4, 2021

quixoticaxis Apr 4, 2021

lawrence-laz Apr 6, 2021

quixoticaxis Apr 6, 2021

lawrence-laz Apr 6, 2021

quixoticaxis Apr 4, 2021 •

edited

Loading

lawrence-laz Apr 6, 2021 •

edited

Loading

quixoticaxis Apr 6, 2021

lawrence-laz Apr 7, 2021

quixoticaxis Apr 7, 2021

wzchua Apr 7, 2021

lawrence-laz Apr 7, 2021

xtqqczze Apr 7, 2021

safern commented Apr 22, 2021

davidfowl commented Apr 23, 2021

maryamariyan commented May 6, 2021

lawrence-laz commented May 9, 2021

ghost commented Jul 3, 2021

	for (int index = 0; index < argumentTypes.Length; index++)
	for (int index = 0; index < parameters.Length; index++)

Unify ActivatorUtilities methods CreateInstance and CreateFactory #50688

Unify ActivatorUtilities methods CreateInstance and CreateFactory #50688

Conversation

lawrence-laz commented Apr 3, 2021 • edited Loading

ghost commented Apr 3, 2021

dnfadmin commented Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quixoticaxis Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

lawrence-laz Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern commented Apr 22, 2021

davidfowl commented Apr 23, 2021

maryamariyan commented May 6, 2021

lawrence-laz commented May 9, 2021

ghost commented Jul 3, 2021

Unify `ActivatorUtilities` methods `CreateInstance` and `CreateFactory` #50688

Unify `ActivatorUtilities` methods `CreateInstance` and `CreateFactory` #50688

lawrence-laz commented Apr 3, 2021 •

edited

Loading

dnfadmin commented Apr 3, 2021 •

edited

Loading

quixoticaxis Apr 4, 2021 •

edited

Loading

lawrence-laz Apr 6, 2021 •

edited

Loading