EnableAOTAnalyzer for Microsoft.Extensions.Hosting by eerhardt · Pull Request #73853 · dotnet/runtime

eerhardt · 2022-08-12T15:24:19Z

With this change, all Microsoft.Extensions.* libraries in dotnet/runtime have EnableAOTAnalyzer=true on.

ghost · 2022-08-12T15:24:24Z

Note regarding the new-api-needs-documentation label:

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.

ghost · 2022-08-12T15:24:44Z

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #71654

With this change, all Microsoft.Extensions.* libraries in dotnet/runtime have EnableAOTAnalyzer=true on.

Author:	eerhardt
Assignees:	-
Labels:	`new-api-needs-documentation`, `area-Extensions-Configuration`
Milestone:	-

ghost · 2022-08-12T15:40:24Z

Tagging subscribers to this area: @dotnet/area-extensions-hosting
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #71654

With this change, all Microsoft.Extensions.* libraries in dotnet/runtime have EnableAOTAnalyzer=true on.

Author:	eerhardt
Assignees:	eerhardt
Labels:	`area-Extensions-Configuration`, `area-Extensions-Hosting`
Milestone:	-

LakshanF

The justification for the single suppression seems safe to me.

src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/Internal/Win32.cs

tarekgh · 2022-08-12T17:17:04Z

src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs

        private PhysicalFileProvider? _defaultProvider;

+        [RequiresDynamicCode(Host.RequiresDynamicCodeMessage)]
+        public HostBuilder()


just wondering, does the attribute cannot be applied to the class fields?

No, it can only be applied to methods and the whole class.

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresDynamicCodeAttribute.cs

Lines 13 to 19 in e71a958

[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited = false)]

#if SYSTEM_PRIVATE_CORELIB

public

#else

internal

#endif

sealed class RequiresDynamicCodeAttribute : Attribute

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?

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.

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.

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.

Correct. That is exactly what I was thinking should work, but didn't.

Opened dotnet/linker#2970 to track the implementation, although this will need changes in linker/analyzer/nativeAOT

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.

(We could possibly do a pragma suppression on the field to shut up analyzer.)

tarekgh

Modulo @jkotas comment, LGTM.

Plus clean up the interop in GetParentProcess.

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs

MichalStrehovsky · 2022-08-12T20:17:11Z

src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs

            return diagnosticListener;
        }

+        [UnconditionalSuppressMessage("AOT", "IL3050:RequiresDynamicCode",


Just double checking that we won't MakeGeneric over the T here?

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.

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.

EnableAOTAnalyzer for Microsoft.Extensions.Hosting

e55c5fe

Fix dotnet#71654

eerhardt requested review from LakshanF, MichalStrehovsky and tarekgh August 12, 2022 15:24

ghost added area-Extensions-Configuration new-api-needs-documentation labels Aug 12, 2022

ghost assigned eerhardt Aug 12, 2022

eerhardt added area-Extensions-Hosting and removed new-api-needs-documentation labels Aug 12, 2022

eerhardt removed the area-Extensions-Configuration label Aug 12, 2022

LakshanF approved these changes Aug 12, 2022

View reviewed changes

jkotas reviewed Aug 12, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/Internal/Win32.cs Outdated Show resolved Hide resolved

tarekgh reviewed Aug 12, 2022

View reviewed changes

tarekgh approved these changes Aug 12, 2022

View reviewed changes

PR feedback

aaa9068

Plus clean up the interop in GetParentProcess.

MichalStrehovsky reviewed Aug 12, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs Outdated Show resolved Hide resolved

MichalStrehovsky reviewed Aug 12, 2022

View reviewed changes

MichalStrehovsky approved these changes Aug 12, 2022

View reviewed changes

tlakollo mentioned this pull request Aug 12, 2022

Suppress warning in instance fields dotnet/linker#2973

Merged

eerhardt added 2 commits August 12, 2022 23:44

Revert message

ec0aee7

Revert message

04616ea

eerhardt merged commit 30dc7e7 into dotnet:main Aug 13, 2022

eerhardt deleted the Fix71654 branch August 13, 2022 15:46

ghost locked as resolved and limited conversation to collaborators Sep 14, 2022

	[AttributeUsage(AttributeTargets.Method \| AttributeTargets.Constructor \| AttributeTargets.Class, Inherited = false)]
	#if SYSTEM_PRIVATE_CORELIB
	public
	#else
	internal
	#endif
	sealed class RequiresDynamicCodeAttribute : Attribute

Conversation

eerhardt commented Aug 12, 2022

Uh oh!

ghost commented Aug 12, 2022

Uh oh!

ghost commented Aug 12, 2022

Uh oh!

ghost commented Aug 12, 2022

Uh oh!

LakshanF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants