Jump to conversation
Unresolved conversations (4)
@halter73 halter73 Jul 10, 2023
Thanks for making the API changes I suggested. As much as I like them, we should at least send an API review email to make sure any API adjustments are approved before merging. Or better yet discuss them in tomorrow's meeting.
...ons/src/IServiceProviderIsKeyedService.cs
@halter73 halter73 Jul 3, 2023
So there are no null checks on the `object serviceKey` and calling `GetKeyedService(serviceType, null)` is equivalent to calling `GetService(serviceType)`? I actually like that behavior, but then we should mark the `serviceKey` as nullable everywhere in the public APIs. If we don't like that behavior, we can throw an `ArgumentNullException`. Either way, we should test the behaviour.
...ependencyInjection/src/ServiceProvider.cs
benjaminpetit
@steveharter steveharter Jun 22, 2023
sealed?
...actions/src/FromKeyedServicesAttribute.cs
halter73 benjaminpetit
@halter73 halter73 Jun 16, 2023
Shouldn't it be invalid for a `[ServiceKey]` parameter to be a `string` and not an object? People will want it to alwas be a string so they can store it in a string field like you do here. I think this proves my earlier point about how it's more convenient to work with this parameter when you know it will be a string.
Outdated
...sts/KeyedServiceProviderContainerTests.cs
benjaminpetit halter73
Resolved conversations (46)
@halter73 halter73 Jul 10, 2023
If we do keep keyed factories as the one thing that doesn't accept a null service key, we should add a test for someone incorrectly trying to add one with a null key. Sorry if I missed an existing test for this.
Outdated
...ion.Abstractions/src/ServiceDescriptor.cs
benjaminpetit
@ViktorHofer ViktorHofer Jul 10, 2023
Are these also not required anymore?
Outdated
...jection/src/CompatibilitySuppressions.xml
benjaminpetit
@steveharter steveharter Jul 7, 2023
Should this be `object? serviceKey` for pass-through support (like elsewhere in this PR)?
Outdated
...ceCollectionDescriptorExtensions.Keyed.cs
benjaminpetit
@steveharter steveharter Jul 7, 2023
Should this be `object? serviceKey` for pass-through support (like elsewhere in this PR)?
Outdated
...ceCollectionDescriptorExtensions.Keyed.cs
benjaminpetit
@steveharter steveharter Jul 7, 2023
Should this be `object? serviceKey` for pass-through support (like elsewhere in this PR)?
Outdated
...ceCollectionDescriptorExtensions.Keyed.cs
benjaminpetit
@steveharter steveharter Jul 7, 2023
Should this be `object? serviceKey` for pass-through support (like elsewhere in this PR)?
Outdated
...ceCollectionDescriptorExtensions.Keyed.cs
benjaminpetit
@steveharter steveharter Jul 7, 2023
Should this be `object? serviceKey` for pass-through support (like elsewhere in this PR)?
Outdated
...ceCollectionDescriptorExtensions.Keyed.cs
halter73 benjaminpetit
@steveharter steveharter Jul 7, 2023
Should this be `object? serviceKey` for pass-through support (like elsewhere in this PR)?
Outdated
...ceCollectionDescriptorExtensions.Keyed.cs
benjaminpetit
@steveharter steveharter Jul 7, 2023
This is a partial class and the other class also has the same doc which I don't think we need in both places. Also nit: extra whitespace before `/>`
Outdated
...ceCollectionDescriptorExtensions.Keyed.cs
benjaminpetit
@steveharter steveharter Jul 7, 2023
@ViktorHofer PTAL at these suppressions here - these are new APIs so should we conditionally exclude them in the .cs \ .csproj to avoid these suppressions?
Outdated
...actions/src/CompatibilitySuppressions.xml
benjaminpetit ViktorHofer
@steveharter steveharter Jul 7, 2023
Is this needed? ObjectFactory.#ctor v6->v7
Outdated
...actions/src/CompatibilitySuppressions.xml
benjaminpetit
@steveharter steveharter Jul 7, 2023
Typically in .ref files, there are not any `using` statements since the api gen tooling fully qualifies namespaces.
Outdated
...sions.DependencyInjection.Abstractions.cs
ViktorHofer
@halter73 halter73 Jul 3, 2023
Rename this file to `ServiceIdentifier.cs`.
Outdated
...erviceLookup/ServiceDescriptorCacheKey.cs
@halter73 halter73 Jul 3, 2023
```suggestion CallSiteFactory.Add(ServiceIdentifier.FromServiceType(typeof(IServiceProviderIsService)), new ConstantCallSite(typeof(IServiceProviderIsService), CallSiteFactory)); CallSiteFactory.Add(ServiceIdentifier.FromServiceType(typeof(IKeyedServiceProviderIsService)), new ConstantCallSite(typeof(IServiceProviderIsService), CallSiteFactory)); ``` I now feel way more strongly that `IKeyedServiceProviderIsService` should implement `IServiceProviderIsService`. Once things like RDF and RDG start supporting `[FromKeyedServices("name")]` parameters, it's probably going to want to be able to check for the existence of both keyed and non-keyed services so bad configurations can fail at startup, and needing to resolve two different services is annoying. @captainsafia I also worry a bit about the discoverability of `IKeyedServiceProviderIsService` because no one is going to interact with `IKeyedServiceProvider` directly, only through the `GetKeyedService` extension method on `IServiceProvider`. I wonder if `IServiceProviderIsKeyedService` is more discoverable since it also starts with `IServiceProvider`. Granted `IServiceProviderIsService` is a terrible name to start from, and maybe we don't need it to be super discoverable. I feel more strongly that we should at least have `IKeyedServiceProviderIsService : IServiceProviderIsService`. I cannot imagine a container supporting detecting only keyed services.
...ependencyInjection/src/ServiceProvider.cs
benjaminpetit
@halter73 halter73 Jul 3, 2023
Are there any tests for this? You need to be able to resolve the IKeyedServiceProviderIsService from the container which means it needs to be added to the CallSiteFactory in the ServiceProvider ctor like IServiceProviderIsService. We should add a specification test for this.
Outdated
...tion/src/ServiceLookup/CallSiteFactory.cs
benjaminpetit
@halter73 halter73 Jul 3, 2023
Can we put this in its own file please?
Outdated
...ractions/src/IServiceProviderIsService.cs
@halter73 halter73 Jul 3, 2023
Nit: `ServiceCollectionServiceExtensions.Keyed.cs`
Outdated
...erviceCollectionKeyedServiceExtensions.cs
@halter73 halter73 Jul 3, 2023
Nit: I think the convention for partial classes is to call this something like `ServiceCollectionDescriptorExtensions.Keyed.cs`. It makes it easier to find when looking for "ServiceCollectionDescriptorExtensions".
Outdated
...iceCollectionKeyedDescriptorExtensions.cs
@steveharter steveharter Jun 22, 2023
What are these suppressions are for (quick summary)? Thanks
Outdated
...actions/src/CompatibilitySuppressions.xml
benjaminpetit
@steveharter steveharter Jun 22, 2023
nit: could remove auto-added change this since no other changes here.
Outdated
...ventLog/src/CompatibilitySuppressions.xml
benjaminpetit
@steveharter steveharter Jun 22, 2023
sealed?
...actions/src/FromKeyedServicesAttribute.cs
benjaminpetit
@steveharter steveharter Jun 22, 2023
sealed?
...n.Abstractions/src/ServiceKeyAttribute.cs
benjaminpetit
@steveharter steveharter Jun 22, 2023
nit: add sealed
Outdated
....Abstractions/src/ISupportKeyedService.cs
benjaminpetit
@steveharter steveharter Jun 22, 2023
Are these suppressions needed? (I couldn't find the types)
Outdated
...n.Tests/src/CompatibilitySuppressions.xml
benjaminpetit
@steveharter steveharter Jun 22, 2023
style nit\preference: I favor `Service service1 = new()` syntax and try to avoid `var` in general whenever the type isn't spelled out later in the same line, but it looks like DI uses `var` quite a bit so keeping consistency at least within each file is good. Also, consistency is not so important for tests, but at least in production code.
...dDependencyInjectionSpecificationTests.cs
@steveharter steveharter Jun 22, 2023
nit: the `unchecked` could be lower
Outdated
...erviceLookup/ServiceDescriptorCacheKey.cs
benjaminpetit
@steveharter steveharter Jun 22, 2023
nit: unnecessary blank line
Outdated
...sts/KeyedServiceProviderContainerTests.cs
benjaminpetit
@steveharter steveharter Jun 22, 2023
I believe you'll need to add back the `DynamicallyAccessedMembersAttribute` here and elsewhere. The API gen tools may have removed it?
Outdated
...sions.DependencyInjection.Abstractions.cs
benjaminpetit
@captainsafia captainsafia Jun 22, 2023
Is the removal of these attributes intentional?
Outdated
...sions.DependencyInjection.Abstractions.cs
benjaminpetit
@ReubenBond ReubenBond Jun 22, 2023
nit: maybe use "match" terminology instead of "equals" and perhaps rename the arguments to `key` and `query` to convey the meaning a little more clearly
Outdated
...tion/src/ServiceLookup/CallSiteFactory.cs
benjaminpetit
@ReubenBond ReubenBond Jun 22, 2023
This could be `ServiceDescriptor.AnyServiceKey` or similar instead of adding a new class, to aid in discoverability. Someone may argue that this class should be named `ServiceKey`. I am ok either way. I wonder if there should be convenience APIs (perhaps extensions) for registering AnyKey services
...sions.DependencyInjection.Abstractions.cs
@halter73 halter73 Jun 19, 2023
Should this be a private type that overrides `ToString()` to return something other than "System.Object"? Maybe it could "AnyKey" or "*" instead.
Outdated
....Abstractions/src/ISupportKeyedService.cs
benjaminpetit
@halter73 halter73 Jun 17, 2023
This seems really bad. Doesn't this mean that a named services `Func<ServiceProviderEngineScope, object?>` could replace that of an equivalent non-named services. That could be really nasty to try to debug 😱. It also means that we always use the `CallSiteRuntimeResolver` and never upgrade to the `ILEmitResolverBuilder`. This is also fixed by adding the name to the ServiceCallSite, right?
Outdated
...ependencyInjection/src/ServiceProvider.cs
benjaminpetit vukovinski
@halter73 halter73 Jun 17, 2023
Shouldn't we add the name to the ServiceCallSite? And to CallSiteValidator.ValidateResolution()? Right now, I don't think our CallSiteValidator is going to identity any scoped-service misusage for named services. We should add a test for this.
Outdated
...ependencyInjection/src/ServiceProvider.cs
benjaminpetit vukovinski
@halter73 halter73 Jun 17, 2023
Nit: ```suggestion return ((ServiceType.GetHashCode() ?? 23) * 397) ^ ServiceKey.GetHashCode(); ``` Can we not use `HashCode.Combine` because of the nestandard2.0 target? `HashCode.Combine` doesn't have an unchecked block. Do we really need it here?
Outdated
...erviceLookup/ServiceDescriptorCacheKey.cs
captainsafia benjaminpetit
@halter73 halter73 Jun 17, 2023
Do we need to add this to `IServiceProviderIsService`? I assume that the existing `IsService(Type)` method will always return false for a named service, is that right?
Outdated
...tion/src/ServiceLookup/CallSiteFactory.cs
benjaminpetit
@halter73 halter73 Jun 16, 2023
Do we need any of the `TryAdd` methods?
Outdated
...sions.DependencyInjection.Abstractions.cs
benjaminpetit
@halter73 halter73 Jun 16, 2023
Do we have a test for `IEnumerable`-resolving named services?
...tion/src/ServiceLookup/CallSiteFactory.cs
benjaminpetit
@halter73 halter73 Jun 16, 2023
Do we have a test for an open generic named service?
...tion/src/ServiceLookup/CallSiteFactory.cs
@halter73 halter73 Jun 16, 2023
Since we're adding these both at once, should we just make `ISupportRequiredKeyedService` the one and only new interface you have to implement? Naturally, it'd have the normal `GetKeyedService` too. We could just call it `ISupportNamedService` :wink: It makes this method a little less complicated and saves us an interface. I cannot see why you would implement one without the other.
Outdated
.../ServiceProviderKeyedServiceExtensions.cs
benjaminpetit
@halter73 halter73 Jun 16, 2023
I'd put these with all the other ServiceCollectionServiceExtensions too.
Outdated
...sions.DependencyInjection.Abstractions.cs
@halter73 halter73 Jun 16, 2023
Have we considered how we're going to prevent .NET 7 and earlier versions of our default ServiceProvider and third-party containers from misinterpreting named services? I asked about this [in the API proposal](https://github.com/dotnet/runtime/issues/64427#issuecomment-1532412173). It seems difficult, but maybe we could do something. Like, maybe for named services, you have to get the `NamedImplementationType` or the `NamedImplementationInstance` and all the normal `Implementation` getters throw.
...ion.Abstractions/src/ServiceDescriptor.cs
benjaminpetit halter73
@halter73 halter73 Jun 16, 2023
Do we need the "key" parameter for this overload?
Outdated
...sts/KeyedServiceProviderContainerTests.cs
benjaminpetit halter73
@halter73 halter73 Jun 16, 2023
Why not add all these to the existing `ServiceProviderServiceExtensions` along with `GetService<T>()`, `GetRequiredService(Type)`, etc...?
...sions.DependencyInjection.Abstractions.cs
benjaminpetit
@halter73 halter73 Jun 16, 2023
Does the key need to be an object? What's the use case for anything other than a string here? Is it just so we can guarantee no conflicts with the `AnyKey`? A string is a lot easier to work with. I like "name" over "key" for this if we do go with string. It'd be nice if registering a named `IConfigureOptions<TOptions>` service would configure the corresponding named options without forcing you to register an `IConfigureNamedOptions` that gets called for options with any name. Similarly, resolving a named `IOptions<TOptions>` would be easier than resolving named options from `IOptionsMontor`. I think not doing this might be a missed opportunity, because if we wait to do it in .NET 9, it might be too breaking.
Outdated
...sions.DependencyInjection.Abstractions.cs
halter73 benjaminpetit
@ericstj ericstj Jun 7, 2023
It'd be good to clean up the suppressions in this diff. Not sure why you're hitting these, but if you need some help wrangling the dotnet/runtime infra reach out to @steveharter to get some tips for working on these libraries and adding API.
Outdated
...erfaces/src/CompatibilitySuppressions.xml