dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.15k stars 4.71k forks source link

IServiceProvider.GetServices return value nullability #94374

Open WeihanLi opened 11 months ago

WeihanLi commented 11 months ago

Currently, the API return value type is IEnumerable<object?> while the API implementation is returning IEnumerable<object>

see

https://github.com/dotnet/runtime/blob/b4823d05c38d7eb44de0db41ba9ba6c6a2df4e46/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceProviderServiceExtensions.cs#L88-L95

so should we update the API return value type to IEnumerable<object> to align with the method return value type?

namespace Microsoft.Extensions.DependencyInjection;

public static class ServiceProviderServiceExtensions
{
-    public static IEnumerable<object?> GetServices(this IServiceProvider provider, Type serviceType);
+    public static IEnumerable<object> GetServices(this IServiceProvider provider, Type serviceType);
}
ghost commented 11 months ago

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

Issue Details
Currently, the API return value type is `IEnumerable` while the API implementation is returning `IEnumerable` see https://github.com/dotnet/runtime/blob/b4823d05c38d7eb44de0db41ba9ba6c6a2df4e46/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceProviderServiceExtensions.cs#L88-L95 so should we update the API return value type to `IEnumerable` to align with the method return value type?
Author: WeihanLi
Assignees: -
Labels: `area-Extensions-DependencyInjection`
Milestone: -
stephentoub commented 11 months ago

I don't see anything in the implementation guaranteeing the objects aren't null. Non-nullability isn't part of the runtime type identity. It'd be ok to change the cast to IEnumerable<object?>, but I don't see anything here that would allow us to change the method return type to IEnumerable<object>.

WeihanLi commented 11 months ago

Then should public static System.Collections.Generic.IEnumerable<T> GetServices<T>(this System.IServiceProvider provider) return IEnumerable<T?>?

https://github.com/dotnet/runtime/blob/b4823d05c38d7eb44de0db41ba9ba6c6a2df4e46/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs#L233

Generally, think we would not register a null for the service, but indeed, it could be injected now

var services = new ServiceCollection();
services.AddSingleton<IService>(sp => null!);
stephentoub commented 11 months ago

@halter73, is there anything preventing a null instance from being registered / returned? If not, should there be?

halter73 commented 11 months ago

As @WeihanLi pointed out, you can register a factory that returns null. I don't think it's very common, but I'm sure people have done it. I'm sure it's even less common to both register null-returning factory and also call GetServices(Type) with the corresponding service type. I've never seen it, but it is possible.

I definitely prefer IEnumerable<object> over IEnumerable<object?> for the GetServices() return type, but I understand not wanting to do that if null could ever end up there even in the rarest circumstances. I see two options for making this non-nullable without lying.

  1. We do a null check every time we ever call a factory and add notnull constraints as appropriate on TImplementation and/or TService for methods that register factories (or perhaps all the service registrations). This would be how I'd do it if I were starting from scratch, but it would definitely be the most breaking option.
  2. Filter out null from the IEnumerable<object> if it is present. This is definitely the weirder behavior, but it's less unlikely anyone would ever hit this case which makes it less likely to break people.
buyaa-n commented 11 months ago

Added api-suggestion label as it is about public API shape, feel free to mark as api-ready-for-review and/or update the mileston as needed.