aspnet / DependencyInjection

[Archived] Contains common DI abstractions that ASP.NET Core and Entity Framework Core use. Project moved to https://github.com/aspnet/Extensions
Apache License 2.0
877 stars 320 forks source link

Make TService type param more permissive on Add*<TService, TImplementation>() #624

Closed Mardoxx closed 6 years ago

Mardoxx commented 6 years ago

Fixes aspnet/Home#2828

Mardoxx commented 6 years ago

Will add tests after feedback (if this is a genuine issue)

davidfowl commented 6 years ago

Is this a breaking change?

Mardoxx commented 6 years ago

Not entirely sure. Superficially I don't think so!

I wouldn't have thought -- seeing as this is making it more permissive on the constraints -- but there may be some way you could write something where it would break something that worked previously using reflection/inline IL??

Mardoxx commented 6 years ago

Okay, it's been picked up as a breaking change - but is it really?

This is a strange one as it clearly works when not passing template params.

What's different between

this invocation

    public static IServiceCollection RegisterScopedTestService<TService, TImplementation>(this IServiceCollection services)
        where TImplementation : class, TService
    {
        return services.AddScoped<TService, TImplementation>();
    }

and this one

    public void SomeFunction(IServiceCollection services)
    {
        services.AddScoped<ITestService, TestService>();
    }

given

    public interface ITestService
    {
    }

    public class TestService : ITestService
    {
    }

The signature still expects TService to be a reference type, but the latter is working. What am I missing here?

davidfowl commented 6 years ago

You just need to add where TService : class so that you match the constraints of the method you're calling.

Mardoxx commented 6 years ago

Just realised that! 😄

Now I'm wondering if the constraint is actually required. Under what circumstances could you register something which fails these constraints

TService : class
TImplementation : TService, class

but passes with these

//TService : any
TImplementation : TService, class
davidfowl commented 6 years ago

Now I'm wondering if the constraint is actually required. Under what circumstances could you register something which fails these constraints

It may not be possible but I'm not sure it matters because we wouldn't make a breaking change like this regardless.

Mardoxx commented 6 years ago

interesting. I'll post on SO, see if anyone can out of curiosity.

Very understandable! Thanks 😄

Mardoxx commented 6 years ago

So after a little reading it would seem that the current definition is more technically correct, even if it's not necessary. I suspect the choice was taken to have both constraints there as it clearly shows that TService must be a reference type!

Mardoxx commented 6 years ago

https://stackoverflow.com/a/48471755/3515174 Brilliant insight here 😄