tgrapperon / swift-dependencies-additions

More dependencies for `swift-dependencies`
MIT License
298 stars 39 forks source link

Addressing Crashes with Non-Equatable Any Values #73

Closed HugoSay closed 1 year ago

HugoSay commented 1 year ago

Description:

This pull request addresses an issue that was introduced in #53, where our codebase experienced crashes in tests for Any values that were not Equatable.

The root cause of these crashes is our usage of the library's internal properties, specifically the set(_ object: Any, forKey: String) function.

This pull request does not make the set(_ object: Any, forKey: String) function public. However, it serves as a first step towards considering this change. Making this function public could align the library with the functionality provided by native UserDefaults, keeping in mind that the responsibility for ensuring that objects are Plist-representable rests with the end user.

tgrapperon commented 1 year ago

Hey @HugoSay! Thanks for the report and the PR. I'm rather surprised that you were able to send values that are not Equatable, as the only public API are the specialized ones AFAIK. Are you using the dependency with legacy values that were written with a default UserDefaults? The general func set<T: Sendable>(_ value: T?, forKey key: String) is normally hidden by @_spi (because it is used by the AppStorage experimental dependency that lives in another module), so I'm not eager to introduce a performance regression for uses that where not planned by the public APIs. But we can maybe find a middle ground to handle your cases if you can give me an example. My plan for the dependency was to support the subset of data SwiftUI exposes through AppStorage, but of course that can be discussed. I'm just trying to see in which circumstances the crash can happen.

HugoSay commented 1 year ago

As I mentioned in the description, I did have to use the internal method when I did the implementation on my project. 🙈

@_spi(Internals) import UserDefaultsDependency

@propertyWrapper
public struct UserDefault<Value: Sendable> {
    let key: String
    let defaultValue: Value
    @Dependency(\.projectUserDefaults) var container

    public var wrappedValue: Value {
        get {
            let object: Value? = container.object(forKey: key)
            return object ?? defaultValue
        }
        set {
            container.set(newValue, forKey: key)
        }
    }
}

extension UserDefaults {
    @UserDefault(key: "nb_max_requests_of_notification", defaultValue: 0)    
    static var nbMaxRequestsOfNotification: Int
    // or in our specific crashing case
    @UserDefault(key: "nb_max_requests_of_notification", defaultValue: nil)    
    static var someStoredEvent: [[String: Any]]
}

We use this kind of logic in our project to have complex typed user defaults that we can use in the project.

One way to work around the issue on our side would be to use the .set(_ value: Data, forKey:) and encode any complex value with codable, but it wouldn't be backward compatible.

To be honest, I'm not sure we should use Userdefaults to store complex data and I'm sure we can come up with a better solution. I initially figured it was fine to use the .set(_ value: Any?, forKey:) because it's a native UserDefaults method.