EnableAOTAnalyzer for Microsoft.Extensions.Hosting #73853
Conversation
ghost
added
|
Note regarding the 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. |
|
Tagging subscribers to this area: @dotnet/area-extensions-configuration Issue DetailsFix #71654 With this change, all Microsoft.Extensions.* libraries in dotnet/runtime have EnableAOTAnalyzer=true on.
|
|
Tagging subscribers to this area: @dotnet/area-extensions-hosting Issue DetailsFix #71654 With this change, all Microsoft.Extensions.* libraries in dotnet/runtime have EnableAOTAnalyzer=true on.
|
LakshanF
left a comment
LakshanF
left a comment
There was a problem hiding this comment.
The justification for the single suppression seems safe to me.
src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/Internal/Win32.cs
Outdated
Show resolved
Hide resolved
| private PhysicalFileProvider? _defaultProvider; | ||
|
|
||
| [RequiresDynamicCode(Host.RequiresDynamicCodeMessage)] | ||
| public HostBuilder() |
There was a problem hiding this comment.
just wondering, does the attribute cannot be applied to the class fields?
There was a problem hiding this comment.
No, it can only be applied to methods and the whole class.
Here I didn't want to mark the whole class, as that has other side-effects with the analyzer - like warning if some other code is using static methods.
I kind of wish just marking the ctor as RequiresDynamicCode suppressed the code in the field initializer, but I didn't see that happening. @tlakollo - is this supposed to work?
There was a problem hiding this comment.
It wouldn't a field initializer will not only call the .ctor but also the .cctor, we don't allow to annotate the .cctor directly and the workaround is to annotate the whole class.
There was a problem hiding this comment.
a field initializer will not only call the .ctor but also the .cctor
Yes, it will call the .cctor, but the .cctor doesn't have any "unsafe" code in it. Just the .ctor would, with an instance field initializer.
There was a problem hiding this comment.
Got it. So what you mean is that you would want something like:
...
private IServiceFactoryAdapter _serviceProviderFactory = new ServiceFactoryAdapter<IServiceCollection>(new DefaultServiceProviderFactory());
...
[RequiresDynamicCode(Host.RequiresDynamicCodeMessage)]
public HostBuilder() {}
...To suppress the warning in the instance field?
If that is the case, yes currently that's not implemented but sounds like a feature that could be done.
There was a problem hiding this comment.
Correct. That is exactly what I was thinking should work, but didn't.
There was a problem hiding this comment.
Opened dotnet/linker#2970 to track the implementation, although this will need changes in linker/analyzer/nativeAOT
There was a problem hiding this comment.
although this will need changes in linker/analyzer/nativeAOT
I think that's an analyzer-only change. IL based analysis will already see this as part of the constructor method body.
There was a problem hiding this comment.
(We could possibly do a pragma suppression on the field to shut up analyzer.)
Plus clean up the interop in GetParentProcess.
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
| return diagnosticListener; | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("AOT", "IL3050:RequiresDynamicCode", |
There was a problem hiding this comment.
Just double checking that we won't MakeGeneric over the T here?
There was a problem hiding this comment.
My understanding is that is only a concern for the DiagnosticSource-EventSource bridge in order to write property values out to an EventSource. This isn’t for EventSource listeners. I’m honestly not even sure how that would work because these objects (IHostBuilder, Host) don’t really have properties that can be read. The usage here is that the objects are passed to listeners in the same process in order to get hooks into the building process.
There was a problem hiding this comment.
I think I've seen that failing in the tests: #73420
I've had a bunch of question marks on how this all works, so I've just filed an issue on it and moved on with life.
6 participants
Fix #71654
With this change, all Microsoft.Extensions.* libraries in dotnet/runtime have EnableAOTAnalyzer=true on.