Fix JSONDecoder superDecoder darwin/linux discrepancy

[adam-fowler]

On Darwin when you call superDecoder on the JSONDecoder keyed container and there is no associated value for the key passed in, it returns a new decoder that returns empty containers. On Linux it would throw a DecodingError.keyNotFound error.

This PR resolves this discrepancy by catching the DecodingError.keyNotFound error and returning a decoder that references a JSONValue.null.

The PR also adds a new test test_notFoundSuperDecoder to verify this fix.

This resolves #3169

cc: @fabianfett

[fabianfett]

Great fix! Thank you so much! @millenomi how can we get this into 5.7 and a 5.6 patch release?

[compnerd]

@swift-ci please test

[millenomi]

@swift-ci please test

[tomerd]

@adam-fowler this would be nice to get into 5.6.3: https://forums.swift.org/t/development-open-for-swift-5-6-3-for-linux-and-windows/58859

Can we get CI green so its mergable?

cc @parkera @shahmishal

[adam-fowler]

Hi @tomerd I haven't had time to look into this. But I think the workaround that @ahoppen put in SourceKit-LSP to get around the issue this was fixing may have broken now it is fixed.

[adam-fowler]

Ok had a closer look at this. It isn't the workaround that is causing the issue. It is the initialisation of a SourceKit-LSP type CancelRequestNotification from the null decoder I return from superDecoder() that throws a different error to Darwin. Darwin throws a DecodingError.valueNotFound while my change causes a DecodingError.typeMismatch error to be thrown. Unfortunately I'm not sure I can construct a decoder that will cause the current CoreFoundation JSONDecoder to throw the error expected by SourceKit-LSP ie DecodingError.valueNotFound

The null decoder current wraps a .null value so when you try to access any values using the superDecoder it returns a typeMismatch error. I could instead return an empty dictionary which might return a more sensible error message ie DecodingError.keyNotFound. It is still not the same as Darwin though. This would actually fix the SourceKit-LSP test case because it converts both keyNotFound and valueNotFound into the same error, but wouldn't be correct (where correct means the same as Darwin).

@fabianfett, @ahoppen what are your thoughts.

[ahoppen]

Looking at the issue you describe, I think that a keyNotFound error might actually be the correct error though and we might want to consider the macOS implementation buggy, because the issue here is that the key is missing, not that there’s a key, which was expected to have a non-null value but had a null value (that’s how I read the valueNotFound documentation).

[iCharlesHu]

IMO DecodingError.valueNotFound is the correct error type here. Consider this example:

func testSuperDecoder() {
    struct OuterType : Decodable {
        init(from decoder: Decoder) throws {
            let container = try decoder.container(keyedBy: CodingKeys.self)
            _ = try InnerType(from: container.superDecoder(forKey: .superDecoder))
        }

        private enum CodingKeys: String, CodingKey {
            case superDecoder = "super"
        }
    }

    struct InnerType : Decodable {
        init(from decoder: Decoder) throws {
            let container = try decoder.container(keyedBy: CodingKeys.self)    <----------
            _ = try container.decode(Bool.self, forKey: .whatever)
        }

        private enum CodingKeys : String, CodingKey {
            case whatever
        }
    }

    let decoder = JSONDecoder()
    do {
        let _ = try decoder.decode(OuterType.self, from: Data("{}".utf8))
    } catch {
        print("Caught error: \(error)")
    }
}

If you run this test on Darwin you'll get

Caught error: valueNotFound(Swift.KeyedDecodingContainer<Unit.TestJSONEncoder.(unknown context at $1172997c8).(unknown context at $1172998a4).InnerType.CodingKeys>, Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "super", intValue: nil)], debugDescription: "Cannot get keyed decoding container -- found null value instead.", underlyingError: nil))

This means the error is actually thrown on the line marked with <-----, which seems to be the "correct" error because the null decoder does not have a container keyed by CodingKeys.self.

The JSONDecoderImpl's implementation of .container throws DecodingError.typeMismatch aggressively:

@usableFromInline func container<Key>(keyedBy _: Key.Type) throws ->
    KeyedDecodingContainer<Key> where Key: CodingKey
{
    guard case .object(let dictionary) = self.json else {
        throw DecodingError.typeMismatch([String: JSONValue].self, DecodingError.Context(
            codingPath: self.codingPath,
            debugDescription: "Expected to decode \([String: JSONValue].self) but found \(self.json.debugDataTypeDescription) instead."
        ))
    }

    let container = KeyedContainer<Key>(
        impl: self,
        codingPath: codingPath,
        dictionary: dictionary
    )
    return KeyedDecodingContainer(container)
}

In our test case, self.json would be .null, and one can argue throwing valueNotFound is better than throwing typeMismatch. In fact, the entirety of JSONDecoderImpl seems to favor typeMismatch over valueNotFound, which is different from Darwin's implementation.

[ahoppen]

To be clear, I don’t have a strong preference either way. I just think that if we have a new divergence between the Darwin and Linux implementation, we should decide which implementation we consider correct.

[tomerd]

@adam-fowler @ahoppen @iCharlesHu what is the next step here? If its actionable, would be nice to get into 5.7

[ahoppen]

I think we need some opinion from corelibs-foundation maintainers to make a decision whether we want to throw valueNotFound or keyNotFound. I’m not really in a place to make that call. But I’m not sure who would be the right one to decide that either…

[adam-fowler]

I misread the comment from @iCharlesHu previously. I have now added code to throw valueNotFound errors when creating a keyed or non-keyed container from a null value. I wasn't sure if I should do the same for singleValueContainer. This should mean the Linux implementation is more inline with the Darwin version.

[adam-fowler]

@tomerd the CI for this change has been running for 10 days. Can you find out why.

[tomerd]

cc @shahmishal

@swift-ci please test

[adam-fowler]

I think this is ready to merge now

[tomerd]

@parkera please review / merge

[tomerd]

thanks @adam-fowler, do you want to also bring into 5.7 branch for the next patch release?

[adam-fowler]

@tomerd #4631