Annotate Conditional attributes for trimmability #9938
Conversation
This should ensure the target members are kept when we're running the tests with trimming enabled. Some tests use these attributes with framework types and framework is always considered trimmable. Example: https://github.com/dotnet/runtime/blob/aafe4d465f97fa3a0ae61b18c2887cf84085d25d/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs#L15
|
@dotnet/ilc-contrib could someone have a look? This is just a couple These values are funneled to here if you want to check I got the MemberKinds right: It's not possible to funnel/annotate this thorough because it passes through xUnit's reflection abstraction thingie. xUnit loves it's abstractions. We don't care about the dataflow warnings. |
| internal const DynamicallyAccessedMemberTypes ConditionalMemberKinds = DynamicallyAccessedMemberTypes.PublicFields | | ||
| DynamicallyAccessedMemberTypes.PublicMethods | | ||
| DynamicallyAccessedMemberTypes.PublicProperties; |
There was a problem hiding this comment.
Based on your description the code in the consumer calls TypeInfo.GetDeclaredMethod (for example):
That method is annotated with PublicMethods | NonPublicMethods so the annotation doesn't match. And to make things worse the code loops (recursively) over base types doing the same - so effectively private reflection on base types. The only annotation which supports that is All - so I think this should be All.
There was a problem hiding this comment.
Good catch! Having GetMethod/GetDeclaredMethod differ in their visibilities was a top-notch decision on the API designer's part.
I saw it goes to the base type but I didn't want to bother with it because people's instinct is to spell out the type that has the property/method/field and I was worried about keeping too much. It does have side effects that are observable on MakeGeneric and we haven't done an exercise to get the framework RDC warning free yet and it might be useful to at least get the heavy hitters clean. But maybe All is better because then we don't need to introduce the const for it either. This is used rarely on framework types.
This should ensure the target members are kept when we're running the tests with trimming enabled.
Some tests use these attributes with framework types and framework is always considered trimmable. Example: https://github.com/dotnet/runtime/blob/aafe4d465f97fa3a0ae61b18c2887cf84085d25d/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs#L15
Cc @dotnet/ilc-contrib