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

Tests for float- and double- backed Enums #29266

Closed
filipnavara opened this issue Apr 15, 2019 · 19 comments · Fixed by #78580
Closed

Tests for float- and double- backed Enums #29266

filipnavara opened this issue Apr 15, 2019 · 19 comments · Fixed by #78580

Comments

@filipnavara
Copy link
Member

There are numerous test for Enum types backed by float or double in EnumTests.cs and EnumTests.netcoreapp.cs.

The ECMA specification doesn't allow these Enum types:

The symbols of an enum are represented by an underlying integer type: one of { bool, char, int8, unsigned int8, int16, unsigned int16, int32, unsigned int32, int64, unsigned int64, native int, unsigned native int }

(ECMA CLR standard, §II.14.3)

Moreover, in many cases the tests actually expect a broken behaviour like incorrect parsing of values or Enum.GetNames/Enum.GetValues not returning results sorted by the values:

https://github.com/dotnet/corefx/blob/10d7d6f812295a572eb148aedb19efd2f96fbe17/src/System.Runtime/tests/System/EnumTests.cs#L90-L96

I believe these tests should be removed, CoreCLR should not allow float- and double- backed enums and there should be test verifying that such enums cannot be created.

/cc @jkotas

@masonwheeler
Copy link
Contributor

I see something like this and I wonder, how did this even come to exist in the first place when it's a thing that the CLR spec disallows?

@jkotas
Copy link
Member

jkotas commented Apr 15, 2019

it's a thing that the CLR spec disallows

It was not always disallowed. https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Enum.cs#L18 explains the history.

@filipnavara
Copy link
Member Author

The tests for this behaviour were added after the fact, probably in good faith. The fact that they test incorrect behaviour is what bother me though. I implemented support for these Enums in Mono, but I also fixed the bugs in parsing/sorting and now it makes the tests fail. I don't think there's any value in testing bug-for-bug compatibility here.

@jkotas
Copy link
Member

jkotas commented Apr 15, 2019

I agree that it does not make sense to be testing bug-for-bug compatibility.

Does Mono use the parsing/sorting implementation from shared partition? I would be fine to update parsing/sorting implementation or tests to have more sensible behavior for these. (Including changing the obviously broken cases to throw instead of returning wrong result.)

As for disallowing the float/double (and intptr/uintptr - they are in a very similar boat) backed enums in the type loader, it has a good chance of breaking boutique languages or obfuscated code. I would do that only if we get something significant that can justify the break in return.

@filipnavara
Copy link
Member Author

filipnavara commented Apr 15, 2019

Does Mono use the parsing/sorting implementation from shared partition?

It uses the code from shared partition, but these particular cases are handled in the non-shared code.

