Reduce the trim requirements on MetadataUpdateHandlerAttribute

[vitek-karas]

Currently the type passed to the .ctor of MetadataUpdateHandlerAttribute will be annotated with DynamicallyAccessedMemberTypes.All. For apps this doesn't matter because the attribute is marked for removal in trimmed apps by default, so the annotation is not applied.

But this does matter for analysis of libraries for trimming. The All annotation is particularly problematic because it will force include all nested types and may cause additional warnings produced by the analysis as it will treat all members on the type as potentially accessed via reflection (so any member annotated for trimming in any way will produce a warning).

My reading of the usage of MetadataUpdateHandlerAttribute shows that we only need methods on the target type (in fact we only need two specific static methods, but there's no way to annotate it that way right now).

Can we improve the annotation on the MetadataUpdateHandlerAttribute to only ask for methods: DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods?

[ghost]

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

Issue Details

---

Currently the type passed to the .ctor of MetadataUpdateHandlerAttribute will be annotated with DynamicallyAccessedMemberTypes.All. For apps this doesn't matter because the attribute is marked for removal in trimmed apps by default, so the annotation is not applied.

But this does matter for analysis of libraries for trimming. The All annotation is particularly problematic because it will force include all nested types and may cause additional warnings produced by the analysis as it will treat all members on the type as potentially accessed via reflection (so any member annotated for trimming in any way will produce a warning).

My reading of the usage of MetadataUpdateHandlerAttribute shows that we only need methods on the target type (in fact we only need two specific static methods, but there's no way to annotate it that way right now).

Can we improve the annotation on the MetadataUpdateHandlerAttribute to only ask for methods: DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods?

Author:	vitek-karas
Assignees:	-
Labels:	area-System.Reflection
Milestone:	-

[stephentoub]

For my edification, can you share an example from one of our handlers where this is causing an issue?

[vitek-karas]

I filed the issue because of this: dotnet/aspnetcore#40430 (comment)

The reduction of the annotation would not fix all of the problems (it would still require all methods and some of those warnings are about methods), but it would help.

We're solving the problem in ASP.NET by creating a subclass which will act solely as the update handler, the attribute points to it and the static methods then do what is necessary (either directly or by forwarding the call to the parent class). Actually, this feels like the best approach right now for the update handlers in general.

I'm wondering if we could also use static abstract methods on interfaces for this purpose - that would allow precise behavior. Personally I don't like these magical behaviors of "and something will call a method with name Update" - the caller has to do reflection and has to be relatively careful (get the right overload if there's more than one, and so on). Declaring the dependency via an interface just feels much cleaner.

[vitek-karas]

The All annotation being applied to a "random" class is also problematic because of compiler generated code. Technically the annotation allows one to use reflection to access anything on the class, including closure classes. Thus linker has to "mark" the closures and everything on them as well. If there's anything annotated anywhere on the closure, it will produce a warning which will be really difficult to explain since it may not directly map to user code. And even if linker could map the closure back to the method it originated from, it's still not clear if it can ignore the potential reflection access (in all other cases we're trying to treat the closures as "not there" and only consider them for the method they come from, but private reflection sees everything).

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Currently the type passed to the .ctor of MetadataUpdateHandlerAttribute will be annotated with DynamicallyAccessedMemberTypes.All. For apps this doesn't matter because the attribute is marked for removal in trimmed apps by default, so the annotation is not applied.

But this does matter for analysis of libraries for trimming. The All annotation is particularly problematic because it will force include all nested types and may cause additional warnings produced by the analysis as it will treat all members on the type as potentially accessed via reflection (so any member annotated for trimming in any way will produce a warning).

My reading of the usage of MetadataUpdateHandlerAttribute shows that we only need methods on the target type (in fact we only need two specific static methods, but there's no way to annotate it that way right now).

Can we improve the annotation on the MetadataUpdateHandlerAttribute to only ask for methods: DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods?

Author:	vitek-karas
Assignees:	-
Labels:	area-System.Reflection, linkable-framework
Milestone:	Future

[steveharter]

Moving to 8.0; we want to be more linker-friendly in 8.0.