`superDecoder(forKey:)` doesn't work as documented

Development Core Libraries
, ,
1

I am 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"
    }
}

If I try to decode this model from empty JSON object({}) I get error at this line:

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))"

But as superDecoder(forKey:) documented, shouldn't the following condition have failed resulting in value1 being assigned nil?:

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

Am I missing something here or is this a bug?

2

Yes, as documented, this should throw — right now, the implementation returns a decoder without checking for the presence of the key first:

private func _superDecoder(forKey key: __owned CodingKey) throws -> Decoder {
    self.decoder.codingPath.append(key)
    defer { self.decoder.codingPath.removeLast() }

    let value: Any = self.container[key.stringValue] ?? NSNull() // <- should throw instead of substituting `NSNull`
    return __JSONDecoder(referencing: value, at: self.decoder.codingPath, options: self.decoder.options)
}

I'd consider this a bug worth a report.

FWIW, the upcoming swift-foundation implementation appears to behave the same right now, so doubly-worth a report there while the implementation is still being built out:

func superDecoder(forKey key: K) throws -> Decoder {
    return decoderForKeyNoThrow(key)
}

private func decoderForKeyNoThrow(_ key: some CodingKey) -> JSONDecoderImpl { 
    let value: JSONMap.Value
    do {
        value = try getValue(forKey: key)
    } catch {
        // if there no value for this key then return a null value
        value = .null
    }
    let impl = JSONDecoderImpl(userInfo: self.impl.userInfo, from: self.impl.jsonMap, codingPathNode: self.codingPathNode.pushing(key), options: self.impl.options)
    impl.push(value: value)
    return impl
}

(cc @jmschonfeld)

3 Likes
3

Thanks @itaiferber, I have reported this bug in both the repos:

1 Like
4

Thanks for the heads up @itaiferber! I agree it does look like the behavior doesn't match our documentation so we should ensure one or the other is updated so they match. I'll make sure I bring this up to Charles and Kevin who have been working in this area recently.

1 Like
5

Thanks, @jmschonfeld! FWIW, I'd lean toward updating the implementation to match the documentation (and consider my original implementation a bug); I can't imagine any significant backwards-compatibility concerns (superDecoder(...) is already pretty niche), so it might be better to match the docs. (Though of course, that's up to the team to explore.)

2 Likes
6

Yeah that makes sense to me as well. cc @glessard since you mentioned to me you might take this one on