Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3431716
Pulling over everything from https://github.com/aspnet/DependencyInje…
jbogard Nov 18, 2018
1286891
Removing LINQ from hot path; Cosmetic fixes
jbogard Nov 19, 2018
3ba35c9
Removing more LINQ in favor of explicit checks
jbogard Nov 26, 2018
2d241e2
Adding tests for positive cases
jbogard Nov 26, 2018
d7a3b82
Adding tests for abstract class constraint
jbogard Nov 27, 2018
bb1d6a8
Adding complex test for self-referencing constraint
jbogard Nov 27, 2018
f32e5b8
Removing LINQ in favor of explicit code
jbogard Nov 27, 2018
64e6808
Switching operator
jbogard Nov 27, 2018
a0b1e93
Inverting conditional as part of conditional operator
jbogard Nov 27, 2018
88b6c1d
Moving fakes over to specification
jbogard Nov 27, 2018
70b4847
Adding spec tests for more complex scenarios; combining ifs
jbogard Nov 27, 2018
7246fd4
Removing ctors from constraints; adding full benchmarks; Adding spec …
jbogard Nov 28, 2018
584de56
Adding spec test for resolving single non-matching generically constr…
jbogard Nov 28, 2018
8cde246
Adjusting benchmarks to target CallSiteFactory
jbogard Nov 29, 2018
9f866e0
Setting baseline
jbogard Nov 29, 2018
7499ba2
Fixing derived class and adjusting benchmarks to use consistent services
jbogard Nov 29, 2018
0626cdc
Throwing on singular open generic constraint violation; adding except…
jbogard Nov 29, 2018
f99b9da
Testing exception messages
jbogard Nov 29, 2018
ae792ae
Using simpler try-catch flow to test for closing generics
jbogard Dec 12, 2018
35c1f89
Merge branch 'master' into ConstrainedOpenGenerics
jbogard Dec 13, 2018
6e73daa
Adding explicit classes to not rely on primitives
jbogard Dec 14, 2018
66a64ba
Skipping failing spec tests for Autofac and StashBox
jbogard Dec 14, 2018
cd1b606
Merge branch 'master' into ConstrainedOpenGenerics
jbogard Feb 26, 2019
e0553cb
Fixing tests
jbogard Feb 26, 2019
90bfb82
Merge branch 'master' into ConstrainedOpenGenerics
jbogard Nov 15, 2019
8fae420
Merge branch 'master' into ConstrainedOpenGenerics
jbogard Mar 5, 2020
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 @@ -545,6 +545,29 @@ public void OpenGenericServicesCanBeResolved()
Assert.Same(singletonService, genericService.Value);
}

[Fact]
public void ConstrainedOpenGenericServicesCanBeResolved()
{
// Arrange
var collection = new TestServiceCollection();
collection.AddTransient(typeof(IFakeOpenGenericService<>), typeof(FakeOpenGenericService<>));
collection.AddTransient(typeof(IFakeOpenGenericService<>), typeof(ConstrainedFakeOpenGenericService<>));
var poco = new PocoClass();
collection.AddSingleton(poco);
collection.AddSingleton<IFakeSingletonService, FakeService>();
var provider = CreateServiceProvider(collection);
// Act
var allServices = provider.GetServices<IFakeOpenGenericService<PocoClass>>().ToList();
var constrainedServices = provider.GetServices<IFakeOpenGenericService<IFakeSingletonService>>().ToList();
var singletonService = provider.GetService<IFakeSingletonService>();
// Assert
Assert.Equal(2, allServices.Count);
Assert.Same(poco, allServices[0].Value);
Assert.Same(poco, allServices[1].Value);
Assert.Equal(1, constrainedServices.Count);
Assert.Same(singletonService, constrainedServices[0].Value);
}

[Fact]
public void ClosedServicesPreferredOverOpenGenericServices()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes
{
public class ConstrainedFakeOpenGenericService<TVal> : IFakeOpenGenericService<TVal>
where TVal : PocoClass
{
public ConstrainedFakeOpenGenericService(TVal value)
{
Value = value;
}
public TVal Value { get; }
}
}
119 changes: 116 additions & 3 deletions src/DependencyInjection/DI/src/ServiceLookup/CallSiteFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ private ServiceCallSite TryCreateExact(Type serviceType, CallSiteChain callSiteC

private ServiceCallSite TryCreateOpenGeneric(Type serviceType, CallSiteChain callSiteChain)
{
if (serviceType.IsConstructedGenericType
&& _descriptorLookup.TryGetValue(serviceType.GetGenericTypeDefinition(), out var descriptor))
if (serviceType.IsConstructedGenericType && _descriptorLookup.TryGetValue(serviceType.GetGenericTypeDefinition(), out var descriptor))
{
return TryCreateOpenGeneric(descriptor.Last, serviceType, callSiteChain, DefaultSlot);
}
Expand Down Expand Up @@ -211,7 +210,8 @@ private ServiceCallSite TryCreateExact(ServiceDescriptor descriptor, Type servic
private ServiceCallSite TryCreateOpenGeneric(ServiceDescriptor descriptor, Type serviceType, CallSiteChain callSiteChain, int slot)
{
if (serviceType.IsConstructedGenericType &&
serviceType.GetGenericTypeDefinition() == descriptor.ServiceType)
serviceType.GetGenericTypeDefinition() == descriptor.ServiceType &&
IsCompatibleWithGenericParameterConstraints(descriptor.ImplementationType, serviceType.GenericTypeArguments))
{
Debug.Assert(descriptor.ImplementationType != null, "descriptor.ImplementationType != null");
var lifetime = new ResultCache(descriptor.Lifetime, serviceType, slot);
Expand Down Expand Up @@ -358,6 +358,119 @@ private ServiceCallSite[] CreateArgumentCallSites(
return parameterCallSites;
}

private static bool IsCompatibleWithGenericParameterConstraints(Type genericTypeDefinition, Type[] parameters)
{
var genericArgumentDefinitions = genericTypeDefinition.GetTypeInfo().GenericTypeParameters;
for (var i = 0; i < genericArgumentDefinitions.Length; ++i)
{
var argumentDefinitionTypeInfo = genericArgumentDefinitions[i].GetTypeInfo();
var parameter = parameters[i];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turns a helpful ArgumentException:

System.ArgumentException : The number of generic arguments provided doesn't equal the arity of the generic type definition.
Parameter name: instantiation
Stack Trace:
   at System.RuntimeType.MakeGenericType(Type[] instantiation)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateOpenGeneric(ServiceDescriptor descriptor, Type serviceType, CallSiteChain callSiteChain, Int32 slot, Boolean throwOnConstraintViolation) in C:\dev\aspnet\Extensions\src\DependencyInjection\DI\src\ServiceLookup\CallSiteFactory.cs:line 216
...

into a relatively unhelpful IndexOutOfRangeException:

System.IndexOutOfRangeException : Index was outside the bounds of the array.
Stack Trace:
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.IsCompatibleWithGenericParameterConstraints(Type genericTypeDefinition, Type[] parameters) in C:\dev\aspnet\Extensions\src\DependencyInjection\DI\src\ServiceLookup\CallSiteFactory.cs:line 370
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateOpenGeneric(ServiceDescriptor descriptor, Type serviceType, CallSiteChain callSiteChain, Int32 slot, Boolean throwOnConstraintViolation) in C:\dev\aspnet\Extensions\src\DependencyInjection\DI\src\ServiceLookup\CallSiteFactory.cs:line 212

This change also made me realize that reordering generic parameters between the implementation type and service type is completely broken. The following implementation type could never implement IFakeOpenGenericService2<,> with our DI system:

public class FakeOpenGenericService2<T, U> : IFakeOpenGenericService2<U, T>

This change makes the situation slightly worse though because before you'd at least get a TypeLoadException if you tried to resolve IEnumerable<IFakeOpenGenericService2<Foo, Bar>> with FakeOpenGenericService2<,> registered as an IFakeOpenGenericService2<,>. After this change, FakeOpenGenericService2 would silently be omitted from the resolved IEnumerable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the first part. The second part is sort of the goal of this change.

var parameterTypeInfo = parameter.GetTypeInfo();
foreach (var constraint in argumentDefinitionTypeInfo.GetGenericParameterConstraints())
{
var substituteGenericParameterConstraint = SubstituteGenericParameterConstraint(parameters, constraint);
if (!ParameterCompatibleWithTypeConstraint(parameter, substituteGenericParameterConstraint))
{
return false;
}
}

var specialConstraints = argumentDefinitionTypeInfo.GenericParameterAttributes;
if ((specialConstraints & GenericParameterAttributes.DefaultConstructorConstraint) == GenericParameterAttributes.DefaultConstructorConstraint)
{
if (!parameterTypeInfo.IsValueType && parameterTypeInfo.DeclaredConstructors.Where(c => c.IsPublic).All(c => c.GetParameters().Length != 0))
{
return false;
}
}
if ((specialConstraints & GenericParameterAttributes.ReferenceTypeConstraint) == GenericParameterAttributes.ReferenceTypeConstraint)
{
if (parameterTypeInfo.IsValueType)
{
return false;
}
}
if ((specialConstraints & GenericParameterAttributes.NotNullableValueTypeConstraint) == GenericParameterAttributes.NotNullableValueTypeConstraint)
{
if (!parameterTypeInfo.IsValueType || parameterTypeInfo.IsGenericType && IsGenericTypeDefinedBy(parameter, typeof(Nullable<>)))
{
return false;
}
}
}
return true;
}

private static bool IsGenericTypeDefinedBy(Type type, Type openGeneric)
{
return !type.ContainsGenericParameters && type.IsGenericType && type.GetGenericTypeDefinition() == openGeneric;
}

private static Type SubstituteGenericParameterConstraint(Type[] parameters, Type constraint)
{
return !constraint.IsGenericParameter ? constraint : parameters[constraint.GenericParameterPosition];
}

private static bool ParameterCompatibleWithTypeConstraint(Type parameter, Type constraint)
{
if (constraint.IsAssignableFrom(parameter))
{
return true;
}

if (ParameterEqualsConstraint(parameter, constraint))
{
return true;
}

if (parameter.BaseType != null && ParameterEqualsConstraint(parameter.BaseType, constraint))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems odd to me that you would check the parent type, but not the grandparent type, great-grandparent type, etc...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to just wrap a try/catch around descriptor.ImplementationType.MakeGenericType(serviceType.GenericTypeArguments), and handle the appropriate exceptions. I'm pretty concerned that this logic will find constraint violations where there aren't any.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbogard @halter73
Chiming in here as I have some experience with this from LightInject. Actually this was one of the things that was really hard to get right as it would practically mimic the behaviour of the compiler. I looked at several other implementations and if I remember correctly, they all do a MakeGenericType in the end to make sure.

For what its worth, here is what I've got for LightInject

https://github.com/seesharper/LightInject/blob/70cfc68718a57bf02959bf7a09fe59f0d372cae5/src/LightInject/LightInject.cs#L6808

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halter73 that was my first go (see https://github.com/aspnet/DependencyInjection/pull/472/files#diff-599c17314f9888a15039a1f175fb6c35) - to only rely on MakeGenericType and not try to reproduce any of the generic type loading arguments. Honestly, I'd still rather go that route, as it's the most correct option. However, it makes exceptions part of the hot path and becomes a pretty big perf hit for constraint misses.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I'm a still bit unsure of the overall performance hit, since hit's only the first resolution that the exception hits for constraint misses, and from then on out the resolution plan is cached.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to me the long-term solution is to remove all of this in favor of my API proposal https://github.com/dotnet/corefx/issues/33769 - TryMakeGenericType, since I've just tried to reproduce the easy cases with this approach. These checks, if successful, still get retried with a call to MakeGenericType.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @halter73. Taking perf hit for an uncommon case to make sure things stay correct is acceptable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll push the simple, but comprehensive, but perf hit case. I prefer that as well - it will make things easy when (if?) TryMakeGenericType happens.

{
return true;
}

foreach (var interfaceType in parameter.GetTypeInfo().ImplementedInterfaces)
{
if (ParameterEqualsConstraint(interfaceType, constraint))
{
return true;
}
}

return false;
}

private static bool ParameterEqualsConstraint(Type parameter, Type constraint)
{
var genericArguments = parameter.GenericTypeArguments;
if (genericArguments.Length > 0 && constraint.IsGenericType)
{
var typeDefinition = constraint.GetGenericTypeDefinition();
if (typeDefinition.GetTypeInfo().GenericTypeParameters.Length == genericArguments.Length)
{
var constraintArguments = constraint.GetTypeInfo().GenericTypeArguments;
for (var i = 0; i < constraintArguments.Length; i++)
{
var constraintArgument = constraintArguments[i];
if (!constraintArgument.IsGenericParameter && !constraintArgument.IsAssignableFrom(genericArguments[i]))
{
return false;
}
}

Type genericType;
try
{
genericType = typeDefinition.MakeGenericType(genericArguments);
}
catch (ArgumentException)
{
return false;
}
return genericType == parameter;
}
}
return false;
}

public void Add(Type type, ServiceCallSite serviceCallSite)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace Microsoft.Extensions.DependencyInjection.Tests.Fakes
{
public class ClassInheritingAbstractClass : AbstractClass
{

}

public class ClassAlsoInheritingAbstractClass : AbstractClass
{

}

public class ClassInheritingClassInheritingAbstractClass : AbstractClass
{

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.Extensions.DependencyInjection.Specification.Fakes;

namespace Microsoft.Extensions.DependencyInjection.Fakes
{
public class ConstrainedFakeOpenGenericService<TVal> : IFakeOpenGenericService<TVal>
where TVal : PocoClass
{
public ConstrainedFakeOpenGenericService(TVal value)
{
Value = value;
}
public TVal Value { get; }
}
}
Loading