aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

[AOT] Fix Blazor AOT warnings

Open JamesNK opened this issue 1 year ago • 8 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Hosting, HTTP, servers, middleware, routing, and auth were annotated for trimming in .NET 7. Projects with trimming are at https://github.com/dotnet/aspnetcore/blob/82daf67c4754ae5b1beac13d14c1b2e49b1351e8/eng/TrimmableProjects.props.

AOT adds new analysis warnings. I tested enabling AOT analysis for all trimmable projects and got 80ish warnings.

image

These warnings seem to fall into a bunch of categories:

  • Reflection with MakeGenericType and MakeGenericMethod. These methods can't be used with value types in AOT. Fortunately, most usages are with reference types, so they can be suppressed.
  • Code that has been made safe during trimming effort but is only suppressed for trim warnings. Need to be suppressed for AOT warnings.
  • Similar to the previous category, there is code identified as never safe (e.g. reflection-based minimal APIs). The public APIs need to be marked as unsafe. This is required so a warning is only raised if the public API is used.
  • Warnings from dependency injection (mostly hosting) and System.Text.Json. Some work with Microsoft.Extensions.DependenceInjection team will be required to develop a good solution for DI safety.
  • Middleware uses a lot of reflection. There is work to have source generation replace reflection here. @davidfowl might be working on this. Talk with him around middleware pipeline.

Describe the solution you'd like

Enable AOT analysis for all the projects in https://github.com/dotnet/aspnetcore/blob/82daf67c4754ae5b1beac13d14c1b2e49b1351e8/eng/TrimmableProjects.props. Most of these projects are in @adityamandaleeka space, but there are a couple of components projects for @mkArtakMSFT. Can place <EnableAOTAnalyzer>false</EnableAOTAnalyzer> to exclude some projects temporarily.

Additional context

No response

JamesNK avatar Dec 06 '22 03:12 JamesNK

Warnings from dependency injection (mostly hosting) Some work with Microsoft.Extensions.DependenceInjection team will be required to develop a good solution for DI safety.

I have opened, and plan on fixing this week, https://github.com/dotnet/runtime/issues/79286 for this issue.

eerhardt avatar Dec 06 '22 17:12 eerhardt

Reflection with MakeGenericType and MakeGenericMethod. These methods can't be used with value types in AOT. Fortunately, most usages are with reference types, so they can be suppressed.

I would not suppress warnings without some mechanism for ensuring they are ONLY ever used with reference types (e.g. Debug.Assert for internal code, runtime checks for public APIs, etc). Even if it is uncommon or unlikely for the call to MakeGenericType with a ValueType, if it is possible for a customer to do it - the warning needs to be addressed. We can't suppress warnings unless we are sure it is impossible for the code to fail at runtime.

eerhardt avatar Dec 06 '22 17:12 eerhardt

@Nick-Stanton can you please research this and summarize what are the changes required from our side (Blazor) to achieve what's needed? Thanks!

mkArtakMSFT avatar Dec 06 '22 17:12 mkArtakMSFT

@amcasey will look at the ones in the runtime area.

adityamandaleeka avatar Dec 07 '22 01:12 adityamandaleeka

Warnings from dependency injection (mostly hosting) and System.Text.Json.

Web UI's biggest pain point here is our JSON serialization and deserialization. To become more AOT-friendly, we need to do some work implementing System.Text.Json source generation into these scenarios. Getting some added performance improvements isn't too bad of a side effect either!

Reflection with MakeGenericType and MakeGenericMethod. These methods can't be used with value types in AOT.

Web UI has a small handful of these. I agree with @eerhardt's point of adding some insurance, but otherwise they all looked suppressible.

There were a couple warnings related to DI and following the completion of dotnet/runtime#79286 we should follow up and make sure that they are resolved. I have tracked the projects that we need to change below.

CC @mkArtakMSFT

Projects for Web UI to resolve

Both JSON and Reflection

  • Microsoft.AspNetCore.Components

JSON

  • Microsoft.AspNetCore.Components.Web
  • Microsoft.AspNetCore.Components.WebAssembly
  • Microsoft.AspNetCore.Components.WebAssembly.Authentication
  • Microsoft.JSInterop

Reflection

  • Microsoft.AspNetCore.Routing
  • Microsoft.AspNetCore.Routing.Abstractions

Nick-Stanton avatar Dec 08 '22 00:12 Nick-Stanton

Web UI's biggest pain point here is our JSON serialization and deserialization.

I have opened #45527 to track this work specifically.

eerhardt avatar Dec 09 '22 23:12 eerhardt

@eerhardt @Nick-Stanton has completed the changes which are straight forward in the Blazor areas. The remaining work for our team is tracked as part of https://github.com/dotnet/aspnetcore/issues/45795 and https://github.com/dotnet/aspnetcore/issues/45578, but we plan to tackle that in the future - not immediately.

mkArtakMSFT avatar Jan 04 '23 17:01 mkArtakMSFT

I enabled AOT analysis for all server-focused trimmed projects in https://github.com/dotnet/aspnetcore/pull/45604

Warnings were addressed for ASP.NET Core server-focused projects (hosting, servers, middleware, etc). AOT analysis was disabled for Blazor projects. It should be re-enabled for Blazor projects when looking at this issue.

Disabled AOT analysis projects:

  • Microsoft.AspNetCore.Components
  • Microsoft.AspNetCore.Components.Web
  • Microsoft.JSInterop.WebAssembly
  • Microsoft.AspNetCore.Components.WebAssembly.Authentication
  • Microsoft.AspNetCore.Components.WebAssembly

JamesNK avatar Jan 04 '23 23:01 JamesNK

I've split out and self-assigned the SPA Proxy work so the remaining projects to be addressed are all Blazor.

amcasey avatar Jan 11 '23 19:01 amcasey

Closing this as we have separate issues tracking the remaining work.

mkArtakMSFT avatar Feb 02 '23 17:02 mkArtakMSFT

It doesn't look like the remaining work was addressed.

https://github.com/dotnet/aspnetcore/blob/ccde0431f28274eeed2ac6758e79da61283c9ca3/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj#L11-L12

If there's a separate issue tracking this, we should update the comment in the csproj.

halter73 avatar Aug 11 '23 22:08 halter73

It doesn't look like the remaining work was addressed.

https://github.com/dotnet/aspnetcore/blob/ccde0431f28274eeed2ac6758e79da61283c9ca3/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj#L11-L12

If there's a separate issue tracking this, we should update the comment in the csproj.

It's been a while, but I believe #45795 and #45578 cover the remaining work for Blazor

Nick-Stanton avatar Aug 12 '23 01:08 Nick-Stanton

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost avatar Dec 14 '23 17:12 ghost

Closing as we'll be addressing this as part of https://github.com/dotnet/aspnetcore/issues/51598

mkArtakMSFT avatar Jan 09 '24 17:01 mkArtakMSFT