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

Allow DynamicallyAccessedMembersAttribute on methods #36708

Closed
vitek-karas opened this issue May 19, 2020 · 2 comments · Fixed by #36340
Closed

Allow DynamicallyAccessedMembersAttribute on methods #36708

vitek-karas opened this issue May 19, 2020 · 2 comments · Fixed by #36340
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime linkable-framework Issues associated with delivering a linker friendly framework untriaged New issue has not been triaged by the area owner

Comments

@vitek-karas
Copy link
Member

Background and Motivation

The DynamicallyAccessedMembersAttribute is used to annotate "data things" (method parameters, fields, return values, ...) of type System.Type and System.String. Adding the attribute means that the code may access some members on the type in question (represented either as System.Type or as the type name in System.String) using reflection or similar APIs. We plan to use these attribute to annotate reflection APIs themselves, so for example Activator.CreateInstance(Type type) will have the attribute on the type parameter marking it to need default constructor.

But some reflection APIs will need to put this annotation onto the "this" parameter. For example Type.GetFields() - should have DynamicallyAccessedMembers[DynamicallyAccessedMemberType.PublicFields] on the "this" parameter.

C# doesn't have a way to add attribute to the "this" parameter, so there needs to be some other way to this.

The DynamicallyAccessedMembersAttribute addition has been discussed here: #33861.

Proposed API

Allow the DynamicallyAccessedMembersAttribute to be applied to methods, the meaning would be that it applies to the "this" parameter. Tools would only recognize the attribute on methods on types derived from System.Type and would issue a warning if such attribute is found on a method on any other type.

    [AttributeUsage(
        AttributeTargets.Field | AttributeTargets.ReturnValue | AttributeTargets.GenericParameter |
        AttributeTargets.Parameter | AttributeTargets.Property |
        AttributeTargets.Method, // This is the proposed API change
        Inherited = false)]
    public sealed class DynamicallyAccessedMembersAttribute : Attribute
    {
    }

Usage Examples

namespace System
{
    public class Type
    {
         [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)]
         public FieldInfo[] GetFields();
    }
}

Alternative Designs

Alternative is to introduce a new attribute type just for this case which would otherwise look exactly like the DynamicallyAccessedMembersAttribute. Possible names could be DynamicallyAccessedMembersOnThisAttribute. The semantics of such attribute would be exactly the same as those described above for DynamicallyAccessedMembersAttribute on method.

Risks

Allowing the DynamicallyAccessedMembersAttribute on methods may lead to users incorrectly adding it onto the method when they meant to put it onto either a method parameter or the return value of a method since compiler would not complain about such usage.

This will be partially mitigated by the tools (IL Linker) warnings about such cases.

In the future we can also mitigate this better by having a Roslyn analyzer detect such cases and warn about them directly in the IDE.

@vitek-karas vitek-karas added api-suggestion Early API idea and discussion, it is NOT ready for implementation linkable-framework Issues associated with delivering a linker friendly framework labels May 19, 2020
@vitek-karas vitek-karas self-assigned this May 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Runtime untriaged New issue has not been triaged by the area owner labels May 19, 2020
@vitek-karas
Copy link
Member Author

/cc @jkotas @MichalStrehovsky @eerhardt

@vitek-karas vitek-karas added api-ready-for-review and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 19, 2020
@jkotas jkotas added the blocking Marks issues that we want to fast track in order to unblock other important work label May 19, 2020
@eerhardt eerhardt added this to In progress in Linker optimization work May 20, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels May 29, 2020
@terrajobst
Copy link
Member

terrajobst commented May 29, 2020

Video

Looks good as proposed

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

Linker optimization work automation moved this from In progress to Done May 29, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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.Runtime linkable-framework Issues associated with delivering a linker friendly framework untriaged New issue has not been triaged by the area owner
Projects
Development

Successfully merging a pull request may close this issue.

4 participants