Resolve MakeGenericType ILLink warning in DependencyInjection #55102

eerhardt · 2021-07-02T22:00:11Z

Resolve the ILLink warning in DependencyInjection by adding a runtime check that is behind a new AppContext switch 'Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability'. The runtime check ensures the trimming annotations on the open generic types are compatible between the service and implementation types. The check is enabled by default when PublishTrimmed=true.

Here is a simple repro of the trimming issue where the developer doesn't see a warning:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>
    <TrimmerDefaultAction>link</TrimmerDefaultAction>
    <PublishTrimmed>true</PublishTrimmed>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="6.0.0-*" />
  </ItemGroup>
</Project>

using Microsoft.Extensions.DependencyInjection;
using System;
using System.Diagnostics.CodeAnalysis;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            IServiceProvider sp = new ServiceCollection()
                .AddSingleton(typeof(IFactory<>), typeof(Factory<>))
                .BuildServiceProvider();

            var factory = sp.GetService<IFactory<Customer>>();

            Console.WriteLine(factory.Create());
        }
    }

    interface IFactory<T>
    {
        T Create();
    }

    class Factory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] T>
        : IFactory<T>
    {
        public T Create()
        {
            return Activator.CreateInstance<T>();
        }
    }

    class Customer { }
}

If you publish this app, it doesn't work:

