Remove RequiresDynamicCode from Microsoft.Extensions.DependencyInjection

[eerhardt]

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

[dotnet-issue-labeler]

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]

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

Issue Details

---

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 #79286

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

[marek-safar]

Is this really the only condition when the generic type cannot be created by NativeAOT?

[eerhardt]

@MichalStrehovsky can correct me, but as far as I know, yes. The reasoning is that generic types of "reference types" all share the same native code. So creating generic types with ref type arguments works successfully.

Similar checks were added here:

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs

Lines 1366 to 1373 in d76ddf3

	if (!RuntimeFeature.IsDynamicCodeSupported && elemType.IsValueType)
	{
	return new EnumeratePropertyFetch(type);
	}
	#endif
	Type instantiatedTypedPropertyFetcher = typeof(EnumeratePropertyFetch<>)
	.GetTypeInfo().MakeGenericType(elemType);
	return (PropertyFetch)Activator.CreateInstance(instantiatedTypedPropertyFetcher, type)!;

[MichalStrehovsky]

Yup - when the compiler sees a generic type being a target of reflection, it will create the canonical reference type instantiation for it. It is a heuristic that makes a lot of things work. We decided to make the heuristic contractual because it allows a lot of MakeGeneric to "just work", with the exact same perf characteristics as if it was statically predicted.

The only possible drawback of the heuristic is more code, but this only kicks in for targets of reflection and with <TrimMode>full</TrimMode> there's few of those.

[JamesNK]

What about if typeArg isn't a value type itself but is a generic type with a value type argument?

For example:

var typeArg = typeof(List<int>);
var newType = typeof(List<>).MakeGenericType(typeArg); // List<List<int>>

I'm guessing this is ok because the compiler can see typeof(List<int>) constant, and the MakeGenericType call is for a list of lists, which is still just a list of references and can be shared.

It would be good if an expert confirmed and we added a test for this if one doesn't already exist.

[MichalStrehovsky]

I'm guessing this is ok because the compiler can see typeof(List<int>) constant, and the MakeGenericType call is for a list of lists, which is still just a list of references and can be shared.

Yes, that's correct.

[vitek-karas]

I agree that it's not a solution we should ask everybody to use - it's just not practical that way.
Personally I think we'll have to introduce some kind of annotation, similar to the trimmer annotations, if this kind of pattern shows up a lot.

Right now I think the one big feature switch for all value types is too big of a hammer, but maybe it will turn out to be useful.

[MichalStrehovsky]

We do the feature switch with JIT time emulation when we expect the situation is uncommon and we want to suppress static warnings.

I think MakeGenericType with valuetypes will always be a lot more common than DI with valuetypes.

Even if one goes through the MakeGenericType usage in the framework - most have the valuetype problem, unless we already gave them some extra AOT attention (like MakeGenericType in EventSource or DiagnosticSource that is now AOT safe).

If an API call is more likely to be an issue for AOT than a non-issue, a static warning is better than JIT time emulation.

[marek-safar]

I agree that the API is "problematic" for NativeAOT but I think an alternative fix where we change MakeGenericType to give better context anyone when the type cannot be constucted, for example like

You called MakeGenericType with value type argument of Foo under AOT setup, consider doing XYZ to resolve this problem.

would have bigger impact as it would help everyone that believes that their MakeGenericType call with value types will be used rarely and suppressing the warning is fine. As I understand the problem, it would also fix the problem in this PR where we are too aggressive with value type check because if the generic instance with value type is available/discovered elsewhere in the program MakeGenericType with value type argument should work or not?

[vitek-karas]

As I understand the problem, it would also fix the problem in this PR where we are too aggressive with value type check because if the generic instance with value type is available/discovered elsewhere in the program MakeGenericType with value type argument should work or not?

I'm not sure I would call this a "fix" - unless such dependency is intentional it would make for a very fragile setup - somebody changes some part of the app, and a completely unrelated part suddenly breaks because the generic instantiation it was using is no longer statically referenced. Honestly even now relying on this behavior is problematic to day the least - I would much rather we have a declarative way of doing this, and one which is verifiable.

[MichalStrehovsky]

As I understand the problem, it would also fix the problem in this PR where we are too aggressive with value type check because if the generic instance with value type is available/discovered elsewhere in the program MakeGenericType with value type argument should work or not?

It is true that if the generic code exists in the app, the MakeGenericType would succeed with native AOT. But this behavior cannot be emulated with the JIT. We would have to allow this with any type on F5 debug. So we would be back to the behavior where things work with F5 debug, but fail when one is ready to ship. This behavior is not desirable for trimming, and it's not desirable for AOT either.

The mantra for AOT is the same as with trimming: the app works the same between F5 debug and PublishAot, or there's a warning.

We want to get rid of a warning here because 95+% of users won't be using DI with value types and a warning would be just noise.

[marek-safar]

Are we backing off from build check and moving back to runtime exceptions for AOT in general or is this somehow a unique case that nobody will mimic?

[eerhardt]

Are we backing off from build check

No, definitely not. The build/publish time warnings are still the main tool we have for telling app devs what will and won't work with NativeAOT.

is this somehow a unique case that nobody will mimic?

This probably won't be the only case where we take this approach, but I expect these situations to be extremely rare. (Note, as called out in #79286, we took this same approach to resolve trimming warnings in this library as well.)

The situation here is that:

1. DependencyInjection is a very popular library - all of ASP.NET is built around it.
2. DependencyInjection has 2 rarely used, edge-case features that aren't compatible with NativeAOT.
3. As it exists today, and what we did for .NET 7, the whole library/feature needs to be marked as RequiresDynamicCode because of these 2 rarely used features.

That isn't an acceptable solution going forward because the library works for the main-line scenarios and we want to enable ASP.NET applications to work with NativeAOT without warnings. Always warning from this library would "numb" developers to AOT warnings, since their app works even though it warns.

moving back to runtime exceptions for AOT in general

Note a big difference here is that VerifyAotCompatibility will be enabled even at F5 and dotnet run time. Which means the developer will know that their app is broken while developing it. They won't need to publish the app for AOT to see the behavior difference.

[davidfowl]

I would refer to this as the "pragmatic approach".

[marek-safar]

I would refer to this as the "pragmatic approach".

That's fair, but should not we use this approach for anyone using MakeGenericType? It seems like the current approach of triggering cascading warning everywhere is too much when it can actually only happen for value-type arguments.

[halter73]

I think waiting until runtime for all MakeGenericType/MakeGenericMethod errors in the hope that it won't get used with value types would probably be going too far, because those methods are commonly used with value types. For example, ASP.NET Cores calls MakeGenericMethod with route handlers' parameter types which could easily be value types when IsDynamicCodeSupported, and that's just one of many similar usages.

This approach does have the downside of not even firing at runtime in development/testing if the right/wrong code paths don't get hit. But in the DI case, I think open generic services with value type arguments are exceedingly rare relative to the number of people using DI, so it's worth accepting this downside to avoid all the false positives.

[davidfowl]

This code isn't AOT safe though. I think in .NET 8, once we have the source generator, we'd get rid of these checks in the codebase.

This approach does have the downside of not even firing at runtime in development/testing if the right/wrong code paths don't get hit. But in the DI case, I think open generic services with value type arguments are exceedingly rare relative to the number of people using DI, so it's worth accepting this downside to avoid all the false positives.

💯

[MichalStrehovsky]

LGTM, but not an expert on DI.

[vitek-karas]

The logic around MakeGenericType looks good to me - but just like Michal - I'm no expert on the internals of DI.

[eerhardt]

This PR is ready for review/approval. I have updated the top comment with the latest approach of using the IsDynamicCodeSupported feature switch instead of introducing a new DI specific feature switch here.