Fix ToString handling of [Flags] enums with negative values#83616
Merged
stephentoub merged 2 commits intodotnet:mainfrom Mar 20, 2023
Merged
Fix ToString handling of [Flags] enums with negative values#83616stephentoub merged 2 commits intodotnet:mainfrom
stephentoub merged 2 commits intodotnet:mainfrom
Conversation
|
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. |
jaredpar
approved these changes
Mar 17, 2023
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.
4878638 to
ee03b43
Compare
This was referenced Mar 19, 2023
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.
7b1522a to
ed6709b
Compare
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. |
jkotas
approved these changes
Mar 19, 2023
Member
jkotas
left a comment
There was a problem hiding this comment.
Yes, it looks good to me. Thank you for fixing it!
Member
Author
|
Test failures are known issues |
Member
Author
|
/azp list |
Member
Author
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This was referenced Mar 20, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.