search

readium / swift-toolkit

A toolkit for ebooks, audiobooks and comics written in Swift
https://readium.org/mobile/
BSD 3-Clause "New" or "Revised" License
222 stars 96 forks source link

First pass at marking types as Sendable #450

Closed domkm closed 3 weeks ago

domkm commented 4 weeks ago

Related to https://github.com/readium/swift-toolkit/discussions/433

domkm commented 4 weeks ago

I used @unchecked Sendable on JSONDictionary because of the underlying [String:Any] dictionary. @unchecked can be removed if we change the underlying to [String:Sendable].

domkm commented 3 weeks ago

I don't plan to merge strict-concurrency into develop unless we can get rid of all the warnings. I guess the warnings are still there in your PR? If yes, could you rebase to develop without including the changes from strict-concurrency?

Sure, I can rebase it on develop if you'd prefer, but may I ask why you don't want to merge with strict concurrency warnings? The warnings highlight potential bugs, many of which are errors in Swift 6. In other words, hiding the warnings just hides issues but doesn't change behavior.

Using [String: Sendable] is fine by me, it's supposed to contain only JSON values anyway (String, Int, etc.), so Sendable seems appropriate.

Cool. Should it be Codable too?

mickael-menu commented 3 weeks ago

but may I ask why you don't want to merge with strict concurrency warnings? The warnings highlight potential bugs, many of which are errors in Swift 6. In other words, hiding the warnings just hides issues but doesn't change behavior.

We have a strict no warnings policy to merge things in the repo (that's why your PR fails all the checks). The goal is to progressively address these issues before Swift 6, without allowing other types of warnings to slip through.

Cool. Should it be Codable too?

Yes that's a good idea. 👍

domkm commented 3 weeks ago

Unfortunately, I wasn't able to make JSONDictionary values Codable without cascading issues, as there are many places that assume it can contain NSNull and nil.

I'm also curious, what is the purpose of this JSONDictionary encoding/decoding scheme instead of using Codable directly?

mickael-menu commented 3 weeks ago

I'm also curious, what is the purpose of this JSONDictionary encoding/decoding scheme instead of using Codable directly?

I don't remember exactly, but it might be because we use JSONSerialization.jsonObject() to parse JSON, which returns Any and can't be casted to [String: Codable].

domkm commented 3 weeks ago

I don't remember exactly, but it might be because we use JSONSerialization.jsonObject() to parse JSON, which returns Any and can't be casted to [String: Codable].

Would it make sense to make these types, or at least the main ones, Codable? In my project, I extended Locator and Manifest to be Codable, and I would guess that almost every project at least extends Locator.

mickael-menu commented 3 weeks ago

Would it make sense to make these types, or at least the main ones, Codable? In my project, I extended Locator and Manifest to be Codable, and I would guess that almost every project at least extends Locator.

Sure, they are all meant to be serializable to JSON. JSONDictionary itself could be Codable using a custom implementation.

Note that many of the models have complex (de)serialization rules so we can't use the default Codable implementation.