Order of serialized properties determined by dictionary enumeration on fast-path source generators

[eiriktsarpalis]

Currently, the fast-path source generator will use the TryFilterSerializableProps method to determine the properties that are serializable. The results are returned as a dictionary, which the source generator will enumerate in order to emit the relevant logic.

Note that dictionary enumeration does not provide any guarantees on ordering, as such it is likely that this could end up not honoring the desired serialization order (e.g. as specified by the JsonPropertyOrderAttribute). That being said, the current implementation of System.Collections.Generic.Dictionary happens to preserve ordering due to an implementation detail that may well change in the future. We should update the TryFilterSerializableProps method to provide more robust guarantees on property ordering.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Currently, the fast-path source generator will use the TryFilterSerializableProps method to determine the properties that are serializable. The results are returned as a dictionary, which the source generator will enumerate in order to emit the relevant logic.

Note that dictionary enumeration does not provide any guarantees on ordering, as such it is likely that this could end up not honoring the desired serialization order (e.g. as specified by the JsonPropertyOrderAttribute). That being said, the current implementation of System.Collections.Generic.Dictionary happens to preserve ordering due to an implementation detail that may well change in the future. We should update the TryFilterSerializableProps method to provide more robust guarantees on property ordering.

Author:	eiriktsarpalis
Assignees:	-
Labels:	enhancement, area-System.Text.Json
Milestone:	7.0.0

[eiriktsarpalis]

Given that the source generator targets netstandard2.0, for the 6.0 release we might want to check if this is honored in older implementations of Dictionary, up to and including the netfx implementation.

[danmoseley]

I wonder whether when built in DEBUG we should have our dictionary randomize its enumeration. Long ago, before string hash codes were randomized by default, the libraries used to randomize string hashcodes (XOR them with the build number or similar) in DEBUG only.

[GSPP]

@danmoseley I have worked on a large application that relied on Dictionary usually being ordered in about 1000 places... I am a fan of intentionally breaking behavior that is not guaranteed but I think that really has to happen in v1 or else it's an upgrade pain.

[danmoseley]

@GSPP in general no framework can reasonably guarantee undefined behavior, post v1 or not. It so heavily constrains maintenance, perf improvements, etc. As well we do not test such behavior - so we do not necessarily know when it changes - nor necessarily know that anyone depends on it. That may turn up much later when the product is in servicing mode. We have seen the result of this in .NET Framework where callers even depended on details of timing to the extent performance improvements caused breaks and had to be reverted later. .NET Core introduced SxS deployment to give more freedom to make improvements like this.

There can always be exceptions where something is so widely assumed in the ecosystem that it is best to document it. I do not have the data to guess whether this is the case here or not but my guess is that it is unlikely.