[release/9.0-staging] [STJ] Fix formatting of flag enums when values are overlapping.#117806
Closed
github-actions[bot] wants to merge 4 commits into
Closed
[release/9.0-staging] [STJ] Fix formatting of flag enums when values are overlapping.#117806github-actions[bot] wants to merge 4 commits into
github-actions[bot] wants to merge 4 commits into
Conversation
eiriktsarpalis
added
area-System.Text.Json
Servicing-consider
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
This was referenced Jul 18, 2025
PranavSenthilnathan
approved these changes
Jul 18, 2025
Member
Member
|
Before merging, let's also cherry pick the improvement from #117839 once merged. cc @PranavSenthilnathan EDIT: now done. |
* [STJ] Only compute PopCount once when topologically sorting Enums Packs the calculated (once per entry) PopCount into the high 32 bits of the long and the original index into the low 32 bits. Then that can just be sorted using the heavily optimized Array.Sort() method. After sorting just extract the low 32 bits as the original array index. As before, we negate the actual _PopCount_ to ensure that `Key`s with more on-bits (e.g. more flags represented) will sort **first**. This trades 2 x O(N log N) [average case] to 2 x O(N^2) [worst case] calls to the `popcount` instruction (or the emulation if NET is not defined) for N **shift-left-32** and **or** + N x **truncate to 32 bits**. It _also_ eliminates the overhead of the `CompareTo` method as it's now a direct `long` low-level compare. * PR Feedback Switched to Tuple * PR Feedback Switched to native tuple syntax Co-authored-by: Pranav Senthilnathan <pranav.senthilnathan@live.com> * Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs --------- Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
PranavSenthilnathan
approved these changes
Jul 23, 2025
jeffhandley
approved these changes
Jul 24, 2025
jeffhandley
added
Servicing-rejected
and removed
Servicing-consider
Member
|
We are going to hold off on backporting this to .NET 9. If more customer impact is reported before migrating to .NET 10 is an option, we will reconsider the backport. |
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.
5 participants
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.
Backport of #117730 to release/9.0-staging
/cc @eiriktsarpalis
Customer Impact
Fixes a regression introduced in .NET 9 impacting formatting of enum flag types as strings, when the enums define values whose bits are a subset of other values. This results in the enum either being formatted as a non-canonical combination of values or being formatted using its underlying numeric value.
Regression
The regression was introduced by #105032, which added support for customizable enum labels. This necessitated a departure from the built-in string formatting for enums in favor of a custom solution, which has created a bug tail.
Testing
New test cases were added for
[Flags]scenarios with overlapping values.Risk
Low. The fix involves topologically sorting of the built-in enum values which changes the string-based formatting of enum values but doesn't impact the underlying value being marshalled (as likewise the regression didn't impact the underlying value either).