[AOT] Add RequiresDynamicCode annotation to Microsoft.AspNetCore.Routing #45580
Conversation
...SInterop/Microsoft.JSInterop/src/Infrastructure/DotNetObjectReferenceJsonConverterFactory.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/TaskGenericsUtil.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/TaskGenericsUtil.cs
Outdated
Show resolved
Hide resolved
| [SuppressMessage( | ||
| "AOT", | ||
| "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", | ||
| Justification = "The referenced methods are AOT safe with reference types.")] |
There was a problem hiding this comment.
I don't think this suppression is valid. We won't ever call this class with a property of type int, double, or Guid, or any other Value Type?
Instead, if we want to make this AOT-compatible, we will need to do something like this:
Basically - check if !RuntimeFeature.IsDynamicCodeSupported. When it isn't, and one of the types is a ValueType, then fallback to "slower" reflection based property setting that boxes.
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
| @@ -375,6 +375,9 @@ private static (MethodInfo, Type[]) GetCachedMethodInfo(AssemblyKey assemblyKey, | |||
| "ReflectionAnalysis", | |||
| "IL2060:MakeGenericMethod", | |||
| Justification = "https://github.com/mono/linker/issues/1727")] | |||
| [SuppressMessage("AOT", | |||
| "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", | |||
| Justification = "Methods being referenced do not have DynamicallyAccessedMembers.")] | |||
There was a problem hiding this comment.
- This isn't a valid justification for suppressing this warning.
- I don't believe this warning can be safely suppressed. From looking at the only callsite, this is used to convert a
ValueTask<T>to aTask<T>. If theTis a ValueType, this code isn't guaranteed to work in Native AOT, sinceCreateValueTaskConverter<T>might not be generated ahead of time for that specific ValueType.
There was a problem hiding this comment.
Further, how important is it for Microsoft.JSInterop to be AOT-compatible in .NET 8? Isn't it only used for WASM, which doesn't use Native AOT?
There was a problem hiding this comment.
It is used across WASM, Server and WebView. You've raised important points about these suppressions, and I will either pursue a fallback like you describe here or document my findings in an issue report for us to milestone and discuss like I did with BindConverter (#45578).
In the meantime, I have reverted my changes in JSInterop since @JamesNK's PR already disables analysis on the project.
There was a problem hiding this comment.
Documented in #45795
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/TaskGenericsUtil.cs
Outdated
Show resolved
Hide resolved
Nick-Stanton
changed the title
[AOT] Add RequiresDynamicCode annotation to Microsoft.AspNetCore.Routing
Adds
RequiresDynamicCodeannotations and appropriate warning suppressions to prepare Web UI's projects for AOT analysis.Description
The addition of the AOT analysis into our projects brought in ~40 warnings for us, half of which were related to reflection. The other half consist of JSON warnings (#45527) and Dependency Injection (dotnet/runtime#79286) and will be handled in the linked issues. This PR addresses the reflection warnings with the exception of
BindConverter.cswhich requires further conversation (see #45578). Please note that reflection withMakeGenericTypeandMakeGenericMethodare allowed for reference types and not value types in AOT, so suppressions should only happen in cases where we know only a reference type is being passed.Part of effort tracked in #45473