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

Consider adding a new reflection API to get field values of an Enum #100592

Open
LakshanF opened this issue Apr 3, 2024 · 5 comments
Open

Consider adding a new reflection API to get field values of an Enum #100592

LakshanF opened this issue Apr 3, 2024 · 5 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Reflection linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@LakshanF
Copy link
Member

LakshanF commented Apr 3, 2024

The trimmer doesn't trim Enum fields. As mentioned in the comment here, it would make sense to have a separate API that doesn't require DynamicallyAccessedMembers annotation to get Enum fields. That would prevent suppressing the warning as done here when getting Enum fields.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 3, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Apr 3, 2024
@MichalPetryka
Copy link
Contributor

Can't NativeAOT not generate reflection info for such fields?

@jkotas
Copy link
Member

jkotas commented Apr 3, 2024

get field values of an Enum

Note that we have added trim and AOT friendly ways to get values of an Enum: Enum.GetValuesAsUnderlyingType - #72498.

The specific case in EnumConverter wants the field values presented as FieldInfos. Fetching enum values as FieldInfos is quite inefficient, but it is required here because of pre-existing public surface.

A new API can look like this:

public class Enum
{
    [UnconditionalSuppressMessage("Trimming", "IL2075:", Justification = "Trimmer does not trim Enums")]
    public static FieldInfo[] GetValuesAsFieldInfos(Type enumType)
    {
        ArgumentNullException.ThrowIfNull(enumType);

        if (!enumType.IsEnum)
            throw new ArgumentException(SR.Arg_MustBeEnum, "enumType")

       return enumType.GetFields(BindingFlags.Public | BindingFlags.Static);
    }

The question is whether fetching enum values as FieldInfos is common enough to warrant a new public API. I think we would want to see at least one or two additional cases from real-world code where this API can be used.

@jkotas jkotas added area-System.Reflection and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 3, 2024
@eerhardt
Copy link
Member

eerhardt commented Apr 3, 2024

The question is whether fetching enum values as FieldInfos is common enough to warrant a new public API.

Here's another use case:

#97737

There we need to get the EnumMemberAttribute on the Field of an Enum.

@steveharter steveharter added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Apr 17, 2024
@steveharter steveharter added this to the 9.0.0 milestone Apr 17, 2024
@steveharter steveharter added trimming-for-aot `EnableAggressiveTrimming=true` used for running tests with AOT linkable-framework Issues associated with delivering a linker friendly framework and removed trimming-for-aot `EnableAggressiveTrimming=true` used for running tests with AOT labels Apr 17, 2024
@steveharter
Copy link
Member

The specific case in EnumConverter wants the field values presented as FieldInfos. Fetching enum values as FieldInfos is quite inefficient, but it is required here because of pre-existing public surface.

The case in EnumConverter doesn't directly expose the FieldInfos but does check for [Browseable] attribute on the FieldInfo, so a new API is necessary here.

However, the case in EventSource could use Enum.GetValuesAsUnderlyingType() instead of FieldInfos as the FieldInfos used there are used temporarily + privately and do not check for the [Browseable] attribute.

Moving to V10; priority appears low here (removing a couple cases of UnconditionalSuppressMessage).

@steveharter steveharter modified the milestones: 9.0.0, 10.0.0 Jul 22, 2024
@eerhardt
Copy link
Member

Here's one more hit on the same scenario:

https://github.com/microsoft/OpenAPI.NET/pull/1717/files#diff-c59c9153ccf5e00142826d1c5e4d3d92470aac0bc740a302a23557432141277cR29

Getting the DisplayAttribute of an enum field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Reflection linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

No branches or pull requests

5 participants