diff --git a/docs/workflow/trimming/feature-switches.md b/docs/workflow/trimming/feature-switches.md index e7aa6be63cf97..5a693bd7bc00e 100644 --- a/docs/workflow/trimming/feature-switches.md +++ b/docs/workflow/trimming/feature-switches.md @@ -25,6 +25,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif | EnableCppCLIHostActivation | System.Runtime.InteropServices.EnableCppCLIHostActivation | C++/CLI host activation code is disabled when set to false and related functionality can be trimmed. | | MetadataUpdaterSupport | System.Reflection.Metadata.MetadataUpdater.IsSupported | Metadata update related code to be trimmed when set to false | | _EnableConsumingManagedCodeFromNativeHosting | System.Runtime.InteropServices.EnableConsumingManagedCodeFromNativeHosting | Getting a managed function from native hosting is disabled when set to false and related functionality can be trimmed. | +| VerifyDependencyInjectionOpenGenericServiceTrimmability | Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability | When set to true, DependencyInjection will verify trimming annotations applied to open generic services are correct | | NullabilityInfoContextSupport | System.Reflection.NullabilityInfoContext.IsSupported | Nullable attributes can be trimmed when set to false | | _AggressiveAttributeTrimming | System.AggressiveAttributeTrimming | When set to true, aggressively trims attributes to allow for the most size savings possible, even if it could result in runtime behavior changes | diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/Microsoft.Extensions.DependencyInjection.Abstractions.csproj b/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/Microsoft.Extensions.DependencyInjection.Abstractions.csproj index b275c71c780c7..dc32454f1759a 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/Microsoft.Extensions.DependencyInjection.Abstractions.csproj +++ b/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/Microsoft.Extensions.DependencyInjection.Abstractions.csproj @@ -1,4 +1,4 @@ - + $(NetCoreAppCurrent);netstandard2.1;netstandard2.0;net461 diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Substitutions.xml b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Substitutions.xml new file mode 100644 index 0000000000000..eb381de19d615 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Substitutions.xml @@ -0,0 +1,7 @@ + + + + + + + diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Suppressions.xml b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Suppressions.xml deleted file mode 100644 index 3a09880ba32d1..0000000000000 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Suppressions.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - - - ILLink - IL2055 - member - M:Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateOpenGeneric(Microsoft.Extensions.DependencyInjection.ServiceDescriptor,System.Type,Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteChain,System.Int32,System.Boolean) - - - \ No newline at end of file diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj index b93c0a8e61e66..7ee51306d042d 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj @@ -1,4 +1,4 @@ - + $(NetCoreAppCurrent);netstandard2.1;netstandard2.0;net461 @@ -19,6 +19,10 @@ $(DefineConstants);SAVE_ASSEMBLIES + + + + @@ -32,7 +36,7 @@ - + @@ -41,7 +45,6 @@ Link="Common\src\Extensions\ParameterDefaultValue\ParameterDefaultValue.cs" /> - diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Resources/Strings.resx b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Resources/Strings.resx index e76446a526b4f..efbb7cc4a7451 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Resources/Strings.resx +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Resources/Strings.resx @@ -174,4 +174,10 @@ Call site type {0} is not supported - \ No newline at end of file + + Generic implementation type '{0}' has a DynamicallyAccessedMembers attribute applied to a generic argument type, but the service type '{1}' doesn't have a matching DynamicallyAccessedMembers attribute on its generic argument type. + + + Generic implementation type '{0}' has a DefaultConstructorConstraint ('new()' constraint), but the generic service type '{1}' doesn't. + + diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs index a2e02ce4f5001..06bcdf632879c 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs @@ -51,11 +51,18 @@ private void Populate() SR.Format(SR.TypeCannotBeActivated, implementationType, serviceType)); } - if (serviceType.GetGenericArguments().Length != implementationType.GetGenericArguments().Length) + Type[] serviceTypeGenericArguments = serviceType.GetGenericArguments(); + Type[] implementationTypeGenericArguments = implementationType.GetGenericArguments(); + if (serviceTypeGenericArguments.Length != implementationTypeGenericArguments.Length) { throw new ArgumentException( SR.Format(SR.ArityOfOpenGenericServiceNotEqualArityOfOpenGenericImplementation, serviceType, implementationType), "descriptors"); } + + if (ServiceProvider.VerifyOpenGenericServiceTrimmability) + { + ValidateTrimmingAnnotations(serviceType, serviceTypeGenericArguments, implementationType, implementationTypeGenericArguments); + } } else if (descriptor.ImplementationInstance == null && descriptor.ImplementationFactory == null) { @@ -77,6 +84,68 @@ private void Populate() } } + /// + /// Validates that two generic type definitions have compatible trimming annotations on their generic arguments. + /// + /// + /// When open generic types are used in DI, there is an error when the concrete implementation type + /// has [DynamicallyAccessedMembers] attributes on a generic argument type, but the interface/service type + /// doesn't have matching annotations. The problem is that the trimmer doesn't see the members that need to + /// be preserved on the type being passed to the generic argument. But when the interface/service type also has + /// the annotations, the trimmer will see which members need to be preserved on the closed generic argument type. + /// + private static void ValidateTrimmingAnnotations( + Type serviceType, + Type[] serviceTypeGenericArguments, + Type implementationType, + Type[] implementationTypeGenericArguments) + { + Debug.Assert(serviceTypeGenericArguments.Length == implementationTypeGenericArguments.Length); + + for (int i = 0; i < serviceTypeGenericArguments.Length; i++) + { + Type serviceGenericType = serviceTypeGenericArguments[i]; + Type implementationGenericType = implementationTypeGenericArguments[i]; + + DynamicallyAccessedMemberTypes serviceDynamicallyAccessedMembers = GetDynamicallyAccessedMemberTypes(serviceGenericType); + DynamicallyAccessedMemberTypes implementationDynamicallyAccessedMembers = GetDynamicallyAccessedMemberTypes(implementationGenericType); + + if (!AreCompatible(serviceDynamicallyAccessedMembers, implementationDynamicallyAccessedMembers)) + { + throw new ArgumentException(SR.Format(SR.TrimmingAnnotationsDoNotMatch, implementationType.FullName, serviceType.FullName)); + } + + bool serviceHasNewConstraint = serviceGenericType.GenericParameterAttributes.HasFlag(GenericParameterAttributes.DefaultConstructorConstraint); + bool implementationHasNewConstraint = implementationGenericType.GenericParameterAttributes.HasFlag(GenericParameterAttributes.DefaultConstructorConstraint); + if (implementationHasNewConstraint && !serviceHasNewConstraint) + { + throw new ArgumentException(SR.Format(SR.TrimmingAnnotationsDoNotMatch_NewConstraint, implementationType.FullName, serviceType.FullName)); + } + } + } + + private static DynamicallyAccessedMemberTypes GetDynamicallyAccessedMemberTypes(Type serviceGenericType) + { + foreach (CustomAttributeData attributeData in serviceGenericType.GetCustomAttributesData()) + { + if (attributeData.AttributeType.FullName == "System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute" && + attributeData.ConstructorArguments.Count == 1 && + attributeData.ConstructorArguments[0].ArgumentType.FullName == "System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes") + { + return (DynamicallyAccessedMemberTypes)(int)attributeData.ConstructorArguments[0].Value; + } + } + + return DynamicallyAccessedMemberTypes.None; + } + + private static bool AreCompatible(DynamicallyAccessedMemberTypes serviceDynamicallyAccessedMembers, DynamicallyAccessedMemberTypes implementationDynamicallyAccessedMembers) + { + // The DynamicallyAccessedMemberTypes don't need to exactly match. + // The service type needs to preserve a superset of the members required by the implementation type. + return serviceDynamicallyAccessedMembers.HasFlag(implementationDynamicallyAccessedMembers); + } + // For unit testing internal int? GetSlot(ServiceDescriptor serviceDescriptor) { @@ -273,6 +342,10 @@ private ServiceCallSite TryCreateExact(ServiceDescriptor descriptor, Type servic return null; } + [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:MakeGenericType", + Justification = "MakeGenericType here is used to create a closed generic implementation type given the closed service type. " + + "Trimming annotations on the generic types are verified when 'Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability' is set, which is set by default when PublishTrimmed=true. " + + "That check informs developers when these generic types don't have compatible trimming annotations.")] private ServiceCallSite TryCreateOpenGeneric(ServiceDescriptor descriptor, Type serviceType, CallSiteChain callSiteChain, int slot, bool throwOnConstraintViolation) { if (serviceType.IsConstructedGenericType && diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs index afe118acf7909..a05e82f8a4498 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs @@ -30,6 +30,9 @@ public sealed class ServiceProvider : IServiceProvider, IDisposable, IAsyncDispo internal ServiceProviderEngineScope Root { get; } + internal static bool VerifyOpenGenericServiceTrimmability { get; } = + AppContext.TryGetSwitch("Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability", out bool verifyOpenGenerics) ? verifyOpenGenerics : false; + internal ServiceProvider(IEnumerable serviceDescriptors, ServiceProviderOptions options) { // note that Root needs to be set before calling GetEngine(), because the engine may need to access Root diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/Microsoft.Extensions.DependencyInjection.Tests.csproj b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/Microsoft.Extensions.DependencyInjection.Tests.csproj index 073451c2cd9f0..698e3ec3dd659 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/Microsoft.Extensions.DependencyInjection.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/Microsoft.Extensions.DependencyInjection.Tests.csproj @@ -1,8 +1,11 @@ - + $(NetCoreAppCurrent);net461 true + true + + $(NoWarn);CS0436 @@ -11,6 +14,12 @@ Link="Shared\SingleThreadedSynchronizationContext.cs" /> + + + + + + diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceLookup/CallSiteFactoryTest.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceLookup/CallSiteFactoryTest.cs index 8d6a78029feff..da24dca4e92cf 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceLookup/CallSiteFactoryTest.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceLookup/CallSiteFactoryTest.cs @@ -3,9 +3,11 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection; using System.Threading.Tasks; +using Microsoft.DotNet.RemoteExecutor; using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.DependencyInjection.Specification.Fakes; using Xunit; @@ -849,6 +851,51 @@ public void CallSitesAreUniquePerServiceTypeAndSlotWithOpenGenericInGraph() } } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] // RuntimeConfigurationOptions are not supported on .NET Framework (and neither is trimming) + public void VerifyOpenGenericTrimmabilityChecks() + { + RemoteInvokeOptions options = new RemoteInvokeOptions(); + options.RuntimeConfigurationOptions.Add("Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability", "true"); + + using RemoteInvokeHandle remoteHandle = RemoteExecutor.Invoke(() => + { + (Type, Type)[] invalidTestCases = new[] + { + (typeof(IFakeOpenGenericService<>), typeof(ClassWithNewConstraint<>)), + (typeof(IServiceWithoutTrimmingAnnotations<>), typeof(ServiceWithTrimmingAnnotations<>)), + (typeof(IServiceWithPublicConstructors<>), typeof(ServiceWithPublicProperties<>)), + (typeof(IServiceWithTwoGenerics<,>), typeof(ServiceWithTwoGenericsInvalid<,>)), + }; + foreach ((Type serviceType, Type implementationType) in invalidTestCases) + { + ServiceDescriptor[] serviceDescriptors = new[] + { + new ServiceDescriptor(serviceType, implementationType, ServiceLifetime.Singleton) + }; + + Assert.Throws(() => new CallSiteFactory(serviceDescriptors)); + } + + (Type, Type)[] validTestCases = new[] + { + (typeof(IFakeOpenGenericService<>), typeof(FakeOpenGenericService<>)), + (typeof(IServiceWithPublicConstructors<>), typeof(ServiceWithPublicConstructors<>)), + (typeof(IServiceWithTwoGenerics<,>), typeof(ServiceWithTwoGenericsValid<,>)), + (typeof(IServiceWithMoreMemberTypes<>), typeof(ServiceWithLessMemberTypes<>)), + }; + foreach ((Type serviceType, Type implementationType) in validTestCases) + { + ServiceDescriptor[] serviceDescriptors = new[] + { + new ServiceDescriptor(serviceType, implementationType, ServiceLifetime.Singleton) + }; + + Assert.NotNull(new CallSiteFactory(serviceDescriptors)); + } + }, options); + } + private static Func GetCallSiteFactory(params ServiceDescriptor[] descriptors) { var collection = new ServiceCollection(); @@ -887,5 +934,20 @@ private class ClassB { public ClassB(ClassC cc) { } } private class ClassC { } private class ClassD { public ClassD(ClassC cd) { } } private class ClassE { public ClassE(ClassB cb) { } } + + // Open generic with trimming annotations + private interface IServiceWithoutTrimmingAnnotations { } + private class ServiceWithTrimmingAnnotations<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T> : IServiceWithoutTrimmingAnnotations { } + + private interface IServiceWithPublicConstructors<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T> { } + private class ServiceWithPublicProperties<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>: IServiceWithPublicConstructors { } + private class ServiceWithPublicConstructors<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>: IServiceWithPublicConstructors { } + + private interface IServiceWithTwoGenerics { } + private class ServiceWithTwoGenericsInvalid : IServiceWithTwoGenerics { } + private class ServiceWithTwoGenericsValid : IServiceWithTwoGenerics { } + + private interface IServiceWithMoreMemberTypes<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicProperties)] T> { } + private class ServiceWithLessMemberTypes<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T> : IServiceWithMoreMemberTypes { } } }