search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

Update DynamicallyAccessedMembers annotation on EventSource #110001

Closed MichalStrehovsky closed 1 day ago

MichalStrehovsky commented 2 days ago

This takes advantage of #109814. This is in theory a breaking change in case someone took advantage of our annotation and is doing their own reflection on EventSource descendants.

Someone else reflecting on EventSource is problematic however. We placed around various suppressions due to our own annotations like this:

https://github.com/dotnet/runtime/blob/0d62887a30553b8177dc90f9e39559be0e6c7707/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs#L414-L424

It means that if someone else is reflection-accessing various members on EventSource, they would not get a trimming warning. Hopefully, nobody does that. This PR also assumes that nobody does that (and the PR should therefore not be breaking in practice).

Cc @eerhardt @dotnet/illink

dotnet-issue-labeler[bot] commented 2 days ago

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, 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.
dotnet-issue-labeler[bot] commented 2 days ago

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, 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.
dotnet-policy-service[bot] commented 2 days ago

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti See info in area-owners.md if you want to be subscribed.

eerhardt commented 1 day ago

It means that if someone else is reflection-accessing various members on EventSource, they would not get a trimming warning.

Why wouldn't they get a trimming warning?

MichalStrehovsky commented 1 day ago

It means that if someone else is reflection-accessing various members on EventSource, they would not get a trimming warning.

Why wouldn't they get a trimming warning?

Because we suppressed them with the suppression I quoted. We annotate EventSource as "this is going to reflect on all methods" and we have a bunch of methods that should never be reflection-invoked. We suppressed it but the suppression will apply to anyone else taking advantage of the annotation because annotation doesn't discriminate.

eerhardt commented 1 day ago

It means that if someone else is reflection-accessing various members on EventSource, they would not get a trimming warning.

Why wouldn't they get a trimming warning?

Because we suppressed them with the suppression I quoted. We annotate EventSource as "this is going to reflect on all methods" and we have a bunch of methods that should never be reflection-invoked. We suppressed it but the suppression will apply to anyone else taking advantage of the annotation because annotation doesn't discriminate.

If someone else is reflection-accessing these specific methods (GenerateManifest, WriteEventCore, WriteEventWithRelatedActivityIdCore, WriteEvent(int, object[]), CreateManifestAndDescriptors) they won't get warnings. But that is the same as it is today with using .All, so this PR isn't making it worse.

The part that could be considered breaking is that someone could have been calling GetType() on some EventSource object and getting the properties/fields/etc from it. Before they wouldn't get a warning since we used .All on the whole class. Now we are only doing methods and nested types. But in this case they WILL get a warning.

MichalStrehovsky commented 1 day ago

The part that could be considered breaking is that someone could have been calling GetType() on some EventSource object and getting the properties/fields/etc from it. Before they wouldn't get a warning since we used .All on the whole class. Now we are only doing methods and nested types. But in this case they WILL get a warning.

Yep, what I meant is that we annotate this as reflected-on "for us", not for someone else to use. The suppressions are one of the indicators that this is very specifically meant for us. Unfortunately there's no annotation that could make this contractually private so someone could be still taking advantage of it.