Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance DynamicallyAccessedMembers Attribute to apply to class #49465

Closed
LakshanF opened this issue Mar 11, 2021 · 5 comments
Closed

Enhance DynamicallyAccessedMembers Attribute to apply to class #49465

LakshanF opened this issue Mar 11, 2021 · 5 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics
Milestone

Comments

@LakshanF
Copy link
Member

LakshanF commented Mar 11, 2021

Background and Motivation

This proposal is taking @MichalStrehovsky's trimer proposal through its API change part. This proposal aligns well with the API proposal #49448 in enforcing the type that are required to be preserved for all derived types of a base class. #49448 helps with validating the type tree (parent type and its descendants) and this API enhancement will help with the enforcement.

The DynamicallyAccessedMembersAttribute attribute currently cannot be applied to a class or interface. Adding this support will allow developers to annotate a type to help the trimmer preserve its members that are being used. If the trimmer needs to keep the type, it will keep the required members on its derived types as well.

Proposed API

 namespace System.Diagnostics.CodeAnalysis
 {
     [AttributeUsage(
+        AttributeTargets.Class |
         AttributeTargets.Field |
         AttributeTargets.GenericParameter |
+        AttributeTargets.Interface |
+        AttributeTargets.Struct |
         AttributeTargets.Method |
         AttributeTargets.Parameter |
         AttributeTargets.Property |
         AttributeTargets.ReturnValue,
         Inherited = false)]
     public partial class DynamicallyAccessedMembersAttribute : Attribute
     {
     }
 }

Usage Examples

For a concrete example, see the existing code in EventSources which will create a manifest for a derived EventSource,

                         m_rawManifest = CreateManifestAndDescriptors(this.GetType(), Name, this);

If the EventSource Type is annotated with DynamicallyAccessedMembersAttribute as shown below, then the trimmer can preserve all methods of the derived EventSource and create a correct manifest for the derived EventSource type.

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
public class EventSource

Without such support, some event methods might not be included in the manifest generation, which can cause tool problems among other issue. See this customer comment for more details.

Alternative Designs

The API proposal #49448 on its own

Risks

This API might cause more code to be preserved than intended since it applies to all derived types of the annotated type.

@LakshanF LakshanF added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 11, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Mar 11, 2021
@LakshanF
Copy link
Member Author

cc @vitek-karas, @MichalStrehovsky

@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Diagnostics and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Mar 11, 2021
@ghost
Copy link

ghost commented Mar 11, 2021

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

Issue Details

Background and Motivation

This proposal is taking @MichalStrehovsky's trimer proposal through its API change part. This proposal aligns well with the API proposal #49448 in enforcing the type that are required to be preserved for all derived types of a base class. #49448 helps with validating the type tree (parent type and its descendants) and this API enhancement will help with the enforcement.

The DynamicallyAccessedMembersAttribute attribute currently cannot be applied to a class or interface. Adding this support will allow developers to annotate a type to help the trimmer preserve its members that are being used. If the trimmer needs to keep the type, it will keep the required members on its derived types as well.

Proposed API

namespace System.Diagnostics.CodeAnalysis
{
-        [AttributeUsage(
-        AttributeTargets.Field | AttributeTargets.ReturnValue | AttributeTargets.GenericParameter |
-        AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.Method,
-        Inherited = false)]
-    public sealed class DynamicallyAccessedMembersAttribute : Attribute

+        [AttributeUsage(
+        AttributeTargets.Interface | AttributeTargets.Class | 
+        AttributeTargets.Field | AttributeTargets.ReturnValue | AttributeTargets.GenericParameter |
+        AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.Method,
+        Inherited = false)]
+    public sealed class DynamicallyAccessedMembersAttribute : Attribute

     }

Usage Examples

For a concrete example, see the existing code in EventSources which will create a manifest for a derived EventSource,

                         m_rawManifest = CreateManifestAndDescriptors(this.GetType(), Name, this);

If the EventSource Type is annotated with DynamicallyAccessedMembersAttribute as shown below, then the trimmer can preserve all methods of the derived EventSource and create a correct manifest for the derived EventSource type.

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
public class EventSource

Without such support, some event methods might not be included in the manifest generation, which can cause tool problems among other issue. See this customer comment for more details.

Alternative Designs

The API proposal #49448 on its own

Risks

This API might cause more code to be preserved than intended since it applies to all derived types of the annotated type.

Author: LakshanF
Assignees: -
Labels:

api-ready-for-review, area-System.Diagnostics

Milestone: -

@terrajobst terrajobst added the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 11, 2021
@tommcdon tommcdon added this to the 6.0.0 milestone Mar 12, 2021
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 16, 2021
@terrajobst
Copy link
Member

terrajobst commented Mar 16, 2021

  • Looks good as proposed
 namespace System.Diagnostics.CodeAnalysis
 {
     [AttributeUsage(
+        AttributeTargets.Class |
         AttributeTargets.Field |
         AttributeTargets.GenericParameter |
+        AttributeTargets.Interface |
+        AttributeTargets.Struct |
         AttributeTargets.Method |
         AttributeTargets.Parameter |
         AttributeTargets.Property |
         AttributeTargets.ReturnValue,
         Inherited = false)]
     public partial class DynamicallyAccessedMembersAttribute : Attribute
     {
     }
 }

@terrajobst terrajobst removed the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 16, 2021
@LakshanF LakshanF self-assigned this Mar 22, 2021
@eerhardt
Copy link
Member

Resolved by #49778.

@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics
Projects
None yet
Development

No branches or pull requests

4 participants