simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 153 forks source link

Collisions of open-generic implementations not caught by Verify #876

Closed jtheisen closed 3 years ago

jtheisen commented 3 years ago

In the following code there's a registration error that doesn't get caught at Verify() (and can't be, really):

void Main()
{
    var container = new Container();

    container.Register(typeof(IModelConstructor<>), typeof(ModelConstructor1<>), Lifestyle.Singleton);
    container.Register(typeof(IModelConstructor<>), typeof(ModelConstructor2<>), Lifestyle.Singleton);

    container.Verify(VerificationOption.VerifyAndDiagnose);

    var creator = container.GetInstance<IModelConstructor<Foo>>();  
}

public interface IModel1 { }
public interface IModel2 { }

public interface IModelConstructor<T>
{
    T Create();
}

public class ModelConstructor1<T> : IModelConstructor<T>
    where T : class, IModel1
{
    public T Create() => throw new NotImplementedException();
}

public class ModelConstructor2<T> : IModelConstructor<T>
    where T : class, IModel2
{
    public T Create() => throw new NotImplementedException();
}

public abstract class Foo : IModel1, IModel2 // some coder didn't know what he was doing
{
}

I understand why, but I would love to get some advice how to deal with this better. It's ok that this fails, but

I would also like to know about any other kinds of problems that verification can't catch as before this issue I was naively thinking that it should really be able to catch everything. Forgive me if I missed the relevant bit in the extensive documentation.

dotnetjunkie commented 3 years ago

Verify won't catch the overlap in this case, because the generic registrations are root types, meaning: nothing depends on it. Simple Injector can't verify open-generic root types, because open-generic types can't have their expression trees built, and they can't be created. So in order to verify them, Simple Injector would have to 'guess' the generic types. This type guessing is very tricky, unreliable, and could easily cause Simple Injector to create types that you won't expect it to create, or would fail at runtime because of some checking you implemented in either the static constructor or the instance constructor, so that's a no-go.

Instead, you can solve this in the following ways:

Forgive me if I missed the relevant bit in the extensive documentation.

I don't think this explained in detail in the documentation, although point 3 of this section does make a mention of the problem at hand.

it should fail mentioning Foo as that's where the problem is.

When it fails, it will mention that the problem is with the overlap in ModelConstructor1<Foo> and ModelConstructor2<Foo>, but not particularly that Foo implements both IModel1 and IModel2. Adding the exact reason why both registrations overlap is actually something that is really hard to create, because type constraints can become really complex. Not sure I want to go there.

jtheisen commented 3 years ago

Indeed, I do get the exception at verification time if I inject IModelConstructor<Foo> into a root that gets registered itself.

As for the last bit, the error message does in fact mention IModelConstructor<Foo> after all (I must have missed that).

So that's all really just how it should be.

Thanks for the help!