Skip to content

Conversation

@stephentoub
Copy link
Member

Our Enum rewrite earlier in .NET 8 broke the handling of ToString for [Flags] enums with negative values. Prior to that rewrite, all of the enum values were stored as a sorted array of ulongs, regardless of the enum's underlying type. Now, they're stored as a sorted array of the underlying type. However, that means that for signed underlying types, the position of negative values moved from the end to the beginning. We knew this, and accordingly updated tests that reflected that order (which as an implementation detail is observable via APIs that get the underlying values). But what we didn't notice because we didn't have tests covering it is that the logic for formatting [Flags] enums actually depends on those negative values being after the non-negative ones. That's for two reasons. First, there's logic that special-cases 0 and assumes that an enum value of 0 must be in the first slot of the values array if it exists. Second, the logic for deciding which enum values should be included starts at the largest unsigned value and walks down subtracting out matching bit patterns as they're found; if the values are sorted differently, the resulting strings are different. Not only might different names be included, but a number might be rendered if the order of evaluation means that no perfect subsetting is found even if there would have been had a different order been used.

This fixes the issues by restoring the ordering based on the values being unsigned. When we sort, we sort based on the unsigned version of the underlying primitive, even if it's signed.

@jaredpar hit this while trying to move Roslyn's tests to .NET 8.

@stephentoub stephentoub requested a review from jkotas March 17, 2023 20:59
@ghost ghost assigned stephentoub Mar 17, 2023
@ghost
Copy link

ghost commented Mar 17, 2023

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.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@stephentoub
Copy link
Member Author

I need to rethink this a bit. There are other places we compare / order the values...

Our Enum rewrite earlier in .NET 8 broke the handling of ToString for [Flags] enums with negative values.  Prior to that rewrite, all of the enum values were stored as a sorted array of ulongs, regardless of the enum's underlying type. Now, they're stored as a sorted array of the underlying type.  However, that means that for signed underlying types, the position of negative values moved from the end to the beginning.  We knew this, and accordingly updated tests that reflected that order (which as an implementation detail is observable via APIs that get the underlying values).  But what we didn't notice because we didn't have tests covering it is that the logic for formatting [Flags] enums actually depends on those negative values being after the non-negative ones.  That's for two reasons.  First, there's logic that special-cases 0 and assumes that an enum value of 0 must be in the first slot of the values array if it exists.  Second, the logic for deciding which enum values should be included starts at the largest unsigned value and walks down subtracting out matching bit patterns as they're found; if the values are sorted differently, the resulting strings are different.  Not only might different names be included, but a number might be rendered if the order of evaluation means that no perfect subsetting is found even if there would have been had a different order been used.

This fixes the issues by restoring the ordering based on the values being unsigned.  When we sort, we sort based on the unsigned version of the underlying primitive, even if it's signed.
Rather than 13 possible `EnumInfo<T>`, we consolidate down to just 8, such that for all of the underlying types that are signed integers, we instead always use their unsigned counterparts.  This then ensures that the data for the values is always sorted according to the unsigned representation and all logic in Enum that performs searches based on value is always doing so consistently.
@stephentoub
Copy link
Member Author

@jkotas, I reworked this (after your approval). Are you ok with the approach? Basically for signed integer underlying values we now store the data as if it were unsigned.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks good to me. Thank you for fixing it!

@stephentoub
Copy link
Member Author

Test failures are known issues

@stephentoub
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@stephentoub
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ghost ghost locked as resolved and limited conversation to collaborators Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants