Ensure deterministic ordering for Dictionary keys#22
Ensure deterministic ordering for Dictionary keys#22rickhohler wants to merge 5 commits intotoon-format:mainfrom
Dictionary keys#22Conversation
mattt
force-pushed
the
fix/deterministic-key-sorting
branch
from
60a3cfa to
7c9543c
Compare
| // Swift Dictionary encoding uses an internal DictionaryCodingKey; detect it to sort keys deterministically. | ||
| private let isDictionaryCodingKey = String(reflecting: Key.self).contains("DictionaryCodingKey") |
There was a problem hiding this comment.
Kind of hacky, but it lets us have nice behavior where Dictionary keys are sorted but struct fields retain ordering.
mattt
changed the title
Dictionary keys
| // MARK: - Factory | ||
|
|
||
| /// Creates a `Value` from an arbitrary value. | ||
| static func from(_ value: Any) -> Value { |
There was a problem hiding this comment.
Turns out, we weren't using this anywhere 🫠
|
Thanks for your help with this, @rickhohler. I think I ended up rewriting all of the original PR, but the good news is that we now have deterministic ordering when encoding objects / key-value pairs (sorted for dictionaries, in declaration / encoding order for structures) |
There was a problem hiding this comment.
Pull request overview
This PR implements deterministic alphabetical ordering for Dictionary keys in the TOONEncoder to ensure consistent serialization output. This was separated from PR #20 which addressed both dictionary ordering and negative zero preservation.
Changes:
- Adds detection logic to identify Swift Dictionary encoding via reflection on internal CodingKey type
- Sorts dictionary keys alphabetically when detected, while preserving field order for regular Encodable structs
- Removes unused
Value.from(_:)factory method that previously handled dictionaries differently - Updates README to document the dictionary key sorting behavior
- Adds comprehensive test with 30 permutations to verify deterministic ordering
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Sources/ToonFormat/Encoder.swift | Adds isDictionaryCodingKey detection and applies alphabetical sorting to dictionary keys in finishEncoding and deinit |
| Sources/ToonFormat/Value.swift | Removes unused Value.from(_:) static factory method (92 lines removed) |
| Tests/ToonFormatTests/EncoderTests.swift | Adds dictionaryKeyOrderingIsDeterministic test with 30 rotation permutations |
| README.md | Documents that Dictionary keys are sorted while Encodable field order is preserved |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| private var container: [String: Value] = [:] | ||
| private var keyOrder: [String] = [] // Track insertion order | ||
| // Swift Dictionary encoding uses an internal DictionaryCodingKey; detect it to sort keys deterministically. |
There was a problem hiding this comment.
The detection of Dictionary encoding via String reflection is fragile and relies on internal Swift implementation details. The string "DictionaryCodingKey" is an internal Swift type name that could change in future Swift versions, breaking this functionality silently.
Consider these alternatives:
- Document this as a known limitation that depends on Swift internals
- Add a test that explicitly verifies the reflection string contains "DictionaryCodingKey" when encoding a Dictionary, so that any Swift version upgrade that breaks this will be caught
- Explore whether there's a more robust way to differentiate Dictionary encoding from struct encoding (though this may not be possible with the Codable API)
Given that this is a critical feature for deterministic output, it would be prudent to have explicit test coverage that verifies the detection mechanism is working as expected.
| // Swift Dictionary encoding uses an internal DictionaryCodingKey; detect it to sort keys deterministically. | |
| /// Heuristic: Swift's `Dictionary` encoding uses an internal `DictionaryCodingKey` type. | |
| /// | |
| /// We detect this by checking whether `String(reflecting: Key.self)` contains | |
| /// the substring `"DictionaryCodingKey"`, and if so, we treat the container | |
| /// as a dictionary and sort its keys deterministically. | |
| /// | |
| /// This relies on Swift's internal implementation details and is therefore | |
| /// inherently fragile: if the type name changes in a future Swift version, | |
| /// this detection will stop working and dictionary key ordering may become | |
| /// non-deterministic again without a compile-time error. | |
| /// | |
| /// This limitation should be documented and covered by tests that assert | |
| /// the presence of `"DictionaryCodingKey"` when encoding a `Dictionary`, | |
| /// so that Swift upgrades which change the internal type name are caught. |
| let finalKeyOrder = isDictionaryCodingKey ? container.keys.sorted() : keyOrder | ||
| encoder.storage.append(.object(container, keyOrder: finalKeyOrder)) | ||
| } | ||
|
|
||
| deinit { | ||
| // Ensure the container is finished when it goes out of scope | ||
| encoder.storage.append(.object(container, keyOrder: keyOrder)) | ||
| let finalKeyOrder = isDictionaryCodingKey ? container.keys.sorted() : keyOrder | ||
| encoder.storage.append(.object(container, keyOrder: finalKeyOrder)) |
There was a problem hiding this comment.
The same logic for determining finalKeyOrder is duplicated in both finishEncoding and deinit. Consider extracting this into a computed property or private method to reduce duplication and improve maintainability. For example:
private var finalKeyOrder: [String] {
isDictionaryCodingKey ? container.keys.sorted() : keyOrder
}This would make both methods simpler and ensure the logic stays consistent if it needs to be updated in the future.
|
Closing in favor of #25 |
Separated from #20 per review feedback.