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

[API Proposal]: Type.GetEnumValuesAsUnderlyingType #72498

Closed
MichalStrehovsky opened this issue Jul 19, 2022 · 9 comments · Fixed by #73057
Closed

[API Proposal]: Type.GetEnumValuesAsUnderlyingType #72498

MichalStrehovsky opened this issue Jul 19, 2022 · 9 comments · Fixed by #73057
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Jul 19, 2022

Background and motivation

The existing Type.GetEnumValues API returns a System.Array that is of the given enum type (i.e. SomeEnum[] when called on typeof(SomeEnum)). This makes implementing the API challenging when the enum array cannot be created. There are multiple reasons why the enum array might not be possible to create:

  1. We're in a reflection-only context such as MetadataLoadContext:
    public sealed override Array GetEnumValues() => throw new InvalidOperationException(SR.Arg_InvalidOperation_Reflection);
  2. We're on a platform where runtime codegen (or universal shared code) is not available.

It would be good to have API that can work in these contexts by returning an array of the underlying type (i.e. int[] for typeof(SomeInt32Enum).

API Proposal

namespace System;

public abstract class Type
{
    public virtual Array GetEnumValuesAsUnderlyingType();
}

API Usage

foreach (var value in typeof(SomeEnum).GetEnumValuesAsUnderlyingType())
    Console.WriteLine(value);

// Prints:
// 0
// 1
//
// As opposed to:
//
// SomeEnumValue1
// SomeEnumValue2

Alternative Designs

Allow implementers to not satisfy the contract

The implementation of the existing Type.GetEnumValues could just return the underlying array directly since the signature is just System.Array. But that is probably pretty breaking - see #72236 (comment) for samples.

Write more code

Can be expressed in a less performant and more verbose way already:

foreach (var f in typeof(DayOfWeek).GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static))
{
    Console.WriteLine(f.Name);
    Console.WriteLine(f.GetRawConstantValue()); // This returns the field value as boxed underlying type
}

Risks

No response

@MichalStrehovsky MichalStrehovsky added area-System.Reflection blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 19, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 19, 2022
@ghost
Copy link

ghost commented Jul 19, 2022

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

Issue Details

Background and motivation

The existing Type.GetEnumValues API returns a System.Array that is of the given enum type (i.e. SomeEnum[] when called on typeof(SomeEnum)). This makes implementing the API challenging when the enum array cannot be created. There are multiple reasons why the enum array might not be possible to create:

  1. We're in a reflection-only context such as MetadataLoadContext:
    public sealed override Array GetEnumValues() => throw new InvalidOperationException(SR.Arg_InvalidOperation_Reflection);
  2. We're on a platform where runtime codegen (or universal shared code) is not available.

It would be good to have API that can work in these contexts by returning an array of the underlying type (i.e. int[] for typeof(SomeInt32Enum).

API Proposal

namespace System;

public abstract class Type
{
    public virtual Array GetEnumValuesAsUnderlyingType();
}

API Usage

foreach (var value in typeof(SomeEnum).GetEnumValuesAsUnderlyingType())
    Console.WriteLine(value);

// Prints:
// 0
// 1
//
// As opposed to:
//
// SomeEnumValue1
// SomeEnumValue2

Alternative Designs

Allow implementers to not satisfy the contract

The implementation of the existing Type.GetEnumValues could just return the underlying array directly since the signature is just System.Array. But that is probably pretty breaking - see #72236 (comment) for samples.

Write more code

Can be expressed in a less performant and more verbose way already:

foreach (var f in typeof(DayOfWeek).GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static))
{
    Console.WriteLine(f.Name);
    Console.WriteLine(f.GetRawConstantValue()); // This returns the field value as boxed underlying type
}

Risks

No response

Author: MichalStrehovsky
Assignees: -
Labels:

area-System.Reflection, blocking, api-ready-for-review

Milestone: -

@MichalPetryka
Copy link
Contributor

MichalPetryka commented Jul 20, 2022

Would this API return an array of the underlying type in all cases or only when it's not possible to figure out the type under AOT?

@MichalStrehovsky
Copy link
Member Author

Would this API return an array of the underlying type in all cases or only when it's not possible to figure out the type under AOT?

In all cases. Switching the array type around would be a compat problem in the same sense as returning int[] from the existing API. No reason to introduce it.

@MichalPetryka
Copy link
Contributor

Does it make more sense to have the API on Type or in Enum? Currently APIs are in both of those places.

@MichalStrehovsky
Copy link
Member Author

For the MetadataLoadContext use case, the Type API is the one that makes sense.

I'll leave it up to the API review board whether it makes sense to add a shortcut on Enum for discoverability.

@GrabYourPitchforks
Copy link
Member

@MichalStrehovsky This is marked blocking. Be sure to add a milestone (7.0 or Future) as well. Thanks!

@MichalStrehovsky MichalStrehovsky added this to the 7.0.0 milestone Jul 20, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 20, 2022
@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky This is marked blocking. Be sure to add a milestone (7.0 or Future) as well. Thanks!

Marked for 7.0, thanks for spotting!

@marek-safar
Copy link
Contributor

/cc @lambdageek

@terrajobst
Copy link
Member

terrajobst commented Jul 26, 2022

Video

  • Let's add a corresponding method on Enum as this would maintain the parity with the existing methods that exist on both Type and Enum.
  • Let's also add a generic version for parity
namespace System;

public partial class Type
{
    // Existing enum related methods:
    //
    // public virtual string? GetEnumName(Object);
    // public virtual string[] GetEnumNames();
    // public virtual Type GetEnumUnderlyingType();
    // public virtual object[] GetEnumValues();

    public virtual Array GetEnumValuesAsUnderlyingType();
}

public partial class Enum
{
    public static Array GetValuesAsUnderlyingType<TEnum>() where TEnum: struct, Enum;
    public static Array GetValuesAsUnderlyingType(Type type);
}

@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 Jul 26, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 29, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2022
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.Reflection blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants