Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,7 @@
<data name="KeyedServicesNotSupported" xml:space="preserve">
<value>This service provider doesn't support keyed services.</value>
</data>
<data name="KeyedDescriptorMisuse" xml:space="preserve">
<value>This service descriptor is keyed. Your service provider may not support keyed services.</value>
</data>
<data name="NonKeyedDescriptorMisuse" xml:space="preserve">
<value>This service descriptor is not keyed.</value>
</data>
</root>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -546,9 +546,9 @@ public static TheoryData NotNullServiceKeyData
public void NotNullServiceKey_IsKeyedServiceTrue(ServiceDescriptor serviceDescriptor)
{
Assert.True(serviceDescriptor.IsKeyedService);
Assert.Throws<InvalidOperationException>(() => serviceDescriptor.ImplementationInstance);
Assert.Throws<InvalidOperationException>(() => serviceDescriptor.ImplementationType);
Assert.Throws<InvalidOperationException>(() => serviceDescriptor.ImplementationFactory);
Assert.Null(serviceDescriptor.ImplementationInstance);
Assert.Null(serviceDescriptor.ImplementationType);
Assert.Null(serviceDescriptor.ImplementationFactory);
}
}
}