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

[NativeAOT] Enum.GetValues(Type) should have an aot safe implementation #72140

Closed
LakshanF opened this issue Jul 13, 2022 · 7 comments
Closed

Comments

@LakshanF
Copy link
Contributor

We have annotated the above API, public static System.Array GetValues(System.Type enumType), as dangerous (RequiresDynamicCode("It might not be possible to create an array of the enum type at runtime. Use the GetValues<TEnum> overload instead.")).

Consider removing the RDC and providing a native AOT safe implementation.

@agocke
Copy link
Member

agocke commented Jul 14, 2022

Looks like this already exists as Enum.GetValues<Type>

@agocke agocke closed this as completed Jul 14, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 14, 2022
@LakshanF
Copy link
Contributor Author

I think we still need an implementation when the source of the type is not known and not able to use the generic one

@LakshanF LakshanF reopened this Jul 14, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 14, 2022
@MichalPetryka
Copy link
Contributor

MichalPetryka commented Jul 14, 2022

Well the issue is that AOT generally requires the type to be known since it can't generate code for every enum possible. So I'm not sure if this would be possible without keeping metadata about every enum.

@MichalStrehovsky
Copy link
Member

The problem is with creating the array type at runtime, not with "what values to fill in". We keep enough metadata to be able to fill it in.

We are discussing "fixing" this by having the API return the underlying type of the array. The signature is Array GetValues(Type). It currently returns SomeInt32Enum[] for SomeInt32Enum. But it could also return int[] and still satisfy the contract. There's an upper bound on primitive type arrays and we can pregenerate that.

We did this in the early days of .NET Native. Backtracked on that, but nobody remembers why. We want to try again :).

@Suchiman
Copy link
Contributor

@MichalStrehovsky couldn't the TypeLoader dynamically typeload SomeInt32Enum[] by basing it off the underlying type? Given that they have identical layouts and calling conventions, etc.

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky couldn't the TypeLoader dynamically typeload SomeInt32Enum[] by basing it off the underlying type? Given that they have identical layouts and calling conventions, etc.

It would probably leak out from the implementation of generic interfaces on the arrays. Maybe something like cast to IEnumerator(SomeEnum), do GetEnumerator and on the returned enumerator call the non-generic IEnumerator.Current. It would return a boxed Int because it's hardcoded in code. We need shared code over enums to get around that.

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this issue Jul 15, 2022
...at the cost of a small compat break. We return `int[]` instead of `SomeInt32Enum[]`.

Fixes dotnet#72140.

We can also delete intrinsic handling of `Enum.GetValues` in dataflow analysis but I don't want to conflict with Vitek's dotnet#71485.
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Jul 15, 2022
@agocke agocke added this to AppModel Jul 20, 2022
@MichalStrehovsky
Copy link
Member

#72498 is the way forward for this.

@MichalStrehovsky MichalStrehovsky closed this as not planned Won't fix, can't repro, duplicate, stale Jul 21, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants