Feature Proposal - Generic Overloads for Register to make delegate registration more concise

[alistairjevans]

Problem Statement

Today I was registering some Autofac components, and realised I wanted to capture a configuration object, and pass that into my component constructor, along with it's dependencies.

So, something like this:

interface IMyDependency1 { } 
interface IMyDependency2 { }

class ConfigOptions { }

class MyComponent 
{
  public MyComponent(ConfigOptions opt, IMyDependency1 dep1, IMyDependency2 dep2) 
  {
  }
}

// Registering (in a module, but that's an implementation detail)
var myOpts = new ConfigOptions 
{
   // Populate my options from wherever
};

builder.Register(ctxt => new MyComponent(
  myOpts, 
  ctxt.Resolve<IMyDependency1>(), 
  ctxt.Resolve<IMyDependency2>());

All good, works as expected, but constructing that instance of MyComponent feels reallly verbose, since I need to call ctxt.Resolve each time. It also smacks of service location in a way, because we're accessing an autofac service locator directly:

// Ugh, noisy, more generic args, etc.
builder.Register(ctxt => new MyComponent(
  myOpts, 
  ctxt.Resolve<IMyDependency1>(), 
  ctxt.Resolve<IMyDependency2>());

Desired Solution

What I'd like to be able to do is this:

// Just declare your dependencies as arguments to the delegate, just as you would with a constructor.
builder.Register((IMyDependency1 dep1, IMyDependency2 dep2) 
  => new MyComponent(myOpts, dep1, dep2));

Personally I feel like this brings things closer to feeling like a normal factory method, plus it has the advantage that I can treat any method that accepts dependencies as a factory method, without leaking Autofac types into it.

I also have to know less Autofac 'stuff', like a component context, to get this working.

But, I'd like to retain the option to access the full component context if I need to do something a bit more complex:

builder.Register((IComponentContext ctxt, IMyDependency2 dep2) 
  => new MyComponent(ctxt.ResolveKeyed<IMyDependency1>("key"), dep2));

Implementation

The implementation is actually pretty simple; we pick a maximum number of generic parameters we want to support; let's say 10 to start, we then create a set of methods for each number of parameters we want to support. Here are the new extension methods for my example with 2 parameters:

public static IRegistrationBuilder<TComponent, SimpleActivatorData, SingleRegistrationStyle>
    Register<TParam1, TParam2, TComponent>(
        this ContainerBuilder builder,
        Func<TParam1, TParam2, TComponent> @delegate)
    where TParam1 : notnull
    where TParam2 : notnull
    where TComponent : notnull
{
    if (@delegate == null)
    {
        throw new ArgumentNullException(nameof(@delegate));
    }

    return builder.Register((c, p) => @delegate(c.Resolve<TParam1>(), c.Resolve<TParam2>()));
}

// Note, here we can include an `IComponentContext` parameter first so users can blend both types if they need to.
public static IRegistrationBuilder<TComponent, SimpleActivatorData, SingleRegistrationStyle>
    Register<TParam1, TParam2, TComponent>(
        this ContainerBuilder builder,
        Func<IComponentContext, TParam1, TParam2, TComponent> @delegate)
    where TParam1 : notnull
    where TParam2 : notnull
    where TComponent : notnull
{
    if (@delegate == null)
    {
        throw new ArgumentNullException(nameof(@delegate));
    }

    return builder.Register((c, p) => @delegate(c, c.Resolve<TParam1>(), c.Resolve<TParam2>()));
}

Granted, it would be a fair number of extra overloads, but it doesn't seem too onerous, since the methods themselves are lightweight. There are prior examples of this in the runtime (e.g. HashCode.Combine), and I've done it in a couple of private projects.

I tested the edges of this solution a little to see if there were obvious pitfalls. One of the nice things (of sorts) is that the compiler won't just 'infer' delegate argument types automatically, you need to specify them. On top of that, if you specify one, you specify all. So, this largely prevents clashes with our existing overloads:

// Existing overloads don't break, because a) non-generic methods are preferred, and b) the type isn't specified as an argument.
builder.Register(ctxt => new Component(ctxt.ResolveKeyed<IFancyDep>("val"));

// Compiler error here, types cannot be inferred, must be specified explicitly. 
// I kind of like it because it makes it mandatory to specify your dependencies.
builder.Register((dep1, dep2, dep3) => new Component(dep1, dep2, dep3));

// Compiler error here because arg1 is IComponentContext and arg2 is IEnumerable<Parameter>, because I didn't specify,
// so I got the non-generic form.
builder.Register((arg1, arg2) => new Component(arg1, arg2));

// Compiler error here, can't mix and match.
builder.Register((dep1, IMyDependency2 dep2, dep3) => new Component(dep1, dep2, dep3));

Alternatives You've (not) Considered

It's entirely possible that this has been suggested before by someone else, and dismissed because of some glaring thing I've missed.

[tillig]

Off the top of my head, would this have worked?

builder
  .RegisterType<MyComponent>()
  .WithParameter(TypedParameter.From(myOpts));

On the resolve side, you can also use the delegate factories thing:

var factory = container.Resolve<Func<MyComponent, ConfigOptions>>();
var thing = factory(opts);

Or is there perhaps some use case I'm missing here? It seems like the proposed solution, while potentially interesting, may be overly complicated compared to adding a parameter and I'm not sure if it's just because the use case in the proposal was possibly oversimplified.

[alistairjevans]

The WithParameter option would have worked, yes; I didn't use it because:

a) The component in question is activated very frequently in a perf hot-path; using delegate activation let's me side-step the bunch of extra code that I happen to know runs in the reflection activator.
b) I forgot about TypedParameter.From (related to a, parameter binding definitely results in some extra code in the reflection activator)

I will say though, ignoring what already works, that just from the perspective of someone relatively new to Autofac, which one requires you to know more about how to use Autofac to get going? It wasn't immediately obvious to me from the intellisense how best to register the parameter (the WithParameter overloads are somewhat obtuse), and I should really know better.

var config = "something";

// Feels fairly intuitive, and not super Autofac-y.
builder.Register((IMyDep1 dep) => new MyComponent(dep, config));

// Need to go and consult the WithParameter docs...
builder.RegisterType<MyComponent>()
        .WithParameter(TypedParameter.From(config));

The resolve-side factory thing doesn't really help me unless I want to inject that func into something else that consumes MyComponent.

I do get that there are existing ways to achieve this, but in some situations like having some external value, it does feel more intuitive to capture into the closure than using WithParameter, putting aside the performance concerns.

[tillig]

Totally fair to suggest some new syntax for something like this; I just didn't see it in the proposal and the "alternatives" section didn't acknowledge parameters so I figured I'd ask. I also kinda figured that the resolve side of things wouldn't help, but omitting it would have been somewhat incomplete from an answer perspective.

I don't think it's unfair to ask people to be familiar with Autofac syntax. Again, not against making things easier, but there are a lot of things (from implementing modules to middleware to even conditionally registering decorators) that aren't entirely fall-down-easy without taking some time to read the docs. It's a new domain. You don't get IntelliSense to tell you what order to set up the middleware in your ASP.NET app or how to properly set up your AutoMapper mappings, either. And having More Than One Way To Do It means multiple paths to test and maintain.

The delegate mechanism coupled with the perf aspect does seem to lend some credence to this being more than just syntax, though, and that's cool. It would be good to see if you get to keep the good perf if you have three or four parameters that end up still having to be resolved. Like, is this still cheaper?

var config = "config";
builder
  .Register(
    (IDep1 dep1, IDep2 dep2, IDep3 dep3, IDep4 dep4) =>
      new MyComponent(dep1, dep2, dep3, dep4, config));

Than this?

// UsingConstructor to remove the constructor location overhead
builder
  .RegisterType<MyComponent>()
  .UsingConstructor(typeof(IDep1), typeof(IDep2), typeof(IDep3), typeof(IDep4), typeof(string))
  .WithParameter(TypedParameter.From("config"));

Absolutely the first one is prettier, but, again, just trying to make sure the various options are considered. One solution already exists and, yeah, it's not pretty, but needing to do this sort of thing feels pretty edge-case-y so it'd just be interesting to know.

Admittedly, when I have config like this, I usually just register the instance and call it good.

builder.RegisterInstance(configObject);
builder.RegisterType<MyComponent>();

Definitely use cases that aren't covered there - different config options by context, trying to avoid keyed instances, wanting the perf of the lambda without the redundant ctx.Resolve<T> calls, that sort of thing. I can see where any perf benefit folks might get could be very quickly lost if additional logic starts showing up in those lambdas and folks will be like, "I know I put Thread.Sleep(1000) in there but you said in the docs that lambdas were faster than reflection!" That'd be nice to avoid.

Let me set up a couple of benchmarks to see what I get. The syntax proposed does look kinda nice, and if it is faster, hey, faster + prettier is a big win.

[alistairjevans]

All points understood, see what you get with those benchmarks; not sure the UseConstructor will help too much, and my god it hurts the eyes to read. Fundamentally the constructor parameter binding does the same resolve call for each parameter same as resolving directly from IComponentContext, so anything extra should be reflection.

I've done that RegisterInstance thing for config in the past as well, but I had the same type registered elsewhere for another component's config, and I didn't want to switch everything to keyed dependencies.

Also...

You don't get IntelliSense to tell you what order to set up the middleware in your ASP.NET app

Maybe it should (he says bitterly).

[tillig]

OK, legit science, looks like lambda is still faster.

Here's my benchmark so you can tell me how I messed it up because I such writing benchmarks.

I have two different setups - one that builds the container with the reflection container, one that builds it with the lambda. The benchmark itself is basically the same - just resolve the thing.

public class ParameterBenchmark
{
    private IContainer _container;

    [GlobalSetup(Target = nameof(ReflectionRegistration))]
    public void SetupReflectionContainer()
    {
        var config = new Configuration { Value = "Value" };
        var builder = new ContainerBuilder();
        builder.RegisterType<Dependency1>().As<IDependency1>();
        builder.RegisterType<Dependency2>().As<IDependency2>();
        builder.RegisterType<Dependency3>().As<IDependency3>();
        builder.RegisterType<Dependency4>().As<IDependency4>();
        builder
            .RegisterType<Component>()
            .UsingConstructor(typeof(IDependency1), typeof(IDependency2), typeof(IDependency3), typeof(IDependency4), typeof(Configuration))
            .WithParameter(TypedParameter.From(config));
        this._container = builder.Build();
    }

    [Benchmark]
    public void ReflectionRegistration()
    {
        var instance = this._container.Resolve<Component>();
        GC.KeepAlive(instance);
    }

    [GlobalSetup(Target = nameof(LambdaRegistration))]
    public void SetupLambdaContainer()
    {
        var config = new Configuration { Value = "Value" };
        var builder = new ContainerBuilder();
        builder.RegisterType<Dependency1>().As<IDependency1>();
        builder.RegisterType<Dependency2>().As<IDependency2>();
        builder.RegisterType<Dependency3>().As<IDependency3>();
        builder.RegisterType<Dependency4>().As<IDependency4>();
        builder
            .Register(ctx =>
            {
                return new Component(
                    ctx.Resolve<IDependency1>(),
                    ctx.Resolve<IDependency2>(),
                    ctx.Resolve<IDependency3>(),
                    ctx.Resolve<IDependency4>(),
                    config
                );
            });
        this._container = builder.Build();
    }

    [Benchmark]
    public void LambdaRegistration()
    {
        var instance = this._container.Resolve<Component>();
        GC.KeepAlive(instance);
    }

    private interface IDependency1 { }
    private class Dependency1 : IDependency1 { }
    private interface IDependency2 { }
    private class Dependency2 : IDependency2 { }
    private interface IDependency3 { }
    private class Dependency3 : IDependency3 { }
    private interface IDependency4 { }
    private class Dependency4 : IDependency4 { }

    private class Configuration
    {
        public string Value { get; set; }
    }

    private class Component
    {
        private readonly IDependency1 _dep1;
        private readonly IDependency2 _dep2;
        private readonly IDependency3 _dep3;
        private readonly IDependency4 _dep4;
        private readonly string configValue;

        public Component(
            IDependency1 dep1,
            IDependency2 dep2,
            IDependency3 dep3,
            IDependency4 dep4,
            Configuration config)
        {
            this._dep1 = dep1;
            this._dep2 = dep2;
            this._dep3 = dep3;
            this._dep4 = dep4;
            this.configValue = config.Value;
        }

        public void DoWork()
        {
            // So the compiler doesn't try optimizing anything out.
            Console.WriteLine($"{this._dep1.GetType()}{this._dep2.GetType()}{this._dep3.GetType()}{this._dep4.GetType()}{this.configValue.GetType()}");
        }
    }
}

Results:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
ReflectionRegistration	2.376 us	0.0471 us	0.0629 us	0.5264	-	3 KB
LambdaRegistration	1.886 us	0.0116 us	0.0108 us	0.3738	0.0019	2 KB

So, not only is it faster, it also appears to allocate less. Color me legitimately shocked. I guess let's go for it. Better syntax plus potentially keeping the perf seems like a win.

[tillig]

Side note, it makes me wonder if there's a way to further optimize the situation where there's a MatchingParametersConstructorSelector such that it might be able to generate a lambda for the specific constructor, execute through that, and do basically the lambda registration behind the scenes. Admittedly a super edge-case, but seems like it should be possible to do something there [without me actually having gone in to look at the code]. 🤔

[alistairjevans]

Nice; as you say, science! Benchmarks look good. Extra memory allocations probably come from the bound constructor objects allocated inside the ReflectionActivator (at a guess).

I think the optimisation of MatchingParametersConstructorSelector might be problematic if we need to swap a registration builder with activator of type ReflectionActivatorData with SimpleActivatorData when changing the registration type. Or perhaps the ReflectionActivator itself could pre-process the factory into a generated delegate based on a single matching constructor at container build time...worth a look, I'll take a gander when I put a PR together. 🤔

[alsami]

Just curious, what's the limit of type parameters I can pass to Register<T1, T2, T3, T4, TComponent>(...)?

Edit: just seen the PR, it's 10 as off now :)

[alistairjevans]

Yeah, I decided to just pick a number. If you want to do more dependencies than that, I think it's OK to make people use the old way of doing things.

[alsami]

I've always wondered if there is a recommendation or best-practice. I've seen for instance System.CommandLine limiting it to exactly 9.

Cool idea anyway, saves on writing loads of boilerplate code :)

[alistairjevans]

Not sure what the best practice is, if there is one. The System.HashCode.Combine method in the runtime goes up to 8, but I'm not sure if there's a reason they chose that number. Can't see any obvious reasoning for it in the API proposal.

[alistairjevans]

I now realise I should have made it 11, so we can say we turned the method all the way up to 11...

[alistairjevans]

A follow-up to adding all the generic overloads of 1 to 10 arguments; while writing some docs, I realised we don't have a version of Register that accepts 0 arguments:

builder.Register(() => new Component()); // Not allowed
builder.Register(c => new Component()); // allowed (IComponentContext).

I was using the term "a variable number of arguments", and realised that maybe that should include none? I'm not sure if there's a reason we never had such an overload?

Easy implementation:

public static IRegistrationBuilder<T, SimpleActivatorData, SingleRegistrationStyle>
    Register<T>(
        this ContainerBuilder builder,
        Func<T> @delegate)
    where T : notnull
{
    if (@delegate == null)
    {
        throw new ArgumentNullException(nameof(@delegate));
    }

    return builder.Register((c, p) => @delegate());
}

[tillig]

There's probably not a great reason it's not there, other than that folks could just ignore the context parameter and move on with life. It also might be confusing as to why it exists for new folks - "I thought you'd just builder.RegisterType<T>() for that." I know it's a perf thing, but that's because I know it's a perf thing, not that it's obvious from an API perspective. Given there are now like 20+ overloads for this, I wonder if it's worth having the zero parameter one, but I guess I don't feel terribly strongly. Sort of... "What's one more, AMIRITE?"