Notably, CoreCLR sorts the names/values in native code (https://github.com/dotnet/coreclr/blob/d3e39bc2f81e3dbf9e4b96347f62b49d8700336c/src/vm/reflectioninvocation.cpp#L2822-L2826), while Mono does it in managed code (https://github.com/mono/mono/blob/b1af4c2726cdc211971efacee7d66a6e447336d2/mcs/class/System.Private.CoreLib/System/Enum.cs#L118-L119). I intentionally broke the sorting in Mono to match the tests, but I can reinstate it by changing one line.

The parsing is also happening in native code in Mono. I didn't find where the values in CoreCLR come from.

Would it make sense if I compile a list of the "broken" test cases?

@masonwheeler
Copy link
Contributor

@jkotas

it has a good chance of breaking ... obfuscated code.

You say that as if it's a bad thing...

@tannergooding
Copy link
Member

and intptr/uintptr - they are in a very similar boat

For these in particular, it is worth noting that even the current spec currently allows it (unlike float/double); However, the spec also says that native int constants should be created from an int32:

Native size constants (type native int) shall be created by conversion from int32 (conversion from int64 would not be portable) using conv.i or conv.u.

@jkotas
Copy link
Member

jkotas commented Apr 15, 2019

Would it make sense if I compile a list of the "broken" test cases?

Sounds good to me.

@danmoseley
Copy link
Member

it has a good chance of breaking ... obfuscated code.

You say that as if it's a bad thing...

It is if you rely on it working. 😃

@masonwheeler
Copy link
Contributor

That's the thing. I don't want obfuscated code working, especially when one of the biggest use cases for it these days is for malware authors to make their malicious code harder to reverse-engineer!

@filipnavara
Copy link
Member Author

filipnavara commented Apr 16, 2019

Here's the list of tests that fail on Mono implementation:

  Discovering: System.Runtime.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Runtime.Tests (found 32 of 5361 test cases)
  Starting:    System.Runtime.Tests (parallel test collections = on, max threads = 4)
    System.Tests.EnumTests.GetNames_GetValues(enumType: typeof(TestName_Single), expectedNames: ["Value1", "Value2", "Value0x3f06", "Value0x3000", "Value0x0f06", ...], expectedValues: [Value1, Value2, Value0x3f06, Value0x3000, Value0x0f06, ...]) [FAIL]
      Assert.Equal() Failure
      Expected: String[] ["Value1", "Value2", "Value0x3f06", "Value0x3000", "Value0x0f06", ...]
      Actual:   String[] ["Value0x0000", "Value1", "Value2", "Value0x0010", "Value0x0f06", ...]
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.cs(1189,0): at System.Tests.EnumTests.GetNames_GetValues(Type enumType, String[] expectedNames, Object[] expectedValues)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.GetNames_GetValues(enumType: typeof(TestName_Double), expectedNames: ["Value1", "Value2", "Value0x3f06", "Value0x3000", "Value0x0f06", ...], expectedValues: [Value1, Value2, Value0x3f06, Value0x3000, Value0x0f06, ...]) [FAIL]
      Assert.Equal() Failure
      Expected: String[] ["Value1", "Value2", "Value0x3f06", "Value0x3000", "Value0x0f06", ...]
      Actual:   String[] ["Value0x0000", "Value1", "Value2", "Value0x0010", "Value0x0f06", ...]
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.cs(1189,0): at System.Tests.EnumTests.GetNames_GetValues(Type enumType, String[] expectedNames, Object[] expectedValues)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.Parse<TestName_Single>(value: "Value1", ignoreCase: False, expected: Value0x0000) [FAIL]
      Assert.Equal() Failure
      Expected: Value0x0000
      Actual:   Value1
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.cs(127,0): at System.Tests.EnumTests.Parse[TestName_Single](String value, Boolean ignoreCase, TestName_Single expected)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.Parse<TestName_Single>(value: "vaLue2", ignoreCase: True, expected: Value0x0000) [FAIL]
      Assert.Equal() Failure
      Expected: Value0x0000
      Actual:   Value2
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.cs(133,0): at System.Tests.EnumTests.Parse[TestName_Single](String value, Boolean ignoreCase, TestName_Single expected)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.Parse<TestName_Double>(value: "Value1", ignoreCase: False, expected: Value0x0000) [FAIL]
      Assert.Equal() Failure
      Expected: Value0x0000
      Actual:   Value1
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.cs(127,0): at System.Tests.EnumTests.Parse[TestName_Double](String value, Boolean ignoreCase, TestName_Double expected)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.Parse<TestName_Double>(value: "vaLue2", ignoreCase: True, expected: Value0x0000) [FAIL]
      Assert.Equal() Failure
      Expected: Value0x0000
      Actual:   Value2
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.cs(133,0): at System.Tests.EnumTests.Parse[TestName_Double](String value, Boolean ignoreCase, TestName_Double expected)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.Parse_NetCoreApp11<TestName_Single>(value: "Value1", ignoreCase: False, expected: Value0x0000) [FAIL]
      Assert.Equal() Failure
      Expected: Value0x0000
      Actual:   Value1
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.netcoreapp.cs(22,0): at System.Tests.EnumTests.Parse_NetCoreApp11[TestName_Single](String value, Boolean ignoreCase, TestName_Single expected)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.Parse_NetCoreApp11<TestName_Single>(value: "vaLue2", ignoreCase: True, expected: Value0x0000) [FAIL]
      Assert.Equal() Failure
      Expected: Value0x0000
      Actual:   Value2
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.netcoreapp.cs(28,0): at System.Tests.EnumTests.Parse_NetCoreApp11[TestName_Single](String value, Boolean ignoreCase, TestName_Single expected)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.Parse_NetCoreApp11<TestName_Double>(value: "Value1", ignoreCase: False, expected: Value0x0000) [FAIL]
      Assert.Equal() Failure
      Expected: Value0x0000
      Actual:   Value1
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.netcoreapp.cs(22,0): at System.Tests.EnumTests.Parse_NetCoreApp11[TestName_Double](String value, Boolean ignoreCase, TestName_Double expected)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Tests.EnumTests.Parse_NetCoreApp11<TestName_Double>(value: "vaLue2", ignoreCase: True, expected: Value0x0000) [FAIL]
      Assert.Equal() Failure
      Expected: Value0x0000
      Actual:   Value2
      Stack Trace:
        /Users/vsts/agent/2.149.2/work/1/s/src/System.Runtime/tests/System/EnumTests.netcoreapp.cs(28,0): at System.Tests.EnumTests.Parse_NetCoreApp11[TestName_Double](String value, Boolean ignoreCase, TestName_Double expected)
        /Users/filipnavara/Documents/mono/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs(395,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
  Finished:    System.Runtime.Tests

Six of them are related to sorting in ReflectionEnum::GetEnumValuesAndNames. Four of them rely on incorrect results from parsing.

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

sorting in ReflectionEnum::GetEnumValuesAndNames

What is different in this sorting in Mono that makes it work right? I am not able to tell by just looking at the code.

Four of them rely on incorrect results from parsing.

Does Mono parses these correctly?

@filipnavara
Copy link
Member Author

filipnavara commented Apr 16, 2019

What is different in this sorting in Mono that makes it work right?

It actually sorts the values for R4- and R8-typed enums. CoreCLR returns them in declared order.

Does Mono parses these correctly?

Yes, as far as I can tell. Mono parses case-insensitive "vaLue2" to "Value2" that is in the enum definition. CoreCLR seems to always return the first element of the enum.

@filipnavara
Copy link
Member Author

I am not able to tell by just looking at the code.

The current code in Mono is intentionally broken to pass the test. I'll post links to the relevant parts and explanations once I get back to my computer.

@filipnavara
Copy link
Member Author

filipnavara commented Apr 16, 2019

Here's the list of relevant Mono code that causes the different sorting behaviour:

  • Major part of Enum.CompareTo is implemented in internal call. There's added case for float and double comparison.
  • Enum.GetValues and Enum.GetNames is wired up in almost identical fashion to CoreCLR on the managed side. Both Mono and CoreCLR use an internal call for the actual implementation. The difference is that Mono implements the sorting on the managed side and CoreCLR implements it on the unmanaged side. In both cases the float and double enums are not sorted correctly. In Mono the fix is simple switch to float/double comparer.
  • Both implementations have shortcut for enums that are already sorted. CoreCLR one doesn't handle ELEMENT_TYPE_R4/ELEMENT_TYPE_R8 in the switch right above the optimisation. Mono one handles R4/R8 through read_enum_value helper function, but in the end just compares the values as ulong instead of float/double which is just as broken (not for the values in the CoreFX test though). The hack that currently prevents the proper sorting is that it return true to the managed code and skips the sorting. I have locally modified it to always return false for R4/R8 enums and fallback to the sorting in managed code (not optimal, but helpful to identify the broken tests).

I don't have explanation for the the CoreCLR difference in the parsing tests. Couple of the tests rely on the unsorted enums and access the values as Enum.GetValues(...)[0]. Once the sorting is fixed it returns different value than expected and the tests fail. Other tests openly admit in the comments that it tests for broken behaviour.

I'll publish my Mono changes in a branch in case I missed something and link it here.

@filipnavara
Copy link
Member Author

/cc @marek-safar

@marek-safar
Copy link
Contributor

marek-safar commented May 9, 2019

@stephentoub @danmosemsft what is your preferred way to resolve this issue?

@danmoseley
Copy link
Member

@marek-safar in the near term I think it is fine to disable these as @EgorBo is already doing.

The actual runtime behavior we want seems a corner case discussion we can defer. I'd be inclined to say that both runtimes can just keep doing what they're doing unless and until we get customer feedback that they depend on the behaviors and then we might want to try to make them consistent.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@tannergooding tannergooding added area-System.Numerics and removed area-System.Runtime untriaged New issue has not been triaged by the area owner labels Jun 23, 2020
@ghost
Copy link

ghost commented Jun 23, 2020

Tagging subscribers to this area: @tannergooding
Notify danmosemsft if you want to be subscribed.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 26, 2022
stephentoub pushed a commit to stephentoub/runtime that referenced this issue Dec 2, 2022
…bles

This change avoids allocation of the temporary ulong[] array during EnumInfo initialization

Fixes dotnet#29266
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 5, 2023
ViktorHofer pushed a commit to dotnet/winforms that referenced this issue Feb 13, 2023
* Add generic Enum.TryFormat method

* Rewrite Enum using generic specialization per underlying type

Also revises the shape of the newly added Enum.TryFormat method and fixes tests accordingly.

* Use Enum.TryFormat in interpolated string handlers

* Implement ISpanFormattable on Enum

* Use generic Enum methods in more places

* Replace Unsafe.As with unmanaged pointers where possible

Reduces cost of the generic methods

* Simplify Enum_GetValuesAndNames QCall, fix handling of floats and doubles

This change avoids allocation of the temporary ulong[] array during EnumInfo initialization

Fixes dotnet/runtime#29266

* Fix mono's GetEnumNamesAndValues, and make ToObject handle float/double as well

* Fix perf gap and polish

* Undo update of Enum.GetTypeCode to include float/double

* Address PR feedback

* Accomodate net8.0 TFM update

* Fix InvalidCastException from GetEnumValues on mono when dealing with bool-based enums

Co-authored-by: Heath Baron-Morgan <[email protected]>
Co-authored-by: Jan Kotas <[email protected]>

Commit migrated from dotnet/runtime@62f3eb2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants