feat: make LDValue hashable - #519
Open
abelonogov-ld wants to merge 1 commit into
Open
Conversation
An LDValue could be compared but not hashed, so anything holding one in a dictionary or a set had to walk the enum and hash each case by hand. The conformance is synthesized, so it agrees with the equality that was already synthesized alongside it, including an object hashing the same whichever order its keys were written in. Hashable refines Equatable, so this replaces that conformance rather than adding to it. Co-authored-by: Cursor <cursoragent@cursor.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Requirements
Related issues
Split out of #516, which needs to hash a flag value as part of an evaluation exposure key. That PR is stacked on this one.
Describe the solution you've provided
LDValuewasEquatablebut notHashable, so anything wanting to hold one in a dictionary or a set had to switch over the enum and hash each case by hand — which #516 was doing, including sorting object keys so that a map's iteration order could not change the hash.Every payload
LDValuecarries is alreadyHashableonceLDValueitself is ([LDValue]and[String: LDValue]conform when their elements do), so the conformance is synthesized. That matters for correctness: it agrees by construction with theEquatableconformance, which was also synthesized, rather than being a second hand-written definition of the same thing that could drift from it.HashablerefinesEquatable, so this replacesEquatablein the conformance list rather than adding to it. Adding a conformance to a public type is source compatible.Describe alternatives you've considered
hash(into:)by hand. It's more code, and it has to be kept in step with equality; the synthesized pair cannot disagree.LDValuealone and keeping the hand-rolled hashing in the deduper. That leaves the same work for the next caller that wants to key something by a flag value, and each copy has to remember details like object key ordering.Additional context
LDValueSpeccovers equal values hashing alike, an object hashing the same whichever order its keys were written in, values of different kinds staying distinct (including pairs a naive payload-only hash would collide, such as.bool(false)against.null), and a value keying a dictionary.Full suite passes: 597 tests.
Made with Cursor