Add logic to properly honor naming policy when serializing flag enums

[layomia]

Fixes #31622.

[steveharter]

Since naming policies are only one-way the resulting JSON that had a naming policy applied may not be able to be deserialized unless we also apply the naming policy to the CLR enum value names during deserialization (and match them up that way -- like we do with properties).

[layomia]

Since naming policies are only one-way the resulting JSON that had a naming policy applied may not be able to be deserialized unless we also apply the naming policy to the CLR enum value names during deserialization (and match them up that way -- like we do with properties).

We already apply the naming policy when serializing, just incorrectly in the case of flags.

This change doesn't prohibit any deserialization that was possible prior:

Serializing Compressed | IntegrityStream with camel case

Before: "compressed, IntegrityStream"
Now: "compressed, integrityStream"

Round tripping is not broken here because we do a case insensitive read with Enum.Parse.

Serializing Compressed | IntegrityStream with a custom snake case policy

Before: "compressed, _integrity_stream"
After: "compressed, integrity_stream"

Neither of these strings can be read by Enum.Parse, as it expects some ordering of "Compressed, IntegrityStream". So, we are not breaking deserialization here, as it never worked for this scenario.

Ultimately, correctly applying the naming won't affect reading, given the current deserialization implementation. The change is a prerequisite if we want to recognize the policy during deserialization (#31619) to enable roundtripping when a naming policy is used (one which does more than return a case-insensitive match).

[steveharter]

Round tripping is not broken here because we do a case insensitive read with Enum.Parse.

That works with camel-casing but not with snake-casing...

[layomia]

Round tripping is not broken here because we do a case insensitive read with Enum.Parse.

That works with camel-casing but not with snake-casing...

Yes, but we already honor snake-casing when serializing, and then can't deserialize the output:

JsonSerializerOptions options = new JsonSerializerOptions { Converters = { new JsonStringEnumConverter(namingPolicy: new SimpleSnakeCasePolicy())} };
FileAttributes val = FileAttributes.IntegrityStream;

string json = JsonSerializer.Serialize(val, options);
Console.WriteLine(json); // "integrity_stream"

JsonSerializer.Deserialize<FileAttributes>(json, options); // JsonException

I'll reach out to discuss this offline.

[layomia]

Benchmarks for this change show serialization when a naming policy is used is up to ~2.48x faster when we have long enum representation. We also have less allocs (up to ~47% decrease) in these scenarios. This is due to creating and caching the JsonEncodedText rather just the result of Enum.ToString. Also, we now key on the enum value, rather than the string representation, so we don't have to call Enum.ToString() on every serialize.

Before

Method	enumValue	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
NoPolicy	Compressed	153.4 ns	0.94 ns	0.79 ns	153.0 ns	152.6 ns	155.1 ns	0.0434	-	-	184 B
CamelCasePolicy	Compressed	241.0 ns	2.34 ns	2.07 ns	241.1 ns	236.8 ns	243.8 ns	0.0509	-	-	216 B
SnakeCasePolicy	Compressed	241.9 ns	1.58 ns	1.48 ns	241.9 ns	239.6 ns	244.1 ns	0.0507	-	-	216 B
NoPolicy	Directory, Co(...)tegrityStream [38]	159.7 ns	0.32 ns	0.27 ns	159.7 ns	159.4 ns	160.3 ns	0.0437	-	-	184 B
CamelCasePolicy	Directory, Co(...)tegrityStream [38]	305.6 ns	1.70 ns	1.59 ns	305.2 ns	303.3 ns	308.6 ns	0.0815	-	-	344 B
SnakeCasePolicy	Directory, Co(...)tegrityStream [38]	308.2 ns	0.95 ns	0.84 ns	308.4 ns	306.4 ns	309.4 ns	0.0834	-	-	352 B
NoPolicy	ReadOnly, Hi(...) NoScrubData [100]	154.3 ns	0.70 ns	0.66 ns	154.1 ns	153.3 ns	155.6 ns	0.0433	-	-	184 B
CamelCasePolicy	ReadOnly, Hi(...) NoScrubData [100]	428.8 ns	2.05 ns	1.82 ns	429.2 ns	425.8 ns	431.9 ns	0.1258	-	-	528 B
SnakeCasePolicy	ReadOnly, Hi(...) NoScrubData [100]	432.4 ns	3.42 ns	3.20 ns	430.8 ns	428.9 ns	438.8 ns	0.1286	-	-	544 B

After

Method	enumValue	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
NoPolicy	Compressed	151.0 ns	1.59 ns	1.32 ns	151.5 ns	147.9 ns	152.7 ns	0.0440	-	-	184 B
CamelCasePolicy	Compressed	160.9 ns	0.66 ns	0.58 ns	160.8 ns	160.2 ns	162.1 ns	0.0454	-	-	192 B
SnakeCasePolicy	Compressed	165.2 ns	2.77 ns	2.59 ns	166.3 ns	160.5 ns	168.0 ns	0.0457	-	-	192 B
NoPolicy	Directory, Co(...)tegrityStream [38]	158.0 ns	1.17 ns	1.03 ns	157.7 ns	156.9 ns	159.9 ns	0.0438	-	-	184 B
CamelCasePolicy	Directory, Co(...)tegrityStream [38]	162.5 ns	0.84 ns	0.79 ns	162.3 ns	161.4 ns	164.0 ns	0.0512	-	-	216 B
SnakeCasePolicy	Directory, Co(...)tegrityStream [38]	159.8 ns	0.62 ns	0.58 ns	159.9 ns	159.0 ns	161.0 ns	0.0531	-	-	224 B
NoPolicy	ReadOnly, Hi(...) NoScrubData [100]	156.4 ns	0.80 ns	0.71 ns	156.5 ns	155.2 ns	157.6 ns	0.0433	-	-	184 B
CamelCasePolicy	ReadOnly, Hi(...) NoScrubData [100]	175.5 ns	0.58 ns	0.54 ns	175.3 ns	174.6 ns	176.3 ns	0.0667	-	-	280 B
SnakeCasePolicy	ReadOnly, Hi(...) NoScrubData [100]	167.0 ns	1.18 ns	1.10 ns	167.0 ns	164.8 ns	168.5 ns	0.0683	-	-	288 B

[layomia]

Updated benchmark results following latest changes:

Method	enumValue	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
NoPolicy	Compressed	149.5 ns	1.03 ns	0.91 ns	149.1 ns	148.4 ns	151.2 ns	0.0439	-	-	184 B
CamelCasePolicy	Compressed	175.0 ns	2.47 ns	2.19 ns	174.8 ns	172.2 ns	179.5 ns	0.0510	-	-	216 B
SnakeCasePolicy	Compressed	172.6 ns	1.30 ns	1.22 ns	172.9 ns	170.8 ns	174.5 ns	0.0514	-	-	216 B
NoPolicy	Directory, Co(...)tegrityStream [38]	151.5 ns	2.91 ns	2.72 ns	150.3 ns	149.0 ns	157.3 ns	0.0437	-	-	184 B
CamelCasePolicy	Directory, Co(...)tegrityStream [38]	173.1 ns	1.40 ns	1.24 ns	173.1 ns	171.5 ns	175.3 ns	0.0570	-	-	240 B
SnakeCasePolicy	Directory, Co(...)tegrityStream [38]	174.6 ns	1.19 ns	1.11 ns	175.0 ns	172.5 ns	176.4 ns	0.0587	-	-	248 B
NoPolicy	ReadOnly, Hi(...) NoScrubData [100]	154.1 ns	0.67 ns	0.63 ns	154.1 ns	153.3 ns	155.5 ns	0.0435	-	-	184 B
CamelCasePolicy	ReadOnly, Hi(...) NoScrubData [100]	181.4 ns	0.29 ns	0.24 ns	181.4 ns	181.0 ns	181.9 ns	0.0723	-	-	304 B
SnakeCasePolicy	ReadOnly, Hi(...) NoScrubData [100]	179.3 ns	1.63 ns	1.36 ns	180.0 ns	177.3 ns	180.9 ns	0.0743	-	-	312 B

Changes have been addressed.

[GrabYourPitchforks]

It's incorrect to cache any thread-local state (such as information about the thread-current culture) into a global static. The end result of this is that the very first time this code is executed, the executing thread's negative sign will be read and will be applied to all subsequent operations, regardless of what culture the other threads are running as.

If you wanted this to be properly culture-aware, you need to query the current thread's negative sign on every single call to IsValidIdentifier.

[danmoseley]

This would be incorrect even if it was a thread-static, of course.

[GrabYourPitchforks]

LGTM, thanks! :)
I also left a comment pointing out a bug that existed prior to this PR. We should fix it next time we're in this code.