Though it does make me think about optimizations we could make. For example, we could detect things like:

• The object only has one constructor, convert the reflection registration into a lambda automatically. (Automatically handle the zero-param constructor.)
• A specific constructor on the object was selected, convert the reflection registration into a lambda. (This was already mentioned above, just restating.)

Basically when there's only one constructor to use in the reflection case, somehow automatically switching to a delegate behind the scenes could be a free-to-the-consumer perf boost.

[alistairjevans]

Looking at those potential optimisations, possibly intriguing.

In the reflection activator we throw a DependencyResolutionException exception with a specific "No Constructors Bindable" message if we cannot satisfy any constructor at resolve time from the set of services and parameters.

The new delegate activator methods largely just "have a go" at resolving what ever arguments are there, so throw a DependencyResolutionException at the first argument that isn't either a service or a parameter.

The choices here I think are:

• Define us changing that exception message as not breaking, and change the message you get when there's only 1 constructor.
• Define us changing the message as breaking, change the message and bump major version.
• In the case where we fast-path the single constructor, catch the inner DependencyResolutionException, and rethrow with the correct message (could not bind the constructor).

[alistairjevans]

Actually, thinking on it a bit more, there's something else to consider here.

Consider a component:

public class Component 
{
  public Component(A dep1, B dep2) 
  {
  }
}

