ifdef out unsupported Enum underlying types for nativeaot

[stephentoub]

@jkotas, worthwhile?

Contributes to #79437

[dotnet-issue-labeler]

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

[jkotas]

It looks fine to me. I would make a new #define for it at the top of the file to make it more self-documenting. We may want to enable the define on other targets where we care a lot about size a lot, like wasm. Thank you for looking for ways to fix the size regression!

[marek-safar]

@SamMonoRT @kotlarmilos should this also include all mobile/web targets?

[SamMonoRT]

Yes, if we can include the mobile/web targets to mitigate the app size regressions, we should include those now, till we are able to identify and fix the Mono AOT compiler limitations.

[jkotas]

I have pushed a commit to disable the rare enums support for Mono as well. Please take a look.

[kotlarmilos]

I've inspected native symbols of an iOS app and non-integral types except bool are supported in the Mono AOT engine.

[jkotas]

Ok, I am going to keep the rare enums enabled for Mono. I do not data to make the call on whether to disable it for Mono or not. It can be done later - it is two line change to enable/disable this for given config.

[SamMonoRT]

Sounds good, leave it enabled for now. We can run some measurements and see if it helps in size if we disable it.

[kotlarmilos]

It doesn't look to me like breaking change - by doing that we will restrict some theoretical or borderline cases. I would suggest excluding them as they are not expressible in C# and such change could mitigate the size regression. Some edge cases might fail where non-integral enums are created by using reflection, which is not recommended by the documentation.

Let's hear the other opinions as well. If it is a blocker, we can proceed and update it once we align.

[jkotas]

@MichalStrehovsky PTLA

This fixes some of the size regression by ifdefing out support for rare enums, to match what we used to do in native AOT before the recent Enum changes.

[MichalStrehovsky]

Looks good, although I've always been puzzled by our position on these. We care, but also we don't care.

I'd honestly just delete the reflection support for these and keep things simple, because it's either important and it was a bug that CoreCLR-AOT didn't have this, or it's not important and people won't miss it on CoreCLR-JIT either. (Unless there's something unique for CoreCLR-JIT, like C++/CLI support, that makes it needed there.)

Do we need a breaking change label for Mono?

[jkotas]

unique for CoreCLR-JIT, like C++/CLI support

Yes, C++/CLI support, obfuscated code and compat for niche languages.

e.g. this compiles fine in C++/CLI:

enum MyEnum : intptr_t
{
	Red, Green, Blue
};

[jkotas]

Do we need a breaking change label for Mono?

@SamMonoRT How would like to handle that?

[jkotas]

`enum MyEnum : intptr_t { Red, Green, Blue };

This example is fine actually. C++/CLI will lower the underlying type to int32/int64. A better example:

enum MyEnum : bool
{
	Red, Blue
};

[SamMonoRT]

Do we need a breaking change label for Mono?

@SamMonoRT How would like to handle that?

Adding @lambdageek and @vargaz for their opinion.

[jkotas]

All known errors according to the build analysis.