swift-foundation icon indicating copy to clipboard operation
swift-foundation copied to clipboard

[Bug] `superDecoder(forKey:)` doesn’t work as documented

Open soumyamahunt opened this issue 2 years ago • 4 comments

When trying to decode a Codable model with following implementation:

struct SomeCodable {
    let value1: String?
}

extension SomeCodable: Decodable {
    init(from decoder: any Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        if let key1_containerDecoder = try? container.superDecoder(forKey: CodingKeys.key1) {
            let key1_container = try key1_containerDecoder.container(keyedBy: CodingKeys.self)
            self.value1 = try key1_container.decodeIfPresent(String.self, forKey: CodingKeys.value1)
        } else {
            self.value1 = nil
        }
    }
}

extension SomeCodable: Encodable {
    func encode(to encoder: any Encoder) throws {
        var container = encoder.container(keyedBy: CodingKeys.self)
        var key1_container = container.nestedContainer(keyedBy: CodingKeys.self, forKey: CodingKeys.key1)
        try key1_container.encodeIfPresent(self.value1, forKey: CodingKeys.value1)
    }
}

extension SomeCodable {
    enum CodingKeys: String, CodingKey {
        case value1 = "key2"
        case key1 = "key1"
    }
}

and empty JSON object({}) error is thrown at

let key1_container = try key1_containerDecoder.container(keyedBy: CodingKeys.self)

valueNotFound(Swift.KeyedDecodingContainer<MetaCodableTests.SomeCodable.CodingKeys>, Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "key1", intValue: nil)], debugDescription: "Cannot get keyed decoding container -- found null value instead.", underlyingError: nil))"

instead of

if let key1_containerDecoder = try? container.superDecoder(forKey: CodingKeys.key1) 

which is diferrent than documented.

As per discussion in forum, both older and new Foundation implementation seems to have this bug.

soumyamahunt avatar Nov 13 '23 08:11 soumyamahunt

Yep, this behaves counter to the documentation. And failing sooner does make sense to me as opposed to deferring it.

The forum thread discusses the low likelihood of regression from clients relying on the existing behavior. Normally I'd tend to agree, but during the swift-foundation reimplementation of the entire API surface, I was repeatedly struck by breakages from odd client behavior and expectation. There's a large number of clients of this API, so even a small percentage chance of this happening makes the expected number of real-world regressions probably non-zero.

kperryua avatar Nov 13 '23 18:11 kperryua

Agreed! The important thing is to match the behaviour from macOS 13 and earlier. (rdar://118337978, for reference.)

glessard avatar Nov 13 '23 18:11 glessard

And that is indeed what's happening here. Again, I'm not opposed to trying this out, but we may end up being forced to explore compatibility techniques to push the change through for the general population.

kperryua avatar Nov 13 '23 19:11 kperryua

@kperryua @glessard meanwhile is there any current workaround that can be used to have the functionality mentioned in the docs?

soumyamahunt avatar Nov 14 '23 01:11 soumyamahunt