If I resolve Component using the delegate activator, if the first parameter (A) can be satisfied by a service, but the second one (B) cannot, then A is activated, and B is not, with an exception being thrown.

If I resolve Component using the reflection activator, if the first parameter (A) can be satisfied by a service, but the second one (B) cannot, then neither service is activated, with an exception being thrown.

Would that be a breaking change if the reflection activator started switching to the former pattern for single-constructor types?

This is sort of a non-issue for single-constructor-with-no-parameter components, in which case we can optimise quite easily.

[tillig]

Eeeesh I didn't think about that activation scenario of one working but one not. Of course, that's also a gotcha with the new lambda syntax - it's going to resolve all the dependencies until it fails. Which is what a hand-written lambda would do, it just may not be obvious. Possibly something to document.

You know, registration sources are a nightmare. Without them, we could query the container for the set of concrete, registered services and always get a reliable answer back. I think registration sources are the biggest hurdle to container validation, too. Hmmm.

I've never considered exception message text as part of the contract. The exception type maybe, but the text? No. If that breaks the way you're handling exceptions, you need to figure out how to handle exceptions better. If folks want more specific details on an exception type to differentiate between two different conditions, that's two different exception types. (No, I'm not proposing we create a new exception type.)

But I like the catch-and-rethrow idea. Outer exception matches the original "we can't bind" message, inner exception explains more detail about what went wrong. Seems like the best of both worlds.

Whether resolving, say, five-out-of-six of the arguments first and then failing on the sixth is breaking... it seems like it could have, at the very least, a memory impact that folks aren't expecting. Admittedly failure to resolve a dependency like that should be sort of a catastrophic/fatal issue that needs to be fixed, so ideally it's really rare, but who knows? I've seen weirder things than a try/catch to ignore something relatively serious like that. Right up there with my favorite unit test assertion ever, Assert.True(true).

My gut says in the 90% case it's probably OK to do the resolutions and fail on one of them. If we assume that's a rare occurrence that folks will troubleshoot and fix, optimizing for the failure condition is less interesting than optimizing for overall performance.

However, it may be interesting to consider an opt-out option. Like, the ability to say, "I want to resolve this through reflection, I accept responsibility for the lower perf, keep doing it the old way, thanks." Sort of like turning off compiler optimizations. Perhaps we don't provide or expose that immediately out of the gate and wait for folks to ask for it, but whatever solution comes out, it might be good to have the possibility of an escape hatch. I also could be overthinking it.

[alistairjevans]

Closing this issue now #1325 has been merged; I've been considering the "zero arguments" delegate registration method now we have improved the performance of a reflection no-args-constructor resolve to almost the same as a delegate resolve, and don't think we need to add the Register(() => new Component()) overload, where RegisterType<Component>() is probably better.

[SzybkiDanny]

Hi. I have just upgraded from 6.2.0 to 6.4.0 and noticed an unexpected behavior while resolving new instances.
Namely: setting up registration with UsingConstructor(IConstructorSelector) has no effect if the service being registered only has a single parameterless constructor defined. Skimming through this issue, I can see that this is a side effect of some optimization effort but I think it could be considered a breaking change.
Please, consider reflecting the behavior in the docs, e.g. here: https://autofac.readthedocs.io/en/latest/register/registration.html?highlight=usingconstructor#specifying-a-constructor

[tillig]

@SzybkiDanny If the component only has one constructor and that constructor has no parameters, what's the use case for having a constructor selector? I don't have any problem updating the docs, but I need to be able to explain the scenario.

[SzybkiDanny]

We have a custom implementation of IConstructorSelector which serves the purpose of making sure that in case of a component with both: a) more than one constructor, b) at least one parameterless constructor, always a parameterized constructor is used (and we are able to populate it with required parameters), otherwise it throws an exception. (Please, don't ask me why we need it.)
We had a unit test for the IConstructorSelector implementation which checked if a proper exception is thrown also in case of components with a single parameterless constructor and that's how I noticed the change in behavior. The test case has been removed because such a component doesn't fulfil both of the mentioned conditions, so the change in behavior no longer concerns me.
Having said that, I can imagine someone using it for logging/debugging purposes because it gives you pretty good look of Autofac's inner workings, but I understand if you'll find this use case farfetched.

[tillig]

I think a note in the docs to mention that IConstructorSelector isn't used in the case of a single, parameterless constructor, is reasonable, but I don't think I'm going to note it as a breaking change. We did note it in the release notes for 6.4.0...

Optimization of reflection activation for components that only have one constructor (#1325)

...but maybe that's not clear enough. I can update the release notes to be more explicit as to what that means.

[SzybkiDanny]

To be honest, I consider Autofac's docs to be as good as it gets for OS projects so I have never had a need to look into release notes and that's why I suggested putting the info in the docs.
I appreciate you taking the time to do it.

[tillig]

Added this issue for the docs.