Circular dependency check is too restrictive for delegates

[jnm2]

Creating a new instance this way is necessary. SomeViewModel can't call its own constructor because the lifetimes of the view models will be completely independent of each other, and each needs separate instances of transient dependencies.

(This implements "New" and "Duplicate" buttons which open new tabs, exactly the way it would be done if the new tab needed to contain a different view model.)

using StrongInject;
using System;

[Register(typeof(SomeViewModel))]
// ❌ SI0101 Error while resolving dependencies for 'SomeViewModel': 'SomeViewModel' has a circular dependency
public sealed partial class Container : IContainer<SomeViewModel>
{
}

public sealed class SomeViewModel
{
    // Would be Func<Owned<SomeViewModel>>, but this simplifies the repro
    public SomeViewModel(Func<SomeViewModel> someViewModelFactory) { }
}

Willing to implement. Also, I can't think of a way to move to StrongInject so long as this restriction is in place.

[YairHalberstadt]

I tried allowing this, but the problem is that it's not immediately obvious, even if we would allow it what code to generate. This case is relatively simple, but I can show you cases where it requires an infinite series of nested functions.

I spent a very long time on this problem, but I'm happy to give it another go. If you want to implement this feel free, but I would be very sceptical you would succeed without very detailed knowledge of Stronginject.

[YairHalberstadt]

Also, I can't think of a way to move to StrongInject so long as this restriction is in place.

You might be able to work around it like this.

using StrongInject;
using System;

delegate T MyFunc<T>();

[Register(typeof(SomeViewModel))]
public sealed partial class Container : IContainer<SomeViewModel>
{
    [Factory(Scope.SingleInstance)] public Func<SomeViewModel> GetViewModelFunc(MyFunc<SomeViewModel> myFunc) => () => myFunc();
}

public sealed class SomeViewModel
{
    public SomeViewModel(Func<SomeViewModel> someViewModelFactory) { }
}

[YairHalberstadt]

I tried allowing this, but the problem is that it's not immediately obvious, even if we would allow it what code to generate. This case is relatively simple, but I can show you cases where it requires an infinite series of nested functions.

For example:

public record A(Func<int, B> CreateB, int I)
public record B(Func<string, A> CreateA, string S)

[Register(typeof(A))]
[Register(typeof(B))]
public Container : IContainer<Func<string, int, A>>
{
}

It might seem like you'd want to generate code like this:

public Func<string, int, A> Resolve()
{
    return (s0, i0) => {
        Func<int, B> funcB = null;
        funcB = i1 => {
            Func<string, A> funcA = null;
            funcA = s2 => {
                return new A(funcB, i1);
            }
            return new B(funcA, s0);
        }
        return new A(funcB, i0);
    }
}

But that's actually incorrect. funcB is capturing the wrong string - it should capture s2, not s0.

I'm not sure whether it's possible to generate the correct code here.

[YairHalberstadt]

I would be happy to scope out a particular subset of such recursive delegates, and allow them, but I would need to be convinced that the subset is:

