fix: Add more Hashable conformance - #737
Conversation
gavirawson-apple
left a comment
gavirawson-apple
left a comment
There was a problem hiding this comment.
Looking great, Hashable conformance will make handling these data structures much easier. I think we just need to remove the Identifiable conformance before merging. Let's brainstorm the best way to move towards Identifiable conformance without negatively impacting developers.
|
|
||
| /// Conforming a type to `OCKAnyTask` allows it to be queried and displayed by CareKit. | ||
| public protocol OCKAnyTask: Sendable { | ||
| public protocol OCKAnyTask: Identifiable, Sendable { |
There was a problem hiding this comment.
We have long-standing problem ever since the Identifiable was introduced - our id property isn't a stable ID for different versions of the same task, but our uuid is. I think that means we can't conform OCKAnyTask to Identifiable directly, the best you can do right now is wrap OCKAnyTask in an Identifiable type.
There was a problem hiding this comment.
I see, I’ll make an update soon.
Removed Identifiable conformance from OCKAnyTask protocol.
cbaker6
changed the title
There was a problem hiding this comment.
Pull request overview
Extends Hashable support across additional CareKitStore types to improve SwiftUI interoperability (per #726, continuing #736).
Changes:
- Make
OCKEventgeneric parametersHashableand addHashableconformance toOCKEvent. - Tighten store protocol associated-type constraints so
Task/Outcomemust beHashable. - Add
Hashableconformance toOCKHealthKitOutcome.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| CareKitStore/CareKitStore/Structs/OCKEvent.swift | Requires Task/Outcome to be Hashable and makes OCKEvent itself Hashable. |
| CareKitStore/CareKitStore/Protocols/Tasks/OCKTaskStore.swift | Updates OCKReadableTaskStore’s Task associated type constraint to Hashable. |
| CareKitStore/CareKitStore/Protocols/Outcomes/OCKOutcomeStore.swift | Updates OCKReadableOutcomeStore’s Outcome associated type constraint to Hashable. |
| CareKitStore/CareKitStore/HealthKit/OCKHealthKitOutcome.swift | Adds Hashable conformance to OCKHealthKitOutcome. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public struct OCKEvent< | ||
| Task: OCKAnyTask & Equatable, | ||
| Outcome: OCKAnyOutcome & Equatable | ||
| >: Identifiable, Comparable, Sendable { | ||
| Task: OCKAnyTask & Hashable, | ||
| Outcome: OCKAnyOutcome & Hashable | ||
| >: Identifiable, Hashable, Comparable, Sendable { |
There was a problem hiding this comment.
OCKEvent now conforms to Hashable, but the synthesized Hashable/Equatable implementation will include outcome (a mutable stored property). That means mutating outcome changes the hash/equality, which can break Set/Dictionary invariants and can cause surprising behavior when OCKEvent is used as a SwiftUI navigation value. Consider implementing custom ==/hash(into:) based only on stable identity (e.g., task UUID + occurrence) or making outcome immutable.
| public struct OCKEvent< | ||
| Task: OCKAnyTask & Equatable, | ||
| Outcome: OCKAnyOutcome & Equatable | ||
| >: Identifiable, Comparable, Sendable { | ||
| Task: OCKAnyTask & Hashable, | ||
| Outcome: OCKAnyOutcome & Hashable | ||
| >: Identifiable, Hashable, Comparable, Sendable { | ||
|
|
There was a problem hiding this comment.
OCKEvent.id is documented as a “stable identifier”, but it’s currently derived from Hasher.finalize(). Swift’s Hasher is intentionally seeded per-process, so this value is not stable across app launches and can also theoretically collide. For a stable ID (and to better support SwiftUI use-cases), prefer a deterministic string derived from task.uuid and scheduleEvent.occurrence rather than Hasher output.
|
|
||
| associatedtype Task: OCKAnyTask, Equatable | ||
| associatedtype Task: OCKAnyTask, Hashable | ||
|
|
There was a problem hiding this comment.
Changing OCKReadableTaskStore.Task from Equatable to Hashable is a source-breaking change for any external store implementations that previously only made their task Equatable. If the intent is to add SwiftUI support without breaking existing conformers, consider keeping the base protocol requirement as Equatable and introducing a separate/refinement protocol (or conditional extensions) for Hashable-specific APIs.
| public protocol OCKReadableOutcomeStore: OCKAnyReadOnlyOutcomeStore { | ||
|
|
||
| associatedtype Outcome: OCKAnyOutcome, Equatable, Identifiable | ||
| associatedtype Outcome: OCKAnyOutcome, Hashable, Identifiable |
There was a problem hiding this comment.
Changing OCKReadableOutcomeStore.Outcome from Equatable to Hashable is a source-breaking change for external outcome store implementations that only provided Equatable outcomes. If avoiding breaking changes is important, consider leaving this protocol as-is and adding a new/refinement protocol that requires Hashable (or a where Outcome: Hashable constrained extension) for SwiftUI-facing APIs.
| associatedtype Outcome: OCKAnyOutcome, Hashable, Identifiable | |
| associatedtype Outcome: OCKAnyOutcome, Equatable, Identifiable |
Close #726
Continuation on #736