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

[API Proposal]: ActivatorUtilities.CreateFactory<T> with params span #101889

Closed
alrz opened this issue May 5, 2024 · 7 comments
Closed

[API Proposal]: ActivatorUtilities.CreateFactory<T> with params span #101889

alrz opened this issue May 5, 2024 · 7 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-DependencyInjection
Milestone

Comments

@alrz
Copy link
Member

alrz commented May 5, 2024

Background and motivation

The arguments passed to CreateFactory and ObjectFactory are usually of a fixed number so it may be worthwhile to consider params span for both instead of array.

API Proposal

I haven't seen an addition like this so far so I'm not sure about the naming convention.

namespace Microsoft.Extensions.DependencyInjection
{
  public delegate T ObjectFactoryWithParams<out T>(IServiceProvider serviceProvider, params ReadOnlySpan<object?> arguments); 

  public static class ActivatorUtilities
  {
    public static ObjectFactoryWithParams<T> CreateFactory2<T>(params ReadOnlySpan<Type> argumentTypes)
  }
}

API Usage

var factory = ActivatorUtilities.CreateFactory<T>(typeof(SomeType));
var result = factory(serviceProvider, new SomeType());

Alternative Designs

An alternative is to consider making the return type a generic delegate over argument types.

public static ObjectFactory<TArg1, T> CreateFactory<T, TArg1>();
public static ObjectFactory<TArg1, TArg2, T> CreateFactory<T, TArg1, TArg2>();
// etc
public delegate T ObjectFactory<in TArg1, out T>(IServiceProvider serviceProvider, TArg1? arg1);
public delegate T ObjectFactory<in TArg1, in TArg2, out T>(IServiceProvider serviceProvider, TArg1? arg1, TArg2? arg2);
// etc
var factory = ActivatorUtilities.CreateFactory<T, SomeType>();
var result = factory(serviceProvider, new SomeType());

Risks

No response

@alrz alrz added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 5, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

@alrz
Copy link
Member Author

alrz commented Jul 11, 2024

@steveharter could this be a candidate for API review alongside #101829?

@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jul 18, 2024
@ericstj ericstj added this to the 9.0.0 milestone Jul 18, 2024
@ericstj ericstj added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jul 18, 2024
@stephentoub stephentoub removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 22, 2024
@stephentoub stephentoub modified the milestones: 9.0.0, 10.0.0 Jul 22, 2024
@steveharter
Copy link
Member

@steveharter could this be a candidate for API review alongside #101829?

Yes this is a good proposal; however the window for 9.0 has been closed for such changes.

@halter73
Copy link
Member

halter73 commented Aug 6, 2024

The alternative design seems more appealing to me if we do anything here. The goal appears to be to reduce allocations, but params ReadOnlySpan<object?> arguments still forces you to potentially box the individual arguments.

We might as well go all the away and do the most optimal thing if we're going to define new CreateFactory overload(s), but I think we should probably just return a Func<in T1, out TResult> instead of an ObjectFactory<in TArg1, out T>.

@terrajobst
Copy link
Contributor

terrajobst commented Aug 6, 2024

Video

  • It seems to odd to have an API that avoids the array allocation, given how expensive the call is
  • The alternative design makes more sense to use, at least the returned delegate is strongly typed to the arguments
    • We'd probably prefer Func<...> over creating 15 new delegate types
namespace Microsoft.Extensions.DependencyInjection;

public delegate T ObjectFactoryWithParams<out T>(IServiceProvider serviceProvider, params ReadOnlySpan<object?> arguments);

public static class ActivatorUtilities
{
    public static ObjectFactoryWithParams<T> CreateFactory2<T>(params ReadOnlySpan<Type> argumentTypes);
}

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 6, 2024
@alrz
Copy link
Member Author

alrz commented Aug 6, 2024

If Func<...> is preferred, might as well take it as a type argument.

public static class ActivatorUtilities
{
    public static T CreateFactoryOf<T>() where T : Delegate;
}
var factory = ActivatorUtilities.CreateFactoryOf<Func<IServiceProvider, SomeType, T>>();
var result = factory(serviceProvider, new SomeType());

This way only one overload would be added. this will avoid array allocation and boxing, but also typechecks that the callsite matches the factory.

@alrz
Copy link
Member Author

alrz commented Jan 9, 2025

As a result of discussion here I opened up #111228 which I think is more straightforward to implement since that's how ActivatorUtilities already works internally.

@alrz alrz closed this as completed Jan 9, 2025
@steveharter steveharter closed this as not planned Won't fix, can't repro, duplicate, stale Jan 13, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-DependencyInjection
Projects
None yet
Development

No branches or pull requests

6 participants