Skip to content
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

Remove RequiresDynamicCode from Microsoft.Extensions.DependencyInjection #79286

Closed
eerhardt opened this issue Dec 6, 2022 · 16 comments · Fixed by #79425
Closed

Remove RequiresDynamicCode from Microsoft.Extensions.DependencyInjection #79286

eerhardt opened this issue Dec 6, 2022 · 16 comments · Fixed by #79425

Comments

@eerhardt
Copy link
Member

eerhardt commented Dec 6, 2022

In .NET 7, with #73829, we annotated Microsoft.Extensions.DependencyInjection as RequiresDynamicCode because it supports enumerable and generic servcies with ValueTypes. When using DI with ValuesTypes, the array and generic code might not be available.

In .NET 8 we need a better approach in order to support applications that use DependencyInjection and publish for NativeAOT. DependencyInjection needs to have reliable behavior before and after publishing for NativeAOT. The application can't work successfully at development-time, but then fail after publishing with PublishAot=true.

Problem

The good thing, there are only limited scenarios where DependencyInjection needs to use dynamic code. And these scenarios are not used very often in real-world applications.

  1. Using "Enumerable" services with ValueTypes. The code currently creates an Array for the enumerable services. The issue is with ValueTypes, there is no guarantee that the runtime has code for an Array of the ValueType.
  2. Using "Open Generic" services with ValuesTypes. The code currently calls MakeGenericType, potentially passing in a ValueType. There is a similar issue here that there is no guarantee that the NativeAOT compiler generated code for the generic type for the ValueType.

Proposal

The proposed approach is similar to how we solved trim-compatibility with DependencyInjection in #55102.

We will resolve the 2 NativeAOT warnings above by adding a runtime check that is behind a new AppContext switch Microsoft.Extensions.DependencyInjection.VerifyAotCompatibility. The runtime check ensures the Types being used with Enumerable and Open Generic services are only Reference Types. If an application tries to create an Enumerable or Closed Generic service of a ValueType, DependencyInjection will throw an exception. The check is enabled by default when PublishAot=true.

The recommendation for applications that want to publish for NativeAOT is to put <PublishAot>true</PublishAot> in their .csproj. That way they get the appropriate analyzers and runtime behaviors during development-time, i.e. F5 in VS or dotnet run from the command line. Applications using the incompatible features above will be broken at development time if the app is intended to be published for AOT. This will give the app developer indication their application won't work after publishing for NativeAOT as well.

cc @davidfowl @halter73 @JamesNK @MichalStrehovsky @agocke @vitek-karas @jkotas

@eerhardt eerhardt added this to the 8.0.0 milestone Dec 6, 2022
@eerhardt eerhardt self-assigned this Dec 6, 2022
@ghost
Copy link

ghost commented Dec 6, 2022

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

In .NET 7, with #73829, we annotated Microsoft.Extensions.DependencyInjection as RequiresDynamicCode because it supports enumerable and generic servcies with ValueTypes. When using DI with ValuesTypes, the array and generic code might not be available.

In .NET 8 we need a better approach in order to support applications that use DependencyInjection and publish for NativeAOT. DependencyInjection needs to have reliable behavior before and after publishing for NativeAOT. The application can't work successfully at development-time, but then fail after publishing with PublishAot=true.

Problem

The good thing, there are only limited scenarios where DependencyInjection needs to use dynamic code. And these scenarios are not used very often in real-world applications.

  1. Using "Enumerable" services with ValueTypes. The code currently creates an Array for the enumerable services. The issue is with ValueTypes, there is no guarantee that the runtime has code for an Array of the ValueType.
  2. Using "Open Generic" services with ValuesTypes. The code currently calls MakeGenericType, potentially passing in a ValueType. There is a similar issue here that there is no guarantee that the NativeAOT compiler generated code for the generic type for the ValueType.

Proposal

The proposed approach is similar to how we solved trim-compatibility with DependencyInjection in #55102.

We will resolve the 2 NativeAOT warnings above by adding a runtime check that is behind a new AppContext switch 'Microsoft.Extensions.DependencyInjection.VerifyAotCompatibility'. The runtime check ensures the Types being used with Enumerable and Open Generic services are only Reference Types. If an application tries to create an Enumerable or Closed Generic service of a ValueType, DependencyInjection will throw an exception. The check is enabled by default when PublishAot=true.

The recommendation for applications that want to publish for NativeAOT is to put <PublishAot>true</PublishAot> in their .csproj. That way they get the appropriate analyzers and runtime behaviors during development-time, i.e. F5 in VS or dotnet run from the command line. Applications using the incompatible features above will be broken at development time if the app is intended to be published for AOT. This will give the app developer indication their application won't work after publishing for NativeAOT as well.

cc @davidfowl @halter73 @JamesNK @MichalStrehovsky @agocke @vitek-karas @jkotas

Author: eerhardt
Assignees: eerhardt
Labels:

area-Extensions-DependencyInjection

Milestone: 8.0.0

@marek-safar
Copy link
Contributor

The check is enabled by default when PublishAot=true.

Please don't break other AOT scenarios

@davidfowl
Copy link
Member

We're trying to make AOT scenarios work, not break them 😄

@eerhardt
Copy link
Member Author

eerhardt commented Dec 7, 2022

Please don't break other AOT scenarios

I definitely don't intend to break existing AOT scenarios. For iOS and Android, I don't believe they are currently using the <PublishAot>true</PublishAot> MSBuild property. @jonathanpeppers @rolfbjarne - can you confirm?

But if they are, and need this functionality turned off, they will be able to disable this property by default, even when PublishAot=true.

@MichalStrehovsky
Copy link
Member

We don't set PublishAot outside of Native AOT. It has other SDK behaviors attached to it that would not be applicable outside Native AOT (e.g. it gates the RequiresDynamicCode analyzer). There was a discussion around using this elsewhere in dotnet/sdk#25392, but it didn't come through.

If we ever want to have this property mean something else than Native AOT, adjusting this feature flag would be just one of the many other things in the SDK that need adjusting.

Right now PublishAot means "publish with the limitations (and optimization opportunities) that apply to Native AOT" and it doesn't have a mapping to a mode of the Mono runtime. It could in the future.

@jonathanpeppers
Copy link
Member

For platforms running Mono, $(RunAOTCompilation) is used instead:

<Import Condition="'$(TargetsCurrent)' == 'true' and '$(RunAOTCompilation)' == 'true' and ('$(UsingBrowserRuntimeWorkload)' == 'true' or '$(UsingMobileWorkload)' == 'true')" Project="Sdk.props" Sdk="Microsoft.NET.Runtime.MonoAOTCompiler.Task" />

I don't know of anything Mono-related that uses $(PublishAot) yet. @radical might know for sure.

@radical
Copy link
Member

radical commented Dec 8, 2022

I don't know of anything Mono-related that uses $(PublishAot) yet.

Yep, that's my understanding too.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 9, 2022
@rolfbjarne
Copy link
Member

Right now PublishAot means "publish with the limitations (and optimization opportunities) that apply to Native AOT" and it doesn't have a mapping to a mode of the Mono runtime. It could in the future.

For iOS, tvOS and partially Mac Catalyst, AOT is on by default, and tweaked/turned off (kind of) with UseInterpreter instead.

We will resolve the 2 NativeAOT warnings above by adding a runtime check that is behind a new AppContext switch Microsoft.Extensions.DependencyInjection.VerifyAotCompatibility.

Might I suggest Microsoft.Extensions.DependencyInjection.VerifyNativeAotCompatibility instead? Since this seems limited to NativeAOT in particular.

@jkotas
Copy link
Member

jkotas commented Dec 12, 2022

Since this seems limited to NativeAOT in particular.

The impact is not limited to native AOT. The patterns flagged by these checks are going to be slow on platforms without a JIT, like iOS, etc.

@mitchdenny
Copy link
Member

@eerhardt as a datapoint I've used dependency injection as a simple extensibility mechanism where I'm injecting an IEnumerable into a service where there has been multiple T's also injected into the DI infra:

Example:

Registration: https://github.com/Azure/azure-sdk-tools/blob/28c8db9782063964505d875bbc9d161baf930ab1/tools/pipeline-witness/Azure.Sdk.Tools.PipelineWitness/Startup.cs#L71
Injection Site: https://github.com/Azure/azure-sdk-tools/blob/28c8db9782063964505d875bbc9d161baf930ab1/tools/pipeline-witness/Azure.Sdk.Tools.PipelineWitness/Services/FailureAnalysis/FailureAnalyzer.cs#L13

I'd want to make sure IEnumerable injection for reference types would still work. From your issue description it wasn't entirely clear whether you saw this as a valid scenario.

@mitchdenny
Copy link
Member

@davidfowl has informed me that IEnumerable where T is a reference type would be supported.

@eerhardt
Copy link
Member Author

Correct - reference types are still 100% supported. From the proposal above:

The runtime check ensures the Types being used with Enumerable and Open Generic services are only Reference Types. If an application tries to create an Enumerable or Closed Generic service of a ValueType, DependencyInjection will throw an exception.

eerhardt added a commit to eerhardt/runtime that referenced this issue Jan 9, 2023
We need a better approach in order to support applications that use DependencyInjection and publish for NativeAOT. DependencyInjection needs to have reliable behavior before and after publishing for NativeAOT. The application can't work successfully at development-time, but then fail after publishing with PublishAot=true.

We will resolve the 2 NativeAOT warnings above by adding a runtime check that is behind a new AppContext switch Microsoft.Extensions.DependencyInjection.VerifyAotCompatibility. The runtime check ensures the Types being used with Enumerable and Open Generic services are only Reference Types. If an application tries to create an Enumerable or Closed Generic service of a ValueType, DependencyInjection will throw an exception. The check is enabled by default when PublishAot=true.

Fix dotnet#79286
@eerhardt
Copy link
Member Author

eerhardt commented Jan 9, 2023

Might I suggest Microsoft.Extensions.DependencyInjection.VerifyNativeAotCompatibility instead? Since this seems limited to NativeAOT in particular.

I have changed the proposal to be based off the DynamicCodeSupport feature switch instead of adding one specific for DependencyInjection.

eerhardt added a commit that referenced this issue Jan 10, 2023
…ion (#79425)

* Remove RequiresDynamicCode from Microsoft.Extensions.DependencyInjection

We need a better approach in order to support applications that use DependencyInjection and publish for NativeAOT. DependencyInjection needs to have reliable behavior before and after publishing for NativeAOT. The application can't work successfully at development-time, but then fail after publishing with PublishAot=true.

We will resolve the 2 NativeAOT warnings above by adding a runtime check that is behind the new AppContext switch added in #80246 (`System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported`). The runtime check ensures the Types being used with Enumerable and Open Generic services are only Reference Types. If an application tries to create an Enumerable or Closed Generic service of a ValueType, DependencyInjection will throw an exception. The check is enabled by default when PublishAot=true.

Fix #79286
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 10, 2023
@YAJeff
Copy link

YAJeff commented Jan 30, 2023

I've been considering contributing to the DI library. It seems to me a like a code generator would be a good solution for this. It may require people modify their code in how they define their DI. But I'm curious, is anyone currently considering/working on this?

@eerhardt
Copy link
Member Author

It seems to me a like a code generator would be a good solution for this. It may require people modify their code in how they define their DI. But I'm curious, is anyone currently considering/working on this?

See https://github.com/pakrym/jab

@davidfowl
Copy link
Member

I've been considering contributing to the DI library. It seems to me a like a code generator would be a good solution for this. It may require people modify their code in how they define their DI. But I'm curious, is anyone currently considering/working on this?

With the current pattern, a code generator doesn't work well because you can't generate "efficient" code unless you have the full closure of dependencies at compile time. That isn't possible today because the service collection is a runtime list of dependencies which makes this sort of code generation hard.

We're don't have plans to re-design how dependencies are registered (like jab does), at least not as part of the core container implementation.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants