Fix semantics of JSONUnkeyedDecodingContainer (#62)

The following behaviors are now handled correctly:

- It is no longer possible for `self.count` or `self.isAtEnd` to accidentally get out of sync with reality, they are now computed.
- Bounds are now correctly checked and a more useful `DecodingError` thrown (may have performance implications; removing this check is not strictly against protocol if so).
- Decoding no longer incorrectly increments the current index if the decode fails for any reason.
- `decodeNil()` no longer incorrectly increments the current index if the result is not `nil`.

Author: gwynne

.github/workflows/ci.yaml

@@ -9,15 +9,15 @@ on:
 
 jobs:
 
-  "sanity-Tests":
+  "validity-Tests":
     runs-on: macOS-latest
     steps:
     - name: Checkout
       uses: actions/checkout@v2
     - name: Install swiftformat
       run: brew install swiftformat
-    - name: Run sanity
-      run: ./scripts/sanity.sh .
+    - name: Run validity
+      run: ./scripts/validity.sh .
 
   "tuxOS-Tests":
     runs-on: ubuntu-latest

Sources/PureSwiftJSON/Decoding/JSONUnkeyedDecodingContainer.swift

@@ -4,27 +4,19 @@ struct JSONUnkeyedDecodingContainer: UnkeyedDecodingContainer {
     let codingPath: [CodingKey]
     let array: [JSONValue]
 
-    let count: Int? // protocol requirement to be optional
-    var isAtEnd: Bool
+    var count: Int? { self.array.count }
+    var isAtEnd: Bool { self.currentIndex >= (self.count ?? 0) }
     var currentIndex = 0
 
     init(impl: JSONDecoderImpl, codingPath: [CodingKey], array: [JSONValue]) {
         self.impl = impl
         self.codingPath = codingPath
         self.array = array
-
-        self.isAtEnd = array.count == 0
-        self.count = array.count
     }
 
     mutating func decodeNil() throws -> Bool {
-        if self.array[self.currentIndex] == .null {
-            defer {
-                currentIndex += 1
-                if currentIndex == count {
-                    isAtEnd = true
-                }
-            }
+        if try self.getNextValue(ofType: Never.self) == .null {
+            self.currentIndex += 1
             return true
         }
 
@@ -34,41 +26,31 @@ struct JSONUnkeyedDecodingContainer: UnkeyedDecodingContainer {
     }
 
     mutating func decode(_ type: Bool.Type) throws -> Bool {
-        defer {
-            currentIndex += 1
-            if currentIndex == count {
-                isAtEnd = true
-            }
-        }
-
-        guard case .bool(let bool) = self.array[self.currentIndex] else {
-            throw createTypeMismatchError(type: type, value: self.array[self.currentIndex])
+        let value = try self.getNextValue(ofType: Bool.self)
+        guard case .bool(let bool) = value else {
+            throw createTypeMismatchError(type: type, value: value)
         }
 
+        self.currentIndex += 1
         return bool
     }
 
     mutating func decode(_ type: String.Type) throws -> String {
-        defer {
-            currentIndex += 1
-            if currentIndex == count {
-                isAtEnd = true
-            }
-        }
-
-        guard case .string(let string) = self.array[self.currentIndex] else {
-            throw createTypeMismatchError(type: type, value: self.array[self.currentIndex])
+        let value = try self.getNextValue(ofType: String.self)
+        guard case .string(let string) = value else {
+            throw createTypeMismatchError(type: type, value: value)
         }
 
+        self.currentIndex += 1
         return string
     }
 
     mutating func decode(_: Double.Type) throws -> Double {
-        try decodeLosslessStringConvertible()
+        try decodeBinaryFloatingPoint()
     }
 
     mutating func decode(_: Float.Type) throws -> Float {
-        try decodeLosslessStringConvertible()
+        try decodeBinaryFloatingPoint()
     }
 
     mutating func decode(_: Int.Type) throws -> Int {
@@ -112,20 +94,33 @@ struct JSONUnkeyedDecodingContainer: UnkeyedDecodingContainer {
     }
 
     mutating func decode<T>(_: T.Type) throws -> T where T: Decodable {
-        let decoder = try decoderForNextElement()
-        return try T(from: decoder)
+        let decoder = try decoderForNextElement(ofType: T.self)
+        let result = try T(from: decoder)
+
+        // Because of the requirement that the index not be incremented unless
+        // decoding the desired result type succeeds, it can not be a tail call.
+        // Hopefully the compiler still optimizes well enough that the result
+        // doesn't get copied around.
+        self.currentIndex += 1
+        return result
     }
 
     mutating func nestedContainer<NestedKey>(keyedBy type: NestedKey.Type) throws
         -> KeyedDecodingContainer<NestedKey> where NestedKey: CodingKey
     {
-        let decoder = try decoderForNextElement()
-        return try decoder.container(keyedBy: type)
+        let decoder = try decoderForNextElement(ofType: KeyedDecodingContainer<NestedKey>.self, isNested: true)
+        let container = try decoder.container(keyedBy: type)
+
+        self.currentIndex += 1
+        return container
     }
 
     mutating func nestedUnkeyedContainer() throws -> UnkeyedDecodingContainer {
-        let decoder = try decoderForNextElement()
-        return try decoder.unkeyedContainer()
+        let decoder = try decoderForNextElement(ofType: UnkeyedDecodingContainer.self, isNested: true)
+        let container = try decoder.unkeyedContainer()
+
+        self.currentIndex += 1
+        return container
     }
 
     mutating func superDecoder() throws -> Decoder {
@@ -134,17 +129,9 @@ struct JSONUnkeyedDecodingContainer: UnkeyedDecodingContainer {
 }
 
 extension JSONUnkeyedDecodingContainer {
-    private mutating func decoderForNextElement() throws -> JSONDecoderImpl {
-        defer {
-            currentIndex += 1
-            if currentIndex == count {
-                isAtEnd = true
-            }
-        }
-
-        let value = self.array[self.currentIndex]
-        var newPath = self.codingPath
-        newPath.append(ArrayKey(index: self.currentIndex))
+    private mutating func decoderForNextElement<T>(ofType: T.Type, isNested: Bool = false) throws -> JSONDecoderImpl {
+        let value = try self.getNextValue(ofType: T.self, isNested: isNested)
+        let newPath = self.codingPath + [ArrayKey(index: self.currentIndex)]
 
         return JSONDecoderImpl(
             userInfo: self.impl.userInfo,
@@ -153,6 +140,34 @@ extension JSONUnkeyedDecodingContainer {
         )
     }
 
+    /// - Note: Instead of having the `isNested` parameter, it would have been quite nice to just check whether
+    ///   `T` conforms to either `KeyedDecodingContainer` or `UnkeyedDecodingContainer`. Unfortunately, since
+    ///   `KeyedDecodingContainer` takes a generic parameter (the `Key` type), we can't just ask if `T` is one, and
+    ///   type-erasure workarounds are not appropriate to this use case due to, among other things, the inability to
+    ///   conform most of the types that would matter. We also can't use `KeyedDecodingContainerProtocol` for the
+    ///   purpose, as it isn't even an existential and conformance to it can't be checked at runtime at all.
+    ///
+    ///   However, it's worth noting that the value of `isNested` is always a compile-time constant and the compiler
+    ///   can quite neatly remove whichever branch of the `if` is not taken during optimization, making doing it this
+    ///   way _much_ more performant (for what little it matters given that it's only checked in case of an error).
+    @inline(__always)
+    private func getNextValue<T>(ofType: T.Type, isNested: Bool = false) throws -> JSONValue {
+        guard !self.isAtEnd else {
+            if isNested {
+                throw DecodingError.valueNotFound(T.self,
+                                                  .init(codingPath: self.codingPath,
+                                                        debugDescription: "Cannot get nested keyed container -- unkeyed container is at end.",
+                                                        underlyingError: nil))
+            } else {
+                throw DecodingError.valueNotFound(T.self,
+                                                  .init(codingPath: [ArrayKey(index: self.currentIndex)],
+                                                        debugDescription: "Unkeyed container is at end.",
+                                                        underlyingError: nil))
+            }
+        }
+        return self.array[self.currentIndex]
+    }
+
     @inline(__always) private func createTypeMismatchError(type: Any.Type, value: JSONValue) -> DecodingError {
         let codingPath = self.codingPath + [ArrayKey(index: self.currentIndex)]
         return DecodingError.typeMismatch(type, .init(
@@ -161,42 +176,32 @@ extension JSONUnkeyedDecodingContainer {
     }
 
     @inline(__always) private mutating func decodeFixedWidthInteger<T: FixedWidthInteger>() throws -> T {
-        defer {
-            currentIndex += 1
-            if currentIndex == count {
-                isAtEnd = true
-            }
-        }
-
-        guard case .number(let number) = self.array[self.currentIndex] else {
-            throw self.createTypeMismatchError(type: T.self, value: self.array[self.currentIndex])
+        let value = try self.getNextValue(ofType: T.self)
+        guard case .number(let number) = value else {
+            throw self.createTypeMismatchError(type: T.self, value: value)
         }
 
         guard let integer = T(number) else {
             throw DecodingError.dataCorruptedError(in: self,
                                                    debugDescription: "Parsed JSON number <\(number)> does not fit in \(T.self).")
         }
 
+        self.currentIndex += 1
         return integer
     }
 
-    @inline(__always) private mutating func decodeLosslessStringConvertible<T: LosslessStringConvertible>() throws -> T {
-        defer {
-            currentIndex += 1
-            if currentIndex == count {
-                isAtEnd = true
-            }
-        }
-
-        guard case .number(let number) = self.array[self.currentIndex] else {
-            throw self.createTypeMismatchError(type: T.self, value: self.array[self.currentIndex])
+    @inline(__always) private mutating func decodeBinaryFloatingPoint<T: LosslessStringConvertible>() throws -> T {
+        let value = try self.getNextValue(ofType: T.self)
+        guard case .number(let number) = value else {
+            throw self.createTypeMismatchError(type: T.self, value: value)
         }
 
         guard let float = T(number) else {
             throw DecodingError.dataCorruptedError(in: self,
                                                    debugDescription: "Parsed JSON number <\(number)> does not fit in \(T.self).")
         }
 
+        self.currentIndex += 1
         return float
     }
 }

Tests/LearningTests/FoundationJSONDecoderTests.swift

@@ -268,4 +268,73 @@ class FoundationJSONDecoderTests: XCTestCase {
             XCTFail("Unexpected error: \(error)")
         }
     }
+
+    func testDecodePastEndOfUnkeyedContainer() {
+        struct HelloWorld: Decodable {
+            enum CodingKeys: String, CodingKey { case list }
+
+            init(from decoder: Decoder) throws {
+                var container = try decoder.container(keyedBy: CodingKeys.self).nestedUnkeyedContainer(forKey: .list)
+                _ = try container.decode(String.self)
+            }
+        }
+
+        let json = #"{"list":[]}"#
+
+        XCTAssertThrowsError(_ = try JSONDecoder().decode(HelloWorld.self, from: json.data(using: .utf8)!)) { error in
+            guard case .valueNotFound(let type, let context) = (error as? DecodingError) else {
+                return XCTFail("Unexpected error: \(error)")
+            }
+            XCTAssertTrue(type is String.Type)
+            XCTAssertEqual(context.codingPath.count, 1)
+            XCTAssertEqual(context.codingPath.first?.intValue, 0)
+            XCTAssertEqual(context.debugDescription, "Unkeyed container is at end.")
+        }
+    }
+
+    func testDecodeNestedKeyedContainerPastEndOfUnkeyedContainer() {
+        struct HelloWorld: Decodable {
+            enum CodingKeys: String, CodingKey { case list }
+
+            init(from decoder: Decoder) throws {
+                var container = try decoder.container(keyedBy: CodingKeys.self).nestedUnkeyedContainer(forKey: .list)
+                _ = try container.nestedContainer(keyedBy: CodingKeys.self)
+            }
+        }
+
+        let json = #"{"list":[]}"#
+
+        XCTAssertThrowsError(_ = try JSONDecoder().decode(HelloWorld.self, from: json.data(using: .utf8)!)) { error in
+            guard case .some(.valueNotFound(let type, let context)) = (error as? DecodingError) else {
+                return XCTFail("Unexpected error: \(error)")
+            }
+            XCTAssertTrue(type is KeyedDecodingContainer<HelloWorld.CodingKeys>.Type)
+            XCTAssertEqual(context.codingPath.count, 1)
+            XCTAssertEqual(context.codingPath.first?.stringValue, HelloWorld.CodingKeys.list.rawValue)
+            XCTAssertEqual(context.debugDescription, "Cannot get nested keyed container -- unkeyed container is at end.")
+        }
+    }
+
+    func testDecodeNestedUnkeyedContainerPastEndOfUnkeyedContainer() {
+        struct HelloWorld: Decodable {
+            enum CodingKeys: String, CodingKey { case list }
+
+            init(from decoder: Decoder) throws {
+                var container = try decoder.container(keyedBy: CodingKeys.self).nestedUnkeyedContainer(forKey: .list)
+                _ = try container.nestedUnkeyedContainer()
+            }
+        }
+
+        let json = #"{"list":[]}"#
+
+        XCTAssertThrowsError(_ = try JSONDecoder().decode(HelloWorld.self, from: json.data(using: .utf8)!)) { error in
+            guard case .some(.valueNotFound(let type, let context)) = (error as? DecodingError) else {
+                return XCTFail("Unexpected error: \(error)")
+            }
+            XCTAssertTrue(type is UnkeyedDecodingContainer.Protocol)
+            XCTAssertEqual(context.codingPath.count, 1)
+            XCTAssertEqual(context.codingPath.first?.stringValue, HelloWorld.CodingKeys.list.rawValue)
+            XCTAssertEqual(context.debugDescription, "Cannot get nested keyed container -- unkeyed container is at end.")
+        }
+    }
 }