[AOT] Fix runtime warnings #45604

JamesNK · 2022-12-15T09:09:24Z

Addresses #45473

Run AOT analysis on all assemblies with IsTrimmable. Exceptions
- SpaProxy, it uses configuration, and isn't urgent for preview 1
- Blazor/components which is being addressed by [AOT] Add RequiresDynamicCode annotation to Microsoft.AspNetCore.Routing #45580
Suppresses all warnings from DI/hosting. These can be removed when Remove RequiresDynamicCode from Microsoft.Extensions.DependencyInjection runtime#79425 lands
Almost all make generic type/method usages can either be suppressed or warn for dynamic code. There is a place in hosting where a type could be a struct, but realistically I don't think anyone would ever use a struct there. I've made it throw a runtime error if RuntimeFeatures.IsDynamicCodeSupported is false.

src/Shared/ProblemDetails/ProblemDetailsJsonConverter.cs

...rotection/DataProtection/src/AuthenticatedEncryption/ManagedAuthenticatedEncryptorFactory.cs

src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs

halter73

This seems like an improvement to me. I do wonder if we should have stronger runtime checks for the container and identity stuff. Maybe we should be adding new : class generic constraints to some APIs.

Should we file a tracking issue to remove the pragmas once dotnet/runtime#79425 is merged?

src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs

src/Identity/Extensions.Core/src/IdentityBuilder.cs

eerhardt · 2022-12-15T21:11:22Z

src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs

+            if (containerType.IsValueType && !RuntimeFeature.IsDynamicCodeSupported)
+            {
+                throw new InvalidOperationException("ValueType startup container isn't supported with AOT.");


I don't think this is a valid approach to avoid the warning. This breaks the principle that "if you don't get any warnings, your app will work the same before and after publishing AOT".

If someone was using this functionality, and published for AOT, they wouldn't get any warnings and their app would work successfully at F5/dotnet run time. But when they went to publish the app, it would be broken.

Note this is different than the approach taken in dotnet/runtime#79425. The difference is that DI is using a "feature switch" which gets enabled when PublishAot=true is set in the .csproj. That approach means that the app will still fail at F5/dotnet run time, if the app was using the AOT-incompatible functionality.

I think the existing hosting APIs should just be marked RequiresDynamicCode instead of suppressing these warnings.

The same applies for the StartupLoader code/suppression.

Startup containers are very rarely used. And I doubt anyone has created a struct startup container. I don't want to annotate the hosting APIs as RequiresDynamicCode for this one weird edge case.

Good point about dotnet run/dotnet publish difference. Should we add a flag like dotnet/runtime#79425 does that ASP.NET Core can use to validate AOT compatibility? Or should there be one flag that is shared by libraries/applications?

I think the use case might be shared between many libraries: AOT always works except in unusual and rare situations. The pragmatic approach is to have a runtime error instead of always warning with annotations.

@vitek-karas @MichalStrehovsky @jkotas - thoughts on converting the DI feature switch in dotnet/runtime#79425 to be a general purpose “Verify AOT Compatibility” feature switch that can be used by other libraries? Like for instance, here in aspnet?

Another option is to always throw an exception for value types, AOT or not, if we think this functionality isn’t used with value types.

Yeah, that's an option. We can publish it as a breaking change.

IMO regardless of whether we use it here or not, I think a generalized runtime verify AOT compatibility flag has some use. I think it provides a pragmatic middle ground in gray situations where something is compatible 99% of the time. Right now we have to choose between suppress and pretend everything is ok, and warn for everyone.

general purpose “Verify AOT Compatibility” feature switch

This sounds like dotnet/runtime#39806

Having something like dotnet/runtime#39806 would be nice.

You're then left with a choice:

Just disable the problematic case always (in our case here, always throw and potentially add class constrains in right places) - breaking change

Rely on Add ability to disable reflection emit for testing runtime#39806 to basically trigger IsDynamicCodeSupported based on a feature switch for you. Downside is that such solution is not granular - and so it would not be possible to disable dynamic code everywhere, except this one case.

Introduce a new feature switch - which is what Remove RequiresDynamicCode from Microsoft.Extensions.DependencyInjection runtime#79425 is doing. Similar to the above, but it adds granularity. Downside is the added complexity and "yet another feature switch"

Mark the API as AOT incompatible via the attributes - and hopefully figure out a new API which can be used instead

I think which one to do is a function of the specific scenario. If it's something super rare, I would go with either outright breaking change or generic feature switch. If it's still rare, but "real", then a dedicated feature switch might be better.

src/Shared/ProblemDetails/HttpValidationProblemDetailsJsonConverter.cs

src/Http/Http.Abstractions/src/Routing/RouteValueDictionary.cs

src/Http/Routing/src/Patterns/RoutePatternFactory.cs

src/Http/Routing/src/Patterns/RoutePatternTransformer.cs

src/Http/Routing/src/RouteEndpointDataSource.cs

src/Identity/Core/src/IdentityBuilderExtensions.cs

src/Identity/Extensions.Core/src/IdentityBuilder.cs

JamesNK · 2022-12-16T14:11:35Z

I've reviewed all the PR comments and changes in this PR. I've simplified some annotations and removed some unnecessary ones.

Some items are still in discussion (startup container, attributes on problem details converters) and work todo (fix problem details errors serialization in converter). Everything else is resolved/improved.

src/Http/Routing/src/Builder/FallbackEndpointRouteBuilderExtensions.cs

src/Http/Http.Abstractions/src/Routing/RouteValueDictionary.cs

src/Shared/ProblemDetails/ProblemDetailsJsonConverter.cs

src/Identity/Extensions.Core/src/IdentityBuilder.cs

src/Shared/ProblemDetails/HttpValidationProblemDetailsJsonConverter.cs

src/Shared/EndpointMetadataPopulator.cs

src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs

src/Hosting/Hosting/src/Internal/StartupLoader.cs

src/Http/Http.Abstractions/src/Routing/RouteValueDictionary.cs

src/Shared/ProblemDetails/HttpValidationProblemDetailsJsonConverter.cs

src/Http/Routing/src/RouteEndpointDataSource.cs

src/Http/Routing/src/Builder/EndpointFilterExtensions.cs

davidfowl · 2022-12-21T01:21:20Z

src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

+    [JsonSerializable(typeof(JsonElement))]
+    [JsonSerializable(typeof(string))]
+    [JsonSerializable(typeof(decimal))]
+    [JsonSerializable(typeof(float))]
+    [JsonSerializable(typeof(double))]
+    [JsonSerializable(typeof(int))]
+    [JsonSerializable(typeof(long))]
+    [JsonSerializable(typeof(Guid))]
+    [JsonSerializable(typeof(Uri))]
+    [JsonSerializable(typeof(TimeSpan))]
+    [JsonSerializable(typeof(DateTime))]
+    [JsonSerializable(typeof(DateTimeOffset))]


We should undo this given our discussion @JamesNK

This is useful because it allows common type values as extension values to just work, e.g. adding a string value to problem details extensions.

Other than a small size cost when problem details aren't trimmed away, this doesn't hurt. Are you sure you want it removed?

I don't mind keeping it for now and removing it with @brunolins16's JSON changes, but I don't think this is generally useful.

Ok! I'd like to keep as is in this PR (rather than reverting to what it was, which doesn't work), and we can iterate on it during .NET 8 as we learn more.

davidfowl · 2022-12-21T01:22:23Z

src/Http/Http.Extensions/src/HttpResponseJsonExtensions.cs

@@ -18,6 +18,8 @@ public static partial class HttpResponseJsonExtensions
 {
    private const string RequiresUnreferencedCodeMessage = "JSON serialization and deserialization might require types that cannot be statically analyzed. " +


@brunolins16 this will all be undone with your changes right?

I don't believe, the unsafe API will still need to be attributed, however, RDF + MVC should not call them anymore.

src/Http/Routing/src/Builder/EndpointFilterExtensions.cs

JamesNK · 2023-01-03T23:08:04Z

@eerhardt @davidfowl All feedback applied. Are there any other changes, or can this get approved and merged 🙏

eerhardt

This looks like a great step forward! Thanks for all the great work here.

I just had a few nits, a question, and a follow-up recommendation for the future.

This LGTM. Let's get this in and make more progress.

src/Http/Http.Abstractions/src/Routing/RouteValueDictionary.cs

src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

src/Http/Routing/src/Matching/JumpTableBuilder.cs

eerhardt · 2023-01-04T00:22:05Z

src/Http/Routing/src/RouteEndpointDataSource.cs

+            "Map methods that configure a RequestDelegate don't use trimmer unsafe features.")]
+        [UnconditionalSuppressMessage("AOT", "IL3050",
+            Justification = "We surface a RequiresDynamicCode in the call to the Map methods adding route handlers this EndpointDataSource. Analysis is unable to infer this. " +
+            "Map methods that configure a RequestDelegate don't use AOT unsafe features.")]


Is there some way we can verify this?

One idea is to add a "Native AOT"-test that uses filter factories and ensures they work correctly.

If it is possible to verify this, we can do it in a follow-up issue/PR. It doesn't need to be addressed in this PR.

I'll create follow-up issues for areas to test with native AOT enabled. Any code that tests IsDynamicCodeSupported is a good candidate.

The plan is to undo this anyways. This should be suppression free after we execute #45524. There will be new API changes that remove the use of RequestDelegateFactory.

How does one add a "native AOT" test?

Edit: as discussed offline, we have yet to develop an approach, but we will probably model it on the one used by dotnet/runtime (i.e. create convenience wrappers for exes published as AOT).

src/Identity/Extensions.Core/src/IdentityBuilder.cs

eerhardt · 2023-01-04T00:31:17Z

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs

@@ -328,3 +352,10 @@ private Task DisplayRuntimeException(HttpContext context, Exception ex)
        return errorPage.ExecuteAsync(context);
    }
 }
+
+internal record ExceptionExtensionData(string Details, IHeaderDictionary Headers, string Path, string? Endpoint, RouteValueDictionary? RouteValues);


For my learning:

In dotnet/runtime we typically don't use record because of the bloat associated with it - C# generates members you don't use/need. Do we have guidelines in dotnet/aspnetcore when to use record and when not? Or don't we care as much?

I think we don't care as much. It's internal, so if we crack down on records in aspnetcore (we have them in other places) then it's simple enough to change.

For reference, this is how much IL is generated by this single record line: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBLANgH1kigBMACAUQTBgAcNsIA7KjGJgZ0aYBEBDDHwAUAAQCMABjI8YgvBzRkAkgAkYfEjCg9sYBsz5QAnmTUatCsuKkAFAQAtF1gPyUmJWhGxMMigEoQAK5sAGp8uIEwOnrchkauAcEwYREwHACUANwAsABQeXkiAExkiaHhkdH6THF5AN55AL4Fud5sUABmfDTKZprautW1uQ25jUA==

This record will translate to about 5kB of binary footprint in the native aot image. 80% of it is dynamically unreachable code, but it is impossible for the trimmer to prove that it is unreachable because a lot of it is behind potentially reachable interfaces.

Impressive! Looks like we should clean up record usage in aspnetcore.

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

davidfowl · 2023-01-04T06:18:21Z

src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs

+    [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "Values set on ProblemDetails.Extensions are supported by the default writer.")]
+    [UnconditionalSuppressMessage("AOT", "IL3050", Justification = "Values set on ProblemDetails.Extensions are supported by the default writer.")]
+    private ProblemDetails CreateProblemDetails(ErrorContext errorContext, HttpContext httpContext)


Why not let it warn until we have the real fix?

davidfowl · 2023-01-04T07:12:44Z

src/Identity/Extensions.Core/src/IdentityBuilder.cs

@@ -32,7 +38,14 @@ public IdentityBuilder(Type user, IServiceCollection services)
    /// <param name="role">The <see cref="Type"/> to use for the roles.</param>
    /// <param name="services">The <see cref="IServiceCollection"/> to attach to.</param>
    public IdentityBuilder(Type user, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type role, IServiceCollection services) : this(user, services)
-        => RoleType = role;
+    {
+        if (role.IsValueType)


Is this a breaking change?

RoleType and UserType are later used to generate types, all of which have a where T : class constraint.

Throwing here means the error is different, and it is thrown in a different place. But a value type as the user or role would always lead to an error.

Although the constructor is public, I imagine the vast majority of people who ever use IdentityBuilder create it via this extension method:

aspnetcore/src/Identity/Extensions.Core/src/IdentityServiceCollectionExtensions.cs

Line 22 in 09b0db0

public static IdentityBuilder AddIdentityCore<TUser>(this IServiceCollection services) where TUser : class

It enforces a class with its own constraint.

davidfowl

I'd like to see issues for:

The developer exception page WRT what we're doing JSON wise there
ProblemDetails and undoing what's there now
Undoing the DI changes when those other changes land
Doing API work in the Map* handlers to remove suppressions in the RouteEndpointDataSource

JamesNK · 2023-01-04T08:41:31Z

Follow-up issues:

[AOT] Improve suppressions on RouteEndpointDataSource #45863
[AOT] Unit testing features with Native AOT #45860
[AOT] ProblemDetails and Extension values #45862 (includes note about developer exception page)
[AOT] Improve suppressions on RouteEndpointDataSource #45863

JamesNK · 2023-01-04T08:49:07Z

Merged! Thanks everyone for your feedback.

amcasey · 2023-01-10T21:20:37Z

src/Middleware/Spa/SpaProxy/src/Microsoft.AspNetCore.SpaProxy.csproj

@@ -4,6 +4,7 @@
    <Description>Helpers for launching the SPA CLI proxy automatically when the application starts in ASP.NET MVC Core.</Description>
    <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
    <IsTrimmable>true</IsTrimmable>
+    <EnableAOTAnalyzer>false</EnableAOTAnalyzer>
    <!-- This is ok since this assembly is not referenced by any application but it is loaded as a hosting startup


Which line was this comment intended to refer to?

amcasey · 2023-01-10T21:36:17Z

src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

    [JsonSerializable(typeof(ProblemDetails))]
+    [JsonSerializable(typeof(JsonElement))]
+    [JsonSerializable(typeof(string))]


Is bool implied and/or specified elsewhere?

amcasey · 2023-01-10T21:39:49Z

src/Http/Routing/src/RouteEndpointDataSource.cs

+            "Map methods that configure a RequestDelegate don't use trimmer unsafe features.")]
+        [UnconditionalSuppressMessage("AOT", "IL3050",
+            Justification = "We surface a RequiresDynamicCode in the call to the Map methods adding route handlers this EndpointDataSource. Analysis is unable to infer this. " +
+            "Map methods that configure a RequestDelegate don't use AOT unsafe features.")]


How does one add a "native AOT" test?

Edit: as discussed offline, we have yet to develop an approach, but we will probably model it on the one used by dotnet/runtime (i.e. create convenience wrappers for exes published as AOT).

...introduced in dotnet#45604 now that dotnet/runtime#79425 is available.

JamesNK added the area-runtime label Dec 15, 2022

JamesNK requested review from davidfowl and eerhardt December 15, 2022 09:09

JamesNK requested review from Tratcher, BrennanConroy, a team, javiercn and halter73 as code owners December 15, 2022 09:09

JamesNK commented Dec 15, 2022

View reviewed changes

src/Shared/ProblemDetails/ProblemDetailsJsonConverter.cs Show resolved Hide resolved

vitek-karas reviewed Dec 15, 2022

View reviewed changes

...rotection/DataProtection/src/AuthenticatedEncryption/ManagedAuthenticatedEncryptorFactory.cs Show resolved Hide resolved

src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs Outdated Show resolved Hide resolved

halter73 approved these changes Dec 15, 2022

View reviewed changes

src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs Outdated Show resolved Hide resolved

src/Identity/Extensions.Core/src/IdentityBuilder.cs Outdated Show resolved Hide resolved

eerhardt reviewed Dec 15, 2022

View reviewed changes

halter73 reviewed Dec 15, 2022

View reviewed changes

src/Shared/ProblemDetails/HttpValidationProblemDetailsJsonConverter.cs Outdated Show resolved Hide resolved

eerhardt reviewed Dec 15, 2022

View reviewed changes

build-analysis bot mentioned this pull request Dec 16, 2022

Request failed "500 Internal Server Error" dotnet/arcade#11737

Open

5 tasks

JamesNK commented Dec 16, 2022

View reviewed changes

src/Http/Routing/src/Builder/FallbackEndpointRouteBuilderExtensions.cs Show resolved Hide resolved

eerhardt reviewed Dec 16, 2022

View reviewed changes

JamesNK mentioned this pull request Dec 17, 2022

[AOT] Fixing the problems with ProblemDetails #45646

Merged

eerhardt reviewed Dec 20, 2022

View reviewed changes

davidfowl reviewed Dec 21, 2022

View reviewed changes

src/Http/Routing/src/Builder/EndpointFilterExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Dec 21, 2022

View reviewed changes

src/Http/Routing/src/Builder/EndpointFilterExtensions.cs Outdated Show resolved Hide resolved

JamesNK force-pushed the jamesnk/Aot branch from 9175347 to aced23d Compare December 21, 2022 02:48

Nick-Stanton mentioned this pull request Dec 21, 2022

[AOT] Add RequiresDynamicCode annotation to Microsoft.AspNetCore.Routing #45580

Merged

Nick-Stanton mentioned this pull request Dec 28, 2022

[AOT] Explore Alternatives to MakeGenericType/MakeGenericMethod For Web UI #45795

Closed

6 tasks

amcasey added 2 commits January 4, 2023 07:11

Enable AOT warnings

a4fdc61

Suppress AOT warning

4ccee5b

JamesNK added 2 commits January 4, 2023 07:14

PR feedback

3b27d80

Fix

dcd762e

JamesNK force-pushed the jamesnk/Aot branch from f0f51ce to dcd762e Compare January 3, 2023 23:14

JamesNK requested a review from a team as a code owner January 3, 2023 23:14

eerhardt approved these changes Jan 4, 2023

View reviewed changes

JamesNK and others added 4 commits January 4, 2023 09:43

Update src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

c755e78

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

PR feedback

f6ff8fe

PR feedback

51ca4ec

PR feedback

09e0fdf

davidfowl reviewed Jan 4, 2023

View reviewed changes

JamesNK mentioned this pull request Jan 4, 2023

Reduce record usage in published aspnetcore projects #45859

Closed

2 tasks

davidfowl reviewed Jan 4, 2023

View reviewed changes

davidfowl approved these changes Jan 4, 2023

View reviewed changes

JamesNK merged commit 38cdcfd into main Jan 4, 2023

JamesNK deleted the jamesnk/Aot branch January 4, 2023 08:46

ghost added this to the 8.0-preview1 milestone Jan 4, 2023

This was referenced Jan 4, 2023

[AOT] Fix Blazor AOT warnings #45473

Closed

[AOT] Fix hosting/HTTP/server/middleware/routing/auth AOT warnings #45887

Closed

[AOT] Publish gRPC app as Native AOT with no warnings #45888

Closed

amcasey mentioned this pull request Jan 11, 2023

[AOT] Remove AOT warning suppressions #46015

Merged

amcasey reviewed Jan 11, 2023

View reviewed changes

amcasey mentioned this pull request Jan 11, 2023

[AOT] Clean up AOT warnings in the SPA Proxy project #46036

Closed

1 task

MichalStrehovsky mentioned this pull request Jan 17, 2023

Make TypeComponentsCache trimmable dotnet/runtime#80726

Merged

amcasey added a commit to amcasey/aspnetcore that referenced this pull request Jan 17, 2023

Remove AOT warning suppressions

d8c97de

...introduced in dotnet#45604 now that dotnet/runtime#79425 is available.

MichalStrehovsky mentioned this pull request Jun 5, 2023

Do not use ValueTuple in SynchronizationContext dotnet/runtime#87120

Merged

amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 6, 2023

		@@ -18,6 +18,8 @@ public static partial class HttpResponseJsonExtensions
		{
		private const string RequiresUnreferencedCodeMessage = "JSON serialization and deserialization might require types that cannot be statically analyzed. " +

[AOT] Fix runtime warnings #45604

[AOT] Fix runtime warnings #45604

Conversation

JamesNK commented Dec 15, 2022

halter73 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Dec 15, 2022 • edited Loading

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

JamesNK commented Dec 16, 2022

davidfowl Dec 21, 2022 • edited Loading

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

JamesNK commented Jan 3, 2023

eerhardt left a 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

amcasey Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 4, 2023 • edited Loading

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

JamesNK Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

JamesNK commented Jan 4, 2023 • edited Loading

JamesNK commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

JamesNK Dec 15, 2022 •

edited

Loading

davidfowl Dec 21, 2022 •

edited

Loading

amcasey Jan 10, 2023 •

edited

Loading

jkotas Jan 4, 2023 •

edited

Loading

JamesNK Jan 4, 2023 •

edited

Loading

JamesNK commented Jan 4, 2023 •

edited

Loading

amcasey Jan 10, 2023 •

edited

Loading