JSONSerialization: Add missing numeric types #1197

spevans · 2017-09-01T20:04:03Z

Add support for Int8/Int16/Int32/Int64, UInt8/UInt16/UInt32/UInt64,
Decimal and NSDecimalNumber.
Improve serializeInteger performance to avoid overall speed regression.
Move JSONSerialization tests into correct testfile and use Int64 to stop tests
breaking on 32bit platforms.
Add tests for isValidJSONObject() to test the support of new types.
Add tests for Float, Double, Decimal and NSDecimalNumber.

This PR does not address the output format of Float and Double, which are not currently the same as Foundation. That will be addressed in a follow-up PR

- Add support for Int8/Int16/Int32/Int64, UInt8/UInt16/UInt32/UInt64, Decimal and NSDecimalNumber. - Improve serializeInteger performance to avoid overall speed regression.

- Move JSONSerialization tests into correct testfile and use Int64 to stop tests breaking on 32bit platforms. - Add tests for isValidJSONObject() to test the support of new types. - Add tests for Float, Double, Decimal and NSDecimalNumber.

ianpartridge · 2017-09-01T20:06:18Z

Foundation/JSONSerialization.swift

@@ -63,7 +63,9 @@ open class JSONSerialization : NSObject {
            }

            // object is Swift.String, NSNull, Int, Bool, or UInt


Delete this comment, which is now wrong (and was pointless anyway)?

ianpartridge · 2017-09-01T20:09:18Z

Foundation/JSONSerialization.swift

-        }
-
-        writer(stringResult)
+        let output = String(cString: Array(buffer.suffix(from: pos)))


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?

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.

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

ianpartridge · 2017-09-01T20:15:28Z

Foundation/JSONSerialization.swift

-            writer(_serializationString(for: num))
+        if CFNumberIsFloatType(num._cfObject) {
+            if !num.doubleValue.isFinite {
+                throw NSError(domain: NSCocoaErrorDomain, code: CocoaError.propertyListReadCorrupt.rawValue, userInfo: ["NSDebugDescription" : "Number cannot be infinity or NaN"])


Perhaps this should say "Number must be finite" now. Or is the string exactly matching Darwin?

I updated it to emit the same message as Darwin

ianpartridge

Looks good, just a few comments.

ianpartridge · 2017-09-01T20:18:33Z

@swift-ci please test

ianpartridge · 2017-09-04T09:46:08Z

@swift-ci please test

ianpartridge · 2017-09-04T09:53:51Z

TestFoundation/TestJSONEncoder.swift

-            let intMax:Int = Int.max
-            let uintMin:UInt = UInt.min
-            let uintMax:UInt = UInt.max
+            let intMin = Int64.min


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

itaiferber

Looks reasonable overall — thanks for going through and doing this. Some minor comments below, but the major things I think we should fix are taking out support for infinity and NaN, and writing out floating-point values in scientific notation.

itaiferber · 2017-09-05T17:27:05Z

Foundation/JSONSerialization.swift

-            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


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

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

itaiferber · 2017-09-05T17:28:37Z

Foundation/JSONSerialization.swift

+            if !dv.isFinite {
+                let value: String
+                if dv.isNaN {
+                    value = "NaN"


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

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

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

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

itaiferber · 2017-09-05T17:31:13Z

TestFoundation/TestJSONSerialization.swift

+
+    func test_serialize_Float() {
+        XCTAssertEqual(try trySerialize([-Float.leastNonzeroMagnitude, Float.leastNonzeroMagnitude]), "[-0,0]")
+        XCTAssertEqual(try trySerialize([-Float.greatestFiniteMagnitude]), "[-340282346638529000000000000000000000000]")


This value (and all floating-point values below, actually) is inconsistent with NSJSONSerialization, which outputs floating-point numbers in scientific notation (via snprintf("%*.g", DBL_DECIMAL_DIG, ...)). We should likely bridge that gap.

I did try and play around with the Float and Double output formats but it broke some other tests. I will see if I can find a better solution - I was actually intending to do a follow-up PR to fix up the output formatting.

itaiferber · 2017-09-05T17:35:11Z

Foundation/JSONSerialization.swift

-                let invalid = number.doubleValue.isInfinite || number.doubleValue.isNaN
-                return !invalid
+                if CFNumberIsFloatType(number._cfObject) {
+                    return number.doubleValue.isFinite


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

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.

itaiferber · 2017-09-05T17:36:49Z

TestFoundation/TestJSONSerialization.swift

+        XCTAssertTrue(JSONSerialization.isValidJSONObject([Decimal(123), Decimal(Double.leastNonzeroMagnitude)]))
+
+        XCTAssertFalse(JSONSerialization.isValidJSONObject(Float.nan))
+        XCTAssertFalse(JSONSerialization.isValidJSONObject(Float.infinity))


For completeness' sake, it's worth also asserting this on negative infinity (below as well).

spevans · 2017-09-05T18:59:27Z

I have fixed all of the outstanding comments expect for the Float/Double output format which I will fix in a followup PR.

itaiferber

LGTM; thanks!

itaiferber · 2017-09-05T19:07:49Z

@swift-ci Please test

spevans · 2017-09-05T21:20:06Z

@itaiferber Looks like the test worked on CI, is this ok to merge now?

itaiferber · 2017-09-05T21:21:55Z

@spevans Which builder did this build on? I don't see any GitHub CI state for it (likely a GitHub issue)

spevans added 2 commits September 1, 2017 20:34

JSONSerialization: Add missing numeric types

64b67c9

- Add support for Int8/Int16/Int32/Int64, UInt8/UInt16/UInt32/UInt64, Decimal and NSDecimalNumber. - Improve serializeInteger performance to avoid overall speed regression.

JSONSerialization tests

a20d25f

- Move JSONSerialization tests into correct testfile and use Int64 to stop tests breaking on 32bit platforms. - Add tests for isValidJSONObject() to test the support of new types. - Add tests for Float, Double, Decimal and NSDecimalNumber.

ianpartridge reviewed Sep 1, 2017

View reviewed changes

ianpartridge suggested changes Sep 1, 2017

View reviewed changes

JSONSerialization: Update error message to match Darwin

38b7293

ianpartridge reviewed Sep 4, 2017

View reviewed changes

ianpartridge approved these changes Sep 4, 2017

View reviewed changes

spevans requested a review from phausler September 4, 2017 11:45

parkera requested a review from itaiferber September 5, 2017 17:14

itaiferber suggested changes Sep 5, 2017

View reviewed changes

JSONSerialization PR review fixes

5e435e7

itaiferber approved these changes Sep 5, 2017

View reviewed changes

itaiferber merged commit 7a5af0f into swiftlang:master Sep 5, 2017

spevans deleted the pr_json_serialise_more_types branch September 19, 2017 15:04

djones6 mentioned this pull request Sep 20, 2017

Enable JSONSerialization Int fastpath for boxed values #1169

Closed

ianpartridge mentioned this pull request Oct 9, 2017

[4.0] JSONSerialization: Add missing numeric types #1255

Merged

		@@ -63,7 +63,9 @@ open class JSONSerialization : NSObject {
		}

		// object is Swift.String, NSNull, Int, Bool, or UInt

JSONSerialization: Add missing numeric types #1197

JSONSerialization: Add missing numeric types #1197

Uh oh!

Conversation

spevans commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge left a comment

Choose a reason for hiding this comment

Uh oh!

ianpartridge commented Sep 1, 2017

Uh oh!

ianpartridge commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spevans commented Sep 5, 2017

Uh oh!

itaiferber left a comment

Choose a reason for hiding this comment

Uh oh!

itaiferber commented Sep 5, 2017

Uh oh!

spevans commented Sep 5, 2017

Uh oh!

itaiferber commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ianpartridge Sep 1, 2017 •

edited

Loading

itaiferber commented Sep 5, 2017 •

edited

Loading