a) Both wide enough to be worth working on (the required changes in both detecting circular depndencies and emitting code will be significant). b) Provably correct. (i.e.. can't lead to circular code generation. Always captures the correct variables.)

[YairHalberstadt]

I think one level that would be safe to carve out is a delegate that depends on itself directly or indirectly without going through any intermediate delegates. I'm not 100% sure about this - will have to think through possible counterexamples.

[jnm2]

These are the two scenarios I can foresee wanting.

This one I know I need already:

class A
{
    public delegate Owned<A> Factory(int aId);

    public A(int aId, A.Factory aFactory) { }
}

And this one seems likely to happen if it didn't already:

class A
{
    public delegate Owned<A> Factory(int aId);

    public A(int aId, B.Factory bFactory) { }
}

class B
{
    public delegate Owned<B> Factory(int bId);

    public B(int bId, A.Factory aFactory) { }
}

If I was doing manual DI to obtain a class that had both A.Factory and B.Factory as dependencies:

A.Factory aFactory = null;
B.Factory bFactory = null;
aFactory = aId => new A(aId, bFactory);
bFactory = bId => new B(bId, aFactory);
return new C(aFactory, bFactory);

[YairHalberstadt]

I think we can do that:

We allow delegate recursion if and only if the set of delegate parameters that are in scope to the recursed object are the same as to the original object.

So resolving B is allowed here:

record A(Func<int, B> func);
record B(Func<int, A> func);

Since the inner Func<int, A> overrides the int that Func<int, B> provides with it's own, so it has exactly the same objects available for it to use as the outer Func<int, A>.

But this wouldn't be:

record A(Func<string, B> func);
record B(Func<int, A> func);

Since the inner Func<int, A> has access to the string that Func<string, B> provides, and the outer Func<int, A> doesn't, so we can't reuse the outer Func<int, A> as a dependency to Func<string, B>.

[jnm2]

Oh no, then I left out something important. Instead of int aId and int bId, often it is different types.

In all cases it would never make sense to use the delegate parameters used to obtain A to later obtain B or vice versa. All these parameters are fully specified at the point of invoking each factory delegate.

class A
{
    public delegate Owned<A> Factory(SomeEntity someEntity);

    public A(SomeEntity someEntity, B.Factory bFactory) { }
}

class B
{
    public delegate Owned<B> Factory(TransactionType transactionType, bool someOption = false);

    public B(TransactionType transactionType, bool someOption, A.Factory aFactory) { }
}

// The wiring needs to act equivalently to this:
A.Factory aFactory = null;
B.Factory bFactory = null;
aFactory = someEntity => new A(someEntity, bFactory);
bFactory = (transactionType, someOption) => new B(transactionType, someOption, aFactory);
return new C(aFactory, bFactory);

[YairHalberstadt]

The problem with that case is due to the recursive nature of what's going on, it's incredibly tricky to prove that the inner func will never use the string.

[jnm2]

Does there have to be an inner func? They both seem top-level to me and independent. It would be interesting to know what the use case would be for letting delegate parameters be used for anything other than directly passing to the constructor of the type used as the delegate return type.

[jnm2]

Let's put it this way: it would be a bad surprise to find out that the CreateB delegate parameter was used to construct A if CreateA was called:

[Register(typeof(A))]
[Register(typeof(B))]
partial class Container : IContainer<A>, IContainer<B>
{
    [Instance] private static readonly C C = new(...);
}

record A(C C, Func<C, B> CreateB);
record B(C C, Func<A> CreateA);

using var container = new Container();
container.Run((A a) => // 'a' uses the container-selected 'C' instance
{
    // 'b' uses a manually-selected 'C' instance
    var b = a.CreateB(new C(...));

    // 'b' needs to create a new instance of 'a' while letting it use the container-selected 'C' instance,
    // not B's instance of 'C'
    var newA = b.CreateA();
});

I don't think this will affect me (except indirectly through this circular dependency check), because I don't know of a use case for a delegate parameter to also be a type registered in the container. But it feels like spooky action at a distance.

[YairHalberstadt]

Why would it be a bad surprise?

I always found the fact this didn't work to be an extremely annoying feature of AutoFac

[jnm2]

In all the cases where this appears, we're using it as navigation between view models (pages) with no correlation of lifetimes or dependencies between A and B.

Having the destination page behave differently depending on how you navigate to it because of something not specified when the destination page's delegate was invoked seems bad.

If it wasn't the default, you could opt in to the behavior of virally passing C by changing Func<A> CreateA to Func<C, A> CreateA and explicitly flowing the value. Since it is the default, you can currently never opt out. (Or maybe there's a way to "opt out" by declaring a factory method for every delegate, not sure, but also not fun.)

[jnm2]

Imagining some new attribute used to configure this per container, I'm thinking that it would still be beneficial to carve something out so that you can still do at least the record A(Foo Foo, Func<Foo, A> AFactory); case even with the viral behavior active (and maybe even the mutual case). That makes me think that we can avoid the whole question for now of whether delegate parameter values should be viral.

[YairHalberstadt]

I definitely plan to carve out some percentage of this and do that.

As for why I think it's important that these things be viral.

Take a typical use case for Funcs with parameters:

public class RequestHandler(Func<Credentials, Owned<FrobEngine>> factory)
{
    public void OnRequest(Credentials creds) 
    {
        using var owned = factory(creds);
        owned.Value.Frob();
    }
}

This works absolutely fine if FrobEngine looks like this:

public class FrobEngine(Credentials creds);

but what if it's like this?

public class FrobEngine(IAuthenticationService authService);
public class AuthenticationService(Credentials creds) : IAuthenticationService;

If you're in Autofac non-viral land, everything breaks. You've now got to basically register your own delegate to create FrobEngine from a Credentials. But the whole point of an IOC container is that in general I shouldn't have to instantiate things myself.

In StrongInject, where delegate params are viral, everything just works as expected. Each delegate creates a new scope, where you've effectively registered whatever the delegate parameter is and can use it normally.

This is already used in FluentLang where ProjectLoader doesn't directly use SolutionInfo, but FileAsemblyLoader, one of it's indirect dependencies, does.

[jnm2]

Okay, that's a great demo of why it's useful to pass delegate arguments to constructors other than the exact type that is the delegate return type. And this also doesn't trigger my "spooky action" suspicion.

Now I'm considering if it's useful to let these flow through a different delegate. Imagine that FrobEngine took Func<IAuthenticationService> instead. Then, it's suddenly much easier to do without the viral values from the original delegate parameter; FrobEngine just needs a Credentials parameter and a Func<Credentials, IAuthenticationService> parameter. At least some of the time, it will already have a Credentials-like parameter.

[YairHalberstadt]

Now I'm considering if it's useful to let these flow through a different delegate. Imagine that FrobEngine took Func instead. Then, it's suddenly much easier to do without the viral values from the original delegate parameter; FrobEngine just needs a Credentials parameter and a Func<Credentials, IAuthenticationService> parameter. At least some of the time, it will already have a Credentials-like parameter.

I'm not really seeing the difference here. FrobEngine shouldn't have to know that the implementation of IAuthenticationService takes a Credentials. Theoretically, I may not control FrobEngine at all.

That said, I think it's worth considering making this an exception anyway, not because it' particularly logical, but because it opens up the ability to support recursive functions.

So here's the options as I see them:

1. Only allow recursive functions where we know it can't close over any delegate parameters. Easily achievable, but only solves a subset of cases.
2. Only allow recursive functions which happen not to close over any delegate parameters. May be extremely complex.
3. Allow all recursive functions, but when a function is recursive it will not close over delegate parameters as you might expect, even though functions usually do. Easily achievable, but may lead to surprising behavior.
4. Disallow any functions from closing over any delegate parameters ever. Easily achievable but both a breaking change, and also disallows other behavior which we want to enable. I'm not too worried about the breaking change since StrongInject is not super widely used right now and this is relatively niche. If we had to allow only one or the other, I'm not sure which is more useful.

[jnm2]

Would a modified version of 3 work, where we stop nesting delegates inside other delegates whenever possible, completely ignoring (at this stage) whether a cycle exists in the graph of all things? Or is that identical to 1?

[YairHalberstadt]

@jnm2 I'm not sure I understand?

[jnm2]

What I was trying to get at, is there a difference if we see it the other way around and say that the default is to not nest delegates, but we do so if forced to by the fact that a delegate parameter could possibly be captured? I guess that's the same as 2 actually.

[YairHalberstadt]

I think that is the same as 2, yes.

[jnm2]

Maybe if we do 1, it will be a long time before the mutually recursive delegates situation comes up.

I tried the workaround in https://github.com/YairHalberstadt/stronginject/issues/162#issuecomment-950282660 and it didn't make any difference to the circular dependency warning, except that it showed up only once per IContainer<X> implementation on the same class rather than seven times per implementation.

[jnm2]

Using the power of Func<Owned<>>, I was able to create a 69-line monstrosity that works perfectly as a workaround. This form looks like it would work automatically for mutual recursion as well.

// Workaround for https://github.com/YairHalberstadt/stronginject/issues/162
[Factory(Scope.SingleInstance)]
public static Func<SomeParameter, Owned<SomeViewModel>> GetSomeViewModelFactory(
    Func<Owned<DependencyA>> dependencyA,
    Func<Owned<DependencyB>> dependencyB,
    Func<Owned<DependencyC>> dependencyC,
    Func<Owned<DependencyD>> dependencyD,
    Func<Owned<DependencyE>> dependencyE,
    Func<Owned<DependencyF>> dependencyF,
    Func<Owned<DependencyG>> dependencyG,
    Func<Owned<DependencyH>> dependencyH,
    Func<Owned<DependencyI>> dependencyI,
    Func<Owned<DependencyJ>> dependencyJ,
    Func<Owned<DependencyK>> dependencyK)
{
    return someParameter =>
    {
        var ownedDependencyA = dependencyA.Invoke();
        var ownedDependencyB = dependencyB.Invoke();
        var ownedDependencyC = dependencyC.Invoke();
        var ownedDependencyD = dependencyD.Invoke();
        var ownedDependencyE = dependencyE.Invoke();
        var ownedDependencyF = dependencyF.Invoke();
        var ownedDependencyG = dependencyG.Invoke();
        var ownedDependencyH = dependencyH.Invoke();
        var ownedDependencyI = dependencyI.Invoke();
        var ownedDependencyJ = dependencyJ.Invoke();
        var ownedDependencyK = dependencyK.Invoke();

        var instance = new SomeViewModel(
            someParameter,
            ownedDependencyA.Value,
            ownedDependencyB.Value,
            ownedDependencyC.Value,
            GetSomeViewModelFactory(
                dependencyA,
                dependencyB,
                dependencyC,
                dependencyD,
                dependencyE,
                dependencyF,
                dependencyG,
                dependencyH,
                dependencyI,
                dependencyJ,
                dependencyK),
            ownedDependencyD.Value,
            ownedDependencyE.Value,
            ownedDependencyF.Value,
            ownedDependencyG.Value,
            ownedDependencyH.Value,
            ownedDependencyI.Value,
            ownedDependencyJ.Value,
            ownedDependencyK.Value);

        return new Owned<SomeViewModel>(instance, () =>
        {
            ownedDependencyA.Dispose();
            ownedDependencyB.Dispose();
            ownedDependencyC.Dispose();
            ownedDependencyD.Dispose();
            ownedDependencyE.Dispose();
            ownedDependencyF.Dispose();
            ownedDependencyG.Dispose();
            ownedDependencyH.Dispose();
            ownedDependencyI.Dispose();
            ownedDependencyJ.Dispose();
            ownedDependencyK.Dispose();
        });
    };
}