Skip to content

JSONSerialization: Add missing numeric types #1197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 5, 2017
Merged

JSONSerialization: Add missing numeric types #1197

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64b67c9
JSONSerialization: Add missing numeric types
spevans Aug 31, 2017
a20d25f
JSONSerialization tests
spevans Sep 1, 2017
38b7293
JSONSerialization: Update error message to match Darwin
spevans Sep 1, 2017
5e435e7
JSONSerialization PR review fixes
spevans Sep 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 86 additions & 107 deletions Foundation/JSONSerialization.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,9 @@ open class JSONSerialization : NSObject {
return true
}

// object is Swift.String, NSNull, Int, Bool, or UInt
if obj is String || obj is NSNull || obj is Int || obj is Bool || obj is UInt {
if obj is String || obj is NSNull || obj is Int || obj is Bool || obj is UInt ||
obj is Int8 || obj is Int16 || obj is Int32 || obj is Int64 ||
obj is UInt8 || obj is UInt16 || obj is UInt32 || obj is UInt64 {
return true
}

Expand All @@ -76,6 +77,10 @@ open class JSONSerialization : NSObject {
return number.isFinite
}

if let number = obj as? Decimal {
return number.isFinite
}

// object is Swift.Array
if let array = obj as? [Any?] {
for element in array {
Expand All @@ -99,8 +104,11 @@ open class JSONSerialization : NSObject {
// object is NSNumber and is not NaN or infinity
// For better performance, this (most expensive) test should be last.
if let number = _SwiftValue.store(obj) as? NSNumber {
let invalid = number.doubleValue.isInfinite || number.doubleValue.isNaN
return !invalid
if CFNumberIsFloatType(number._cfObject) {
return number.doubleValue.isFinite
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line made me do a double-take for a moment because at first glance, it's hard to visually distinguish between number.doubleValue.isFinite and number.doubleValue.isInfinite. Do you mind just calling this out with a comment saying something like "All floating-point numbers except for NaN and infinity are valid." (or similar)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will actually put it back to how it was but store the doubleValue in a local since getting the doubleValue twice involved into 2 calls into CFNumber so was a significant speedup.

} else {
return true
}
}

// invalid object
Expand Down Expand Up @@ -285,9 +293,7 @@ internal extension JSONSerialization {

//MARK: - JSONSerializer
private struct JSONWriter {

private let maxUIntLength = String(describing: UInt.max).count
private let maxIntLength = String(describing: Int.max).count

var indent = 0
let pretty: Bool
let sortedKeys: Bool
Expand Down Expand Up @@ -320,13 +326,37 @@ private struct JSONWriter {
case let boolValue as Bool:
serializeBool(boolValue)
case let num as Int:
try serializeInt(value: num)
serializeInteger(value: num)
case let num as Int8:
serializeInteger(value: num)
case let num as Int16:
serializeInteger(value: num)
case let num as Int32:
serializeInteger(value: num)
case let num as Int64:
serializeInteger(value: num)
case let num as UInt:
try serializeUInt(value: num)
serializeInteger(value: num)
case let num as UInt8:
serializeInteger(value: num)
case let num as UInt16:
serializeInteger(value: num)
case let num as UInt32:
serializeInteger(value: num)
case let num as UInt64:
serializeInteger(value: num)
case let array as Array<Any?>:
try serializeArray(array)
case let dict as Dictionary<AnyHashable, Any?>:
try serializeDictionary(dict)
case let num as Float:
try serializeNumber(NSNumber(value: num))
case let num as Double:
try serializeNumber(NSNumber(value: num))
case let num as Decimal:
writer(num.description)
case let num as NSDecimalNumber:
writer(num.description)
case is NSNull:
try serializeNull()
case _ where _SwiftValue.store(obj) is NSNumber:
Expand All @@ -336,92 +366,33 @@ private struct JSONWriter {
}
}

private func serializeUInt(value: UInt) throws {
if value == 0 {
writer("0")
return
}
var array: [UInt] = []
var stringResult = ""
//Maximum length of an UInt
array.reserveCapacity(maxUIntLength)
stringResult.reserveCapacity(maxUIntLength)
var number = value

while number != 0 {
array.append(number % 10)
private func serializeInteger<T: UnsignedInteger>(value: T, isNegative: Bool = false) {
let maxIntLength = 22 // 20 digits in UInt64 + optional sign + trailing '\0'
let ZERO: CChar = 0x30 // ASCII '0' == 0x30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a purely stylistic standpoint, I don't think we really use all caps anywhere to indicate a constant. I think this would read more clearly as just asciiZero (and MINUS would read a bit better as asciiMinus).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually copied this from Decimal but I will tidy it up.

let MINUS: CChar = 0x2d // ASCII '-' == 0x2d

var number = UInt64(value)
var buffer = Array<CChar>(repeating: 0, count: maxIntLength)
var pos = maxIntLength - 1

repeat {
pos -= 1
buffer[pos] = ZERO + CChar(number % 10)
number /= 10
} while number != 0

if isNegative {
pos -= 1
buffer[pos] = MINUS
}

/*
Step backwards through the array and append the values to the string. This way the values are appended in the correct order.
*/
var counter = array.count
while counter > 0 {
counter -= 1
let digit: UInt = array[counter]
switch digit {
case 0: stringResult.append("0")
case 1: stringResult.append("1")
case 2: stringResult.append("2")
case 3: stringResult.append("3")
case 4: stringResult.append("4")
case 5: stringResult.append("5")
case 6: stringResult.append("6")
case 7: stringResult.append("7")
case 8: stringResult.append("8")
case 9: stringResult.append("9")
default: fatalError()
}
}

writer(stringResult)
let output = String(cString: Array(buffer.suffix(from: pos)))
Copy link
Contributor

@ianpartridge ianpartridge Sep 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this do two copies? One to create a new Array and another to create the String. Can we avoid at least one of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I was allocating a buffer once in the JSONWriter() and using String(bytesNoCopy:) but even that does a copy and then there is another one when the string is appended to the JSON output.
So I decided not to do anything too complicated in this PR and just copied this from Decimal.

In a followup PR I will try converting JSONWriter to allocate the output buffer at the start and just have the serialise methods write directly into it and hopefully avoid any copying.

@djones6 Ran some tests against this change and confirmed that it is faster than the current method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing directly into the output buffer sounds much better - thanks.

writer(output)
}

private func serializeInt(value: Int) throws {
if value == 0 {
writer("0")
return
}
var array: [Int] = []
var stringResult = ""
array.reserveCapacity(maxIntLength)
//Account for a negative sign
stringResult.reserveCapacity(maxIntLength + 1)
var number = value

while number != 0 {
array.append(number % 10)
number /= 10
}
//If negative add minus sign before adding any values
if value < 0 {
stringResult.append("-")
}
/*
Step backwards through the array and append the values to the string. This way the values are appended in the correct order.
*/
var counter = array.count
while counter > 0 {
counter -= 1
let digit = array[counter]
switch digit {
case 0: stringResult.append("0")
case 1, -1: stringResult.append("1")
case 2, -2: stringResult.append("2")
case 3, -3: stringResult.append("3")
case 4, -4: stringResult.append("4")
case 5, -5: stringResult.append("5")
case 6, -6: stringResult.append("6")
case 7, -7: stringResult.append("7")
case 8, -8: stringResult.append("8")
case 9, -9: stringResult.append("9")
default: fatalError()
}
}
writer(stringResult)

private func serializeInteger<T: SignedInteger>(value: T) {
serializeInteger(value: UInt64(value.magnitude), isNegative: value < 0)
}

func serializeString(_ str: String) throws {
writer("\"")
for scalar in str.unicodeScalars {
Expand Down Expand Up @@ -462,15 +433,30 @@ private struct JSONWriter {
}

mutating func serializeNumber(_ num: NSNumber) throws {
if num.doubleValue.isInfinite || num.doubleValue.isNaN {
throw NSError(domain: NSCocoaErrorDomain, code: CocoaError.propertyListReadCorrupt.rawValue, userInfo: ["NSDebugDescription" : "Number cannot be infinity or NaN"])
}

switch num._cfTypeID {
case CFBooleanGetTypeID():
serializeBool(num.boolValue)
default:
writer(_serializationString(for: num))
if CFNumberIsFloatType(num._cfObject) {
let dv = num.doubleValue
if !dv.isFinite {
let value: String
if dv.isNaN {
value = "NaN"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent with NSJSONSerialization, which does not support NaN or infinite values. This should arguably preconditionFailure/fatalError.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Seeing also as we claim in isValidJSONValue that infinite numbers/NaN are not valid, we shouldn't support them here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually to exactly match the error messaging on Darwin, not to serialise the value. value is used in the NSError below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally right, of course — I'd misread this. 😬

} else if dv.isInfinite {
value = "infinite"
} else {
value = String(dv)
}

throw NSError(domain: NSCocoaErrorDomain, code: CocoaError.propertyListReadCorrupt.rawValue, userInfo: ["NSDebugDescription" : "Invalid number value (\(value)) in JSON write"])
}

let string = CFNumberFormatterCreateStringWithNumber(nil, _numberformatter, num._cfObject)._swiftObject
writer(string)
} else {
switch num._cfTypeID {
case CFBooleanGetTypeID():
serializeBool(num.boolValue)
default:
writer(num.stringValue)
}
}
}

Expand Down Expand Up @@ -578,13 +564,6 @@ private struct JSONWriter {
}
}

//[SR-2151] https://bugs.swift.org/browse/SR-2151
private mutating func _serializationString(for number: NSNumber) -> String {
if !CFNumberIsFloatType(number._cfObject) {
return number.stringValue
}
return CFNumberFormatterCreateStringWithNumber(nil, _numberformatter, number._cfObject)._swiftObject
}
}

//MARK: - JSONDeserializer
Expand Down
46 changes: 6 additions & 40 deletions TestFoundation/TestJSONEncoder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,64 +405,30 @@ class TestJSONEncoder : XCTestCase {
// UInt and Int
func test_codingOfUIntMinMax() {

let encoder = JSONEncoder()

struct MyValue: Codable {
let intMin:Int = Int.min
let intMax:Int = Int.max
let uintMin:UInt = UInt.min
let uintMax:UInt = UInt.max
let intMin = Int64.min
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this would be better called int64Min now. No big deal though.

let intMax = Int64.max
let uintMin = UInt64.min
let uintMax = UInt64.max
}

let myValue = MyValue()
let myDictI: [String:Any] = ["intMin": myValue.intMin, "intMax": myValue.intMax]
let myDictU: [String:Any] = ["uintMin": myValue.uintMin, "uintMax": myValue.uintMax]
let myDict1: [String:Any] = ["intMin": myValue.intMin]
let myDict2: [String:Any] = ["intMax": myValue.intMax]
let myDict3: [String:Any] = ["uintMin": myValue.uintMin]
let myDict4: [String:Any] = ["uintMax": myValue.uintMax]

func compareJSON(_ s1: String, _ s2: String) {
let ss1 = s1.trimmingCharacters(in: CharacterSet(charactersIn: "{}")).split(separator: Character(",")).sorted()
let ss2 = s2.trimmingCharacters(in: CharacterSet(charactersIn: "{}")).split(separator: Character(",")).sorted()
XCTAssertEqual(ss1, ss2)
}

do {
let encoder = JSONEncoder()
let myValue = MyValue()
let result = try encoder.encode(myValue)
let r = String(data: result, encoding: .utf8) ?? "nil"
compareJSON(r, "{\"uintMin\":0,\"uintMax\":18446744073709551615,\"intMin\":-9223372036854775808,\"intMax\":9223372036854775807}")

let resultI = try JSONSerialization.data(withJSONObject: myDictI)
let rI = String(data: resultI, encoding: .utf8) ?? "nil"
compareJSON(rI, "{\"intMin\":-9223372036854775808,\"intMax\":9223372036854775807}")

let resultU = try JSONSerialization.data(withJSONObject: myDictU)
let rU = String(data: resultU, encoding: .utf8) ?? "nil"
compareJSON(rU, "{\"uintMax\":18446744073709551615,\"uintMin\":0}")

let result1 = try JSONSerialization.data(withJSONObject: myDict1)
let r1 = String(data: result1, encoding: .utf8) ?? "nil"
XCTAssertEqual(r1, "{\"intMin\":-9223372036854775808}")

let result2 = try JSONSerialization.data(withJSONObject: myDict2)
let r2 = String(data: result2, encoding: .utf8) ?? "nil"
XCTAssertEqual(r2, "{\"intMax\":9223372036854775807}")

let result3 = try JSONSerialization.data(withJSONObject: myDict3)
let r3 = String(data: result3, encoding: .utf8) ?? "nil"
XCTAssertEqual(r3, "{\"uintMin\":0}")

let result4 = try JSONSerialization.data(withJSONObject: myDict4)
let r4 = String(data: result4, encoding: .utf8) ?? "nil"
XCTAssertEqual(r4, "{\"uintMax\":18446744073709551615}")
} catch {
XCTFail(String(describing: error))
}
}



// MARK: - Helper Functions
private var _jsonEmptyDictionary: Data {
return "{}".data(using: .utf8)!
Expand Down
Loading