Skip to content

[AOT] Fix Blazor AOT warnings #45473

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

Closed
1 task done
JamesNK opened this issue Dec 6, 2022 · 14 comments
Closed
1 task done

[AOT] Fix Blazor AOT warnings #45473

JamesNK opened this issue Dec 6, 2022 · 14 comments
Labels
area-blazor Includes: Blazor, Razor Components ✔️ Resolution: Duplicate Resolved as a duplicate of another issue NativeAOT Pillar: Technical Debt Status: Resolved

Comments

@JamesNK
Copy link
Member

JamesNK commented Dec 6, 2022

Is there an existing issue for this?

  • 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 JamesNK added area-runtime area-blazor Includes: Blazor, Razor Components labels Dec 6, 2022
@eerhardt
Copy link
Member

eerhardt commented Dec 6, 2022

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, dotnet/runtime#79286 for this issue.

@eerhardt
Copy link
Member

eerhardt commented Dec 6, 2022

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.

@mkArtakMSFT
Copy link
Contributor

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

@adityamandaleeka
Copy link
Member

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

@Nick-Stanton
Copy link
Member

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

@eerhardt
Copy link
Member

eerhardt commented Dec 9, 2022

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

I have opened #45527 to track this work specifically.

@mkArtakMSFT
Copy link
Contributor

mkArtakMSFT commented Jan 4, 2023

@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 #45795 and #45578, but we plan to tackle that in the future - not immediately.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 4, 2023

I enabled AOT analysis for all server-focused trimmed projects in #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 JamesNK changed the title [AOT] Fix hosting/HTTP/server/middleware/routing/auth AOT warnings [AOT] Fix Blazor AOT warnings Jan 4, 2023
@amcasey amcasey removed their assignment Jan 11, 2023
@amcasey
Copy link
Member

amcasey commented Jan 11, 2023

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

@mkArtakMSFT mkArtakMSFT removed this from the 8.0-preview1 milestone Feb 2, 2023
@mkArtakMSFT
Copy link
Contributor

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

@mkArtakMSFT mkArtakMSFT added the ✔️ Resolution: Duplicate Resolved as a duplicate of another issue label Feb 2, 2023
@ghost ghost added the Status: Resolved label Feb 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2023
@halter73
Copy link
Member

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

<!-- TODO: Address Native AOT analyzer warnings https://github.com/dotnet/aspnetcore/issues/45473 -->
<EnableAOTAnalyzer>false</EnableAOTAnalyzer>

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

@halter73 halter73 reopened this Aug 11, 2023
@halter73 halter73 removed ✔️ Resolution: Duplicate Resolved as a duplicate of another issue Status: Resolved labels Aug 11, 2023
@Nick-Stanton
Copy link
Member

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

<!-- TODO: Address Native AOT analyzer warnings https://github.com/dotnet/aspnetcore/issues/45473 -->
<EnableAOTAnalyzer>false</EnableAOTAnalyzer>

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

@dotnet dotnet unlocked this conversation Aug 13, 2023
@halter73 halter73 added this to the .NET 9 Planning milestone Aug 17, 2023
@ghost
Copy link

ghost commented Dec 14, 2023

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.

@mkArtakMSFT
Copy link
Contributor

Closing as we'll be addressing this as part of #51598

@mkArtakMSFT mkArtakMSFT closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2024
@mkArtakMSFT mkArtakMSFT added the ✔️ Resolution: Duplicate Resolved as a duplicate of another issue label Jan 9, 2024
@ghost ghost added the Status: Resolved label Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components ✔️ Resolution: Duplicate Resolved as a duplicate of another issue NativeAOT Pillar: Technical Debt Status: Resolved
Projects
None yet
Development

No branches or pull requests

8 participants