Unhandled exception. System.MissingMethodException: Cannot dynamically create an instance of type 'ConsoleApp1.Customer'. Reason: No parameterless constructor defined.
   at System.RuntimeType.ActivatorCache..ctor(RuntimeType ) in System.Private.CoreLib.dll:token 0x600041c+0x93
   at System.RuntimeType.CreateInstanceOfT() in System.Private.CoreLib.dll:token 0x60003e7+0xf
   at System.Activator.CreateInstance[T]() in System.Private.CoreLib.dll:token 0x6000601+0x0
   at ConsoleApp1.Factory`1.Create() in C:\Users\eerhardt\source\repos\WorkerService4\ConsoleApp1\Program.cs:line 31
   at ConsoleApp1.Program.Main(String[] ) in C:\Users\eerhardt\source\repos\WorkerService4\ConsoleApp1\Program.cs:line 17

Today, the developer sees a warning coming from DependencyInjection:

warning IL2104: Assembly 'Microsoft.Extensions.DependencyInjection' produced trim warnings. For more information see https://aka.ms/dotnet-illink/libraries

That is the warning being suppressed here and being validated by the new runtime check.

dotnet-issue-labeler · 2021-07-02T22:00:17Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-07-02T22:00:22Z

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

This adds RequiresUnreferencedCode to any DI API that could take a separate Service and Implementation type. The reason is because if the Implementation type is generic and has a DynamicallyAccessedMembers annotation on its generic types, and the Service (interface) type doesn't, the trimmer won't preserve the correct members on the inner type. This leads to runtime failures.

Adding the RequiresUnreferencedCode attribute allows us to suppress the MakeGenericType warning we are getting in DependencyInjection, but it forces all places that add DI services to ensure their types annotations match, and then suppress the warning.

Author:	eerhardt
Assignees:	-
Labels:	`area-Extensions-DependencyInjection`, `new-api-needs-documentation`
Milestone:	-

ghost · 2021-07-02T22:00:33Z

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

This adds RequiresUnreferencedCode to any DI API that could take a separate Service and Implementation type. The reason is because if the Implementation type is generic and has a DynamicallyAccessedMembers annotation on its generic types, and the Service (interface) type doesn't, the trimmer won't preserve the correct members on the inner type. This leads to runtime failures.

Adding the RequiresUnreferencedCode attribute allows us to suppress the MakeGenericType warning we are getting in DependencyInjection, but it forces all places that add DI services to ensure their types annotations match, and then suppress the warning.

Author:	eerhardt
Assignees:	-
Labels:	`area-Extensions-DependencyInjection`, `linkable-framework`
Milestone:	-

eerhardt · 2021-07-03T00:16:28Z

Heads up @pranavkm - when this change gets to the aspnetcore repo, we will probably need to suppress some new warnings.

I can probably do that in aspnetcore.

davidfowl · 2021-07-03T07:20:44Z

This change doesn't look right. This is a type system limitation that will cause things to warn unnecessarily just in case you use a generic type in that slot. This should only warn for open generic types not closed ones. It's too aggressive

eerhardt · 2021-07-06T15:01:26Z

This should only warn for open generic types not closed ones. It's too aggressive

Agreed. But given the current feature set of the trimmer, I don't see how it is possible to make it less aggressive.

cc @vitek-karas @marek-safar @agocke @MichalStrehovsky @sbomer

davidfowl · 2021-07-06T15:34:32Z

Let it break for open generics and build an analyzer to detect open generics used in these methods. This is way too aggressive for my liking.

sbomer · 2021-07-06T16:07:52Z

src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs

@@ -13,12 +13,15 @@ namespace Microsoft.Extensions.DependencyInjection
    [DebuggerDisplay("Lifetime = {Lifetime}, ServiceType = {ServiceType}, ImplementationType = {ImplementationType}")]
    public class ServiceDescriptor
    {
+        internal const string RequiresUnreferencedCode = "Using generic types in DependencyInjection can lead to required code being trimmed. Ensure any generic Service types (e.g. 'IMyService<T>') have matching trimming annotations as the Implementation types implementing them (e.g. 'MyService<[DynamicallyAccessedMembers] T>).";


Suggested change

internal const string RequiresUnreferencedCode = "Using generic types in DependencyInjection can lead to required code being trimmed. Ensure any generic Service types (e.g. 'IMyService<T>') have matching trimming annotations as the Implementation types implementing them (e.g. 'MyService<[DynamicallyAccessedMembers] T>).";

internal const string RequiresUnreferencedCode = "Using generic types in DependencyInjection can lead to required code being trimmed. Ensure any generic Service types (e.g. 'IMyService<T>') have the same trimming annotations as the Implementation types implementing them (e.g. 'MyService<[DynamicallyAccessedMembers] T>).";

sbomer · 2021-07-06T16:12:42Z

...ons.DependencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.cs

@@ -129,6 +129,9 @@ public static class ServiceCollectionDescriptorExtensions
        /// </summary>
        /// <param name="collection">The <see cref="IServiceCollection"/>.</param>
        /// <param name="service">The type of the service to register.</param>
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


Suggested change

Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

sbomer · 2021-07-06T16:13:28Z

...ons.DependencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.cs

@@ -217,6 +221,9 @@ public static class ServiceCollectionDescriptorExtensions
        /// </summary>
        /// <typeparam name="TService">The type of the service to add.</typeparam>
        /// <param name="collection">The <see cref="IServiceCollection"/>.</param>
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


Suggested change

Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "TryAddTransient has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

sbomer · 2021-07-06T16:14:02Z

...ons.DependencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.cs

@@ -270,6 +278,9 @@ public static class ServiceCollectionDescriptorExtensions
        /// </summary>
        /// <param name="collection">The <see cref="IServiceCollection"/>.</param>
        /// <param name="service">The type of the service to register.</param>
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


Suggested change

Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

sbomer · 2021-07-06T16:15:04Z

...ons.DependencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.cs

@@ -358,6 +370,9 @@ public static class ServiceCollectionDescriptorExtensions
        /// </summary>
        /// <typeparam name="TService">The type of the service to add.</typeparam>
        /// <param name="collection">The <see cref="IServiceCollection"/>.</param>
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


Suggested change

Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "TryAddScoped has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

sbomer · 2021-07-06T16:31:15Z

src/libraries/Microsoft.Extensions.Hosting/src/HostingHostBuilderExtensions.cs

                collection.Configure(configureOptions);
            });
        }

+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


Suggested change

Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

sbomer · 2021-07-06T16:31:46Z

src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs

@@ -282,5 +283,14 @@ private void CreateServiceProvider()
            // service provider, ensuring it will be properly disposed with the provider
            _ = _appServices.GetService<IConfiguration>();
        }
+
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


Suggested change

Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

sbomer · 2021-07-06T16:32:19Z

...rosoft.Extensions.Hosting.WindowsServices/src/WindowsServiceLifetimeHostBuilderExtensions.cs

@@ -67,5 +67,13 @@ public static IHostBuilder UseWindowsService(this IHostBuilder hostBuilder, Acti

            return hostBuilder;
        }
+
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


Suggested change

Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

sbomer · 2021-07-06T16:34:58Z

src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs

@@ -19,6 +19,9 @@ public static class OptionsServiceCollectionExtensions
        /// </summary>
        /// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
        /// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "ServiceDescriptor's have RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


Suggested change

Justification = "ServiceDescriptor's have RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

sbomer · 2021-07-06T16:36:59Z

...soft.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs

@@ -20,6 +20,9 @@ public static class HttpClientFactoryServiceCollectionExtensions
        /// </summary>
        /// <param name="services">The <see cref="IServiceCollection"/>.</param>
        /// <returns>The <see cref="IServiceCollection"/>.</returns>
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +


This one looks like it silences warnings from ServiceDescriptor static methods, and also from TryAddTransient/TryAddSingleton - I wonder if that's worth mentioning in the justification.

Suggested change

Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Justification = "ServiceDescriptor, TryAddTransient, and TryAddSingleton have RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

davidfowl · 2021-07-06T17:27:41Z

I think this should be a forcing function to improve generics (the language and runtime), attributes and the linker. We shouldn't make the library unusable because of those limitations.

MichalStrehovsky · 2021-07-08T08:50:14Z

Let it break for open generics and build an analyzer to detect open generics used in these methods. This is way too aggressive for my liking.

We do not break user code without warnings. Suppression of a warning that can result in broken code after trimming is a buggy suppression - not all dynamic patterns can be described statically - it's laws of physics. If this pattern ends up being general enough warranting a illinker feature to describe it, we can add an illinker feature to describe it.

davidfowl · 2021-07-08T08:53:24Z

This can be described statically and I'd be ok if it warned for just open generics but we don't currently have a way to describe that. I don't think it's reasonable to make everything suffer because of that one scenario.

MichalStrehovsky · 2021-07-08T08:59:27Z

I don't think it's reasonable to make everything suffer because of that one scenario.

What's the failure mode when the constructor gets stripped? Will the user get actionable feedback on what to do next?

The warning added here states what the potential problem can be. I would not describe it as "suffering".

I've seen a lot of failure modes caused by tools stripping necessary things. That is a lot closer to what I see when someone says "suffering":

It happens after one already debugged their app and they're ready to ship (hitting Publish instead of F5 debug).
The failure mode is obscure (ranging from an exception saying something wasn't found, through a NullReferenceException to, "a control doesn't show up" or "nothing happens when I click a button").

davidfowl · 2021-07-08T09:08:27Z

What's the failure mode when the constructor gets stripped? Will the user get actionable feedback on what to do next?

The failure mode is based on what happens with the generic type. I don't know how they would know what to do next as its really dependent on the usage of the T in the concrete implementation of the generic.

jkotas · 2021-07-08T13:36:03Z

Is it an option to add new set of APIs that disallow the open generic types and that are trimming friendly? Then the fix for this warning would be to use the new trimming friendly API, and the warnings would be only issued for the really problematic cases.

vitek-karas · 2021-07-08T13:40:28Z

@eerhardt and I discussed this offline and we both like the analyzer solution proposed by @davidfowl, just "improved".

The way I see this is that this is a pattern for which we don't have a solution in the trimmer. We don't even know what the solution would look like yet. And we won't get one in .NET 6 (too late for this I think).

I also think (based on my own perception as well as feedback from @davidfowl) that we can't accept the PR as is - it would produce too much noise. The assumption is that vast majority of the callsites for the problematic methods are actually completely trim compatible, we just can't detect it. So generating warnings for all of them is just super noisy.

My assumptions:

We won't have a proper solution in 6 (be it trimmer based or new APIs)
We think it's too important to not to anything about this (as in accept this PR)

So I accept that this is a problem we don't have a nice solution for in .NET 6 but at the same time we want at least some solution. So using a "hacky" solution seems like an OK thing to do - as long as we see it as a point-in-time change.

The "hacky" solution would be to build understanding of this into the linker:

Linker would recognize these methods intrinsically (using custom step infra so that it's not hardwired into the core logic) and check that the types involved are open generic, and only then issue a warning. For all other cases it would not warn.
We would build similar capability into our analyzer
We would suppress the warning in the framework (the reason why this PR exists) - with a good comment and how to remove it later on when we do have a proper solution.

This is not too different from what we're doing for example for operators and System.Linq.Expressions... we're also patching a problem with intrinsic linker behavior.

Obviously I would like to avoid doing this, but if we think this is too important the above solution doesn't sound too bad.

jkotas · 2021-07-08T13:56:09Z

My assumptions:
We think it's too important to not to anything about this

It would be useful to clarify why it is must-have to do something about this for .NET 6.

eerhardt · 2021-07-08T23:24:03Z

why it is must-have to do something about this for .NET 6.

The current behavior isn't acceptable in my opinion. If I dotnet publish the repro project at the top of this PR, I do get a warning:

warning IL2104: Assembly 'Microsoft.Extensions.DependencyInjection' produced trim warnings. For more information see https://aka.ms/dotnet-illink/libraries

Navigating to that link brings me to a page that shows me how to make my own library trimmable - not what I can do because Microsoft.Extensions.DependencyInjection isn't fully trim-compatible.

As a user, I have no idea what is the problem with DependencyInjection. And this is the same warning I get for something that 100% doesn't work with trimming - like System.ComponentModel.Composition, System.CodeDom, or System.DirectoryServices. However, 95% of DependencyInjection works just fine with trimming. It is just this one last issue - open generic services - that is causing a problem. If there was a way to just flag those usages, I think the user experience would be perfect.

The situation would be slightly different if this was a one-off, rarely used library. But Microsoft.Extensions.DependencyInjection.Abstractions has been downloaded over one billion times - half a million a day. It is an extremely used library, and is the heart of all our other "Microsoft.Extensions" libraries. It is going to get used in even more places going forward with Maui apps taking a dependency on it.

For these reasons, I think this is an issue we must do something about in .NET 6 to complete our "make the .NET Libraries trim compatible" user story #43078.

jkotas · 2021-07-09T00:09:06Z

I agree that this is a popular library. But I do not think doing something about it in .NET 6 will move the needle significantly from the user scenario point of view. The app models where this library is used will generated a ton of other impossible to reason about warnings in .NET 6. (For example, I have just tried dotnet new web + dotnet publish -r win-x64 /p:PublishTrimmed=true.)

I understand that the special casing in the linker is always the easy way out. I do not think it is sustainable. If we want to do something about this, we should be looking at fixing this properly, and not by special casing in the linker.

eerhardt · 2021-07-09T02:37:23Z

The app models where this library is used will generated a ton of other impossible to reason about warnings in .NET 6.

Even Maui?

Note: this isn't just for app developers. If library authors want to make their libraries trimmable, our docs say to reference the library in a console app, and run the linker to get all the warnings in the library. If the library interacts with DependencyInjection, giving them a decent warning when they inject open generic services would actually move the needle to helping them make their libraries trim compatible.

davidfowl · 2021-07-09T03:22:31Z

We should absolutely try our best to make the trim warnings accurate and safe but this PR isn't the solution. I like the idea that the linker would have knowledge of this open generic "issue" but it would be nice if there was some attribute to indicate you wanted tho behavior on these types of methods.

eerhardt · 2021-07-27T20:49:01Z

I've updated this PR with the above plan (excluding leaving a LibraryBuild warning in). We believe the runtime check that is enabled when PublishTrimmed=true will be enough notification to the developer.

Please take a look at the new approach.

davidfowl · 2021-07-27T21:01:57Z

(excluding leaving a LibraryBuild warning in)

😢

eerhardt · 2021-07-27T21:08:18Z

(excluding leaving a LibraryBuild warning in)

😢

I thought you didn't want the warning.

davidfowl · 2021-07-27T21:12:56Z

Oh wait, I don't 😄

vitek-karas · 2021-07-28T08:47:15Z

Sounds good - if we figure out a way to check for this statically we can add a new warning in future releases.

agocke · 2021-07-28T17:00:44Z

I also noticed that there are now multiple source-generator-based dependency injection libraries, e.g.

https://github.com/giggio/sourceinject/
https://github.com/pakrym/jab

which promise AOT-friendly codegen for dependency injection. These libraries could be alternatives for users if we find more problems than expected.

jkotas · 2021-07-28T18:36:19Z

Yes, there will be more problems. The current pattern, even with this change, is not AOT friendly. Do we have a solution in mind that would make it AOT friendly?

I also noticed that there are now multiple source-generator-based dependency injection libraries

Yep, we will know that trimming and AOT friendliness is taking off once there is something like https://quarkus.io/ for .NET.

eerhardt · 2021-07-28T18:47:12Z

The current pattern, even with this change, is not AOT friendly. Do we have a solution in mind that would make it AOT friendly?

AFAIK, no. If we limited the generic parameter types to "ref" types, would that make it AOT friendly? If yes, maybe that's just what we instruct library and app developers who want to use this feature with AOT.

The open generics feature is used in a few pretty core places:

Options

runtime/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs

Lines 29 to 33 in cf52b7e

    
           services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptions<>), typeof(UnnamedOptionsManager<>))); 
        
           services.TryAdd(ServiceDescriptor.Scoped(typeof(IOptionsSnapshot<>), typeof(OptionsManager<>))); 
        
           services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>))); 
        
           services.TryAdd(ServiceDescriptor.Transient(typeof(IOptionsFactory<>), typeof(OptionsFactory<>))); 
        
           services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitorCache<>), typeof(OptionsCache<>)));

Logging

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggingServiceCollectionExtensions.cs

Line 42 in f5a7304

services.TryAdd(ServiceDescriptor.Singleton(typeof(ILogger<>), typeof(Logger<>)));

Http

runtime/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs

Lines 44 to 45 in f5a7304

    
           services.TryAdd(ServiceDescriptor.Transient(typeof(ITypedHttpClientFactory<>), typeof(DefaultTypedHttpClientFactory<>))); 
        
           services.TryAdd(ServiceDescriptor.Singleton(typeof(DefaultTypedHttpClientFactory<>.Cache), typeof(DefaultTypedHttpClientFactory<>.Cache)));

davidfowl · 2021-07-28T18:48:40Z

Yes, there will be more problems. The current pattern, even with this change, is not AOT friendly. Do we have a solution in mind that would make it AOT friendly?

What isn't AOT friendly?

agocke · 2021-07-28T18:50:53Z

I believe all uses of MakeGenericType with structs are fundamentally AOT unfriendly as they require runtime code generation.

davidfowl · 2021-07-28T18:52:41Z

I believe all uses of MakeGenericType with structs are fundamentally AOT unfriendly as they require runtime code generation.

I don't see why if these are reference types.

agocke · 2021-07-28T18:57:47Z

Is there any protection against registering structs?

davidfowl · 2021-07-28T19:10:11Z

For arbitrary open generics no.

davidfowl · 2021-07-28T19:29:56Z

I think the fear that this pattern isn't AOT friendly is a "perfect is the enemy of good" situation. I think this is pretty good already. I can see a future where we have a new method designed for open generics that would allow us to add more static analysis where possible.

eerhardt · 2021-07-30T15:09:58Z

Any feedback here? I believe this is ready to merge.

See * dotnet/runtime#55102 * dotnet/runtime#56475

eerhardt requested review from davidfowl, krwq, sbomer, maryamariyan and vitek-karas July 2, 2021 22:00

dotnet-issue-labeler bot added area-Extensions-DependencyInjection new-api-needs-documentation labels Jul 2, 2021

eerhardt added linkable-framework Issues associated with delivering a linker friendly framework and removed new-api-needs-documentation labels Jul 2, 2021

sbomer reviewed Jul 6, 2021

View reviewed changes

eerhardt mentioned this pull request Jul 28, 2021

WebAssembly should support handling runtimeconfig.json #38433

Closed

Make VerifyOpenGenericServiceTrimmability a full feature switch

ae283e5

karelz mentioned this pull request Jul 29, 2021

Massive long running tests on Linux #56477

Closed

eerhardt mentioned this pull request Jul 30, 2021

Update SDK to support new DI and Attribute feature switches dotnet/sdk#19432

Merged

davidfowl approved these changes Jul 30, 2021

View reviewed changes

LakshanF mentioned this pull request Jul 30, 2021

Adding warnings to make generic and static ctor #56628

Merged

eerhardt added 3 commits July 30, 2021 13:07

Merge branch 'main' into DIILink

5044fa8

Merge branch 'main' into DIILink

ccab974

Fix up merge

0efdbcc

eerhardt merged commit 65f04b9 into dotnet:main Jul 31, 2021

eerhardt deleted the DIILink branch July 31, 2021 14:46

eerhardt added a commit to dotnet/sdk that referenced this pull request Jul 31, 2021

Update SDK to support new DI and Attribute feature switches (#19432)

e10860e

See * dotnet/runtime#55102 * dotnet/runtime#56475

ghost locked as resolved and limited conversation to collaborators Aug 30, 2021

	internal const string RequiresUnreferencedCode = "Using generic types in DependencyInjection can lead to required code being trimmed. Ensure any generic Service types (e.g. 'IMyService<T>') have matching trimming annotations as the Implementation types implementing them (e.g. 'MyService<[DynamicallyAccessedMembers] T>).";
	internal const string RequiresUnreferencedCode = "Using generic types in DependencyInjection can lead to required code being trimmed. Ensure any generic Service types (e.g. 'IMyService<T>') have the same trimming annotations as the Implementation types implementing them (e.g. 'MyService<[DynamicallyAccessedMembers] T>).";

	Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +
	Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

	Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +
	Justification = "TryAddTransient has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

	Justification = "AddSingleton has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +
	Justification = "TryAddScoped has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

	Justification = "ServiceDescriptor's have RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +
	Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

	Justification = "ServiceDescriptor has RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +
	Justification = "ServiceDescriptor, TryAddTransient, and TryAddSingleton have RequiresUnreferencedCode because Service and Implementation types could be generic with mis-matching trimming annotations. " +

Resolve MakeGenericType ILLink warning in DependencyInjection #55102

Resolve MakeGenericType ILLink warning in DependencyInjection #55102

Conversation

eerhardt commented Jul 2, 2021 • edited Loading

dotnet-issue-labeler bot commented Jul 2, 2021

ghost commented Jul 2, 2021

ghost commented Jul 2, 2021

eerhardt commented Jul 3, 2021 • edited Loading

davidfowl commented Jul 3, 2021

eerhardt commented Jul 6, 2021

davidfowl commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Jul 6, 2021 • edited Loading

MichalStrehovsky commented Jul 8, 2021

davidfowl commented Jul 8, 2021

MichalStrehovsky commented Jul 8, 2021 • edited Loading

davidfowl commented Jul 8, 2021

jkotas commented Jul 8, 2021 • edited Loading

vitek-karas commented Jul 8, 2021

jkotas commented Jul 8, 2021

eerhardt commented Jul 8, 2021 • edited Loading

jkotas commented Jul 9, 2021

eerhardt commented Jul 9, 2021 • edited Loading

davidfowl commented Jul 9, 2021

eerhardt commented Jul 27, 2021

davidfowl commented Jul 27, 2021

eerhardt commented Jul 27, 2021

davidfowl commented Jul 27, 2021

vitek-karas commented Jul 28, 2021

agocke commented Jul 28, 2021

jkotas commented Jul 28, 2021

eerhardt commented Jul 28, 2021 • edited Loading

davidfowl commented Jul 28, 2021

agocke commented Jul 28, 2021 • edited Loading

davidfowl commented Jul 28, 2021

agocke commented Jul 28, 2021

davidfowl commented Jul 28, 2021

davidfowl commented Jul 28, 2021

eerhardt commented Jul 30, 2021

eerhardt commented Jul 2, 2021 •

edited

Loading

eerhardt commented Jul 3, 2021 •

edited

Loading

davidfowl commented Jul 6, 2021 •

edited

Loading

MichalStrehovsky commented Jul 8, 2021 •

edited

Loading

jkotas commented Jul 8, 2021 •

edited

Loading

eerhardt commented Jul 8, 2021 •

edited

Loading

eerhardt commented Jul 9, 2021 •

edited

Loading

eerhardt commented Jul 28, 2021 •

edited

Loading

agocke commented Jul 28, 2021 •

edited

Loading