-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Return null instead of throwing in ServiceDescriptor.ImplementationInstance\Type\Factory if a keyed service #105776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,24 +152,22 @@ private ServiceDescriptor(Type serviceType, object? serviceKey, ServiceLifetime | |
| private Type? _implementationType; | ||
|
|
||
| /// <summary> | ||
| /// Gets the <see cref="Type"/> that implements the service. | ||
| /// Gets the <see cref="Type"/> that implements the service, | ||
| /// or returns <see langword="null"/> if <see cref="IsKeyedService"/> is <see langword="true"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// If <see cref="IsKeyedService"/> is <see langword="true"/>, <see cref="KeyedImplementationType"/> should be called instead. | ||
| /// </remarks> | ||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] | ||
| public Type? ImplementationType | ||
| { | ||
| get | ||
| { | ||
| if (IsKeyedService) | ||
| { | ||
| ThrowKeyedDescriptor(); | ||
| } | ||
| return _implementationType; | ||
| } | ||
| } | ||
| public Type? ImplementationType => IsKeyedService ? null : _implementationType; | ||
|
|
||
| /// <summary> | ||
| /// Gets the <see cref="Type"/> that implements the service. | ||
| /// Gets the <see cref="Type"/> that implements the service, | ||
| /// or throws <see cref="InvalidOperationException"/> if <see cref="IsKeyedService"/> is <see langword="false"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// If <see cref="IsKeyedService"/> is <see langword="false"/>, <see cref="ImplementationType"/> should be called instead. | ||
| /// </remarks> | ||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] | ||
| public Type? KeyedImplementationType | ||
| { | ||
|
|
@@ -186,23 +184,21 @@ public Type? KeyedImplementationType | |
| private object? _implementationInstance; | ||
|
|
||
| /// <summary> | ||
| /// Gets the instance that implements the service. | ||
| /// Gets the instance that implements the service, | ||
| /// or returns <see langword="null"/> if <see cref="IsKeyedService"/> is <see langword="true"/>. | ||
| /// </summary> | ||
| public object? ImplementationInstance | ||
| { | ||
| get | ||
| { | ||
| if (IsKeyedService) | ||
| { | ||
| ThrowKeyedDescriptor(); | ||
| } | ||
| return _implementationInstance; | ||
| } | ||
| } | ||
| /// <remarks> | ||
| /// If <see cref="IsKeyedService"/> is <see langword="true"/>, <see cref="KeyedImplementationInstance"/> should be called instead. | ||
| /// </remarks> | ||
| public object? ImplementationInstance => IsKeyedService ? null : _implementationInstance; | ||
|
|
||
| /// <summary> | ||
| /// Gets the instance that implements the service. | ||
| /// Gets the instance that implements the service, | ||
| /// or throws <see cref="InvalidOperationException"/> if <see cref="IsKeyedService"/> is <see langword="false"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// If <see cref="IsKeyedService"/> is <see langword="false"/>, <see cref="ImplementationInstance"/> should be called instead. | ||
| /// </remarks> | ||
| public object? KeyedImplementationInstance | ||
| { | ||
| get | ||
|
|
@@ -218,23 +214,21 @@ public object? KeyedImplementationInstance | |
| private object? _implementationFactory; | ||
|
|
||
| /// <summary> | ||
| /// Gets the factory used for creating service instances. | ||
| /// Gets the factory used for creating service instance, | ||
| /// or returns <see langword="null"/> if <see cref="IsKeyedService"/> is <see langword="true"/>. | ||
| /// </summary> | ||
| public Func<IServiceProvider, object>? ImplementationFactory | ||
| { | ||
| get | ||
| { | ||
| if (IsKeyedService) | ||
| { | ||
| ThrowKeyedDescriptor(); | ||
| } | ||
| return (Func<IServiceProvider, object>?)_implementationFactory; | ||
| } | ||
| } | ||
| /// <remarks> | ||
| /// If <see cref="IsKeyedService"/> is <see langword="true"/>, <see cref="KeyedImplementationFactory"/> should be called instead. | ||
| /// </remarks> | ||
| public Func<IServiceProvider, object>? ImplementationFactory => IsKeyedService ? null : (Func<IServiceProvider, object>?) _implementationFactory; | ||
|
|
||
| /// <summary> | ||
| /// Gets the factory used for creating Keyed service instances. | ||
| /// Gets the factory used for creating Keyed service instances, | ||
| /// or throws <see cref="InvalidOperationException"/> if <see cref="IsKeyedService"/> is <see langword="false"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// If <see cref="IsKeyedService"/> is <see langword="false"/>, <see cref="ImplementationFactory"/> should be called instead. | ||
| /// </remarks> | ||
| public Func<IServiceProvider, object?, object>? KeyedImplementationFactory | ||
| { | ||
| get | ||
|
|
@@ -1058,8 +1052,6 @@ private string DebuggerToString() | |
| return debugText; | ||
| } | ||
|
|
||
| private static void ThrowKeyedDescriptor() => throw new InvalidOperationException(SR.KeyedDescriptorMisuse); | ||
|
|
||
| private static void ThrowNonKeyedDescriptor() => throw new InvalidOperationException(SR.NonKeyedDescriptorMisuse); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't throw when accessing on non-leyed property when the descriptor is kedyed, shoud we also remove this, to be consistent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering about that as well. The original issue was a backwards compat issue with existing APIs so not throwing there makes sense. Throwing for the new APIs I think is OK and desired to help catch issues (as originally intended). Other than consistency, can you think of a scenario where we would want to return null for the new APIs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope it's only for consistency's sake. It's fine to me to throw here otherwise |